drupal_container() is deprecated, and all calls in the translation_entity module need to be replaced with Drupal::service(), except for where the module_handler service is requested, which needs to be replaced with Drupal::moduleHandler() (see #1957154: Replace calls to drupal_container()->get('module_handler') service with Drupal::moduleHandler())

This task a part of #2001206: Replace drupal_container() with Drupal::service()

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kgoel’s picture

Assigned: Unassigned » kgoel
kgoel’s picture

Status: Active » Needs review
FileSize
3.33 KB
ddrozdik’s picture

Status: Needs review » Needs work
--- a/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.php
+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.php
@@ -435,7 +435,7 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
-    $attributes = drupal_container()->get('request')->attributes;
+    $attributes = \Drupal::service('request')->attributes;

We should use \Drupal::request() instead \Drupal::service('request').

kgoel’s picture

Status: Needs work » Needs review
FileSize
2.49 KB
3.31 KB

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, translation-entity-2014033-4.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Novice

#4: translation-entity-2014033-4.patch queued for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

#4 looks good to me. I didn't find any more uses of drupal_container() in translation_entity.

dcam’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, I was mistaken. $this->container->get() should be used instead of \Drupal::service in tests.

kgoel’s picture

dcam’s picture

Yes. That comment is what made me set the status back to NW. Were you referring me to that post due to my mistake or to my retraction?

kgoel’s picture

@dcam - not pointing for any mistake but referred that link because @DmitryDrozdik said to use $this->container->get() instead \Drupal::service in tests.

dcam’s picture

@kgoel - Ok, that's cool. Thank you for trying to help me out.

kgoel’s picture

@dcam - ah, nevermind

kgoel’s picture

Status: Needs work » Needs review
FileSize
841 bytes
3.32 KB
ddrozdik’s picture

patch #14 looks good, if bot status will be green - RTBC

dcam’s picture

Status: Needs review » Reviewed & tested by the community

Agreed, #14 is RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Unfortunately, this no longer applies for me.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
2.49 KB

Rerolling the patch. As a matter of fact, there is no change against the EntityTranslationSyncImageTest.php file anymore, as the latest tree already has that change.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

So the module has been renamed in #2024867: Rename translation_entity to content_translation

So this needs a reroll...

Gábor Hojtsy’s picture

Title: Replace drupal_container() with Drupal::service() in the translation_entity module » Replace drupal_container() with Drupal::service() in the content_translation module
Component: translation_entity.module » content_translation.module
hussainweb’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.58 KB

Here is the reroll.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: -Novice
@@ -435,7 +435,7 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
     // Set contextual information that can be reused during the storage phase.
     // @todo Remove this once we have an EntityLanguageDecorator to deal with
     //   the active language.
-    $attributes = drupal_container()->get('request')->attributes;
+    $attributes = \Drupal::request()->attributes;
     $attributes->set('working_langcode', $form_langcode);
     $attributes->set('source_langcode', $source_langcode);

This should inject the request object properly if not already. Instead of using \Drupal direct.

hussainweb’s picture

I am thinking about how to inject the object.

My first thought was to check the caller. I see that this function is called through Form API's #entity_bundles. These functions are called with only those four parameters:

      foreach ($form['#entity_builders'] as $function) {
        call_user_func_array($function, array($entity_type, $entity, &$form, &$form_state));
      }

On the other hand, this can be injected in the object itself when the '#entity_bundles' is defined, but the documentation for Drupal::request() says this:

If this method must be used, never save the request object that is returned. Doing so may lead to inconsistencies as the request object is volatile and may change at various times, such as during a subrequest.

The last way that I can see is the recommended method in the documentation for Drupal::request().

If code must access the request, it is considerably better to register an object with the Service Container and give it a setRequest() method that is configured to run when the service is created. That way, the correct request object can always be provided by the container and the service can still be unit tested.

However, I am not entirely sure how to go about this. If someone can point me in the right direction, I can take it up, or at least I hope this analysis proves useful.

plach’s picture

Status: Needs work » Reviewed & tested by the community

I think we may want to avoid properly injecting the request since that piece code is going to be killed as soon as it is feasible and replaced with plain access to entity fields, hence we won't depend on the request anymore. Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed fd3884c and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-content

Retroactive tagging.

Automatically closed -- issue fixed for 2 weeks with no activity.