Now that dependency injection has made it into core: #1497230: Use Dependency Injection to handle object definitions we should reduce the number of things we put into the $GLOBALS array, starting with things that touch the site's language .

On line 168 of language.admin.inc there is a $GLOBALS['language_interface']->langcode
On line 317 of language.negotionation.inc there is a $GLOBALS[$type]->langcode
In language.test there are many references to $GLOBALS that can be updated to use depenedency injection.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Component: language system » language.module
Issue tags: +Dependency Injection (DI)
cosmicdreams’s picture

found and fixed these issues. please review

cosmicdreams’s picture

Status: Active » Needs review
RobLoach’s picture

Priority: Normal » Minor
Status: Needs review » Reviewed & tested by the community
Dries’s picture

I think this may need to be re-rolled. Asking for a re-test.

Dries’s picture

#2: 1539614_2_language_inc_di.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Dependency Injection (DI)

The last submitted patch, 1539614_2_language_inc_di.patch, failed testing.

cosmicdreams’s picture

FileSize
1.85 KB

rerolled

cosmicdreams’s picture

Status: Needs work » Needs review
cosmicdreams’s picture

#8: 1539614_8_language_di.patch queued for re-testing.

cosmicdreams’s picture

I'm starting to think that this is the wrong approach but want to check my logic here:

This is the wrong way to use the Dependency Injection Container. Methods and arguments to those methods are things that should be in the DIC, not static values for this we want to persist. That's what config() is for.

Therefore the code for these settings should be modified so that we aren't storing them in the container but instead in config.

Is that wise?

cosmicdreams’s picture

Assigned: Unassigned » cosmicdreams

Assigning to me so I can show these to folks easier

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +D8MI, +language-base
+++ b/core/modules/language/language.negotiation.incundefined
@@ -313,7 +313,7 @@ function language_switcher_url($type, $path) {
 function language_switcher_session($type, $path) {
   $param = variable_get('language_negotiation_session_param', 'language');
-  $language_query = isset($_SESSION[$param]) ? $_SESSION[$param] : $GLOBALS[$type]->langcode;
+  $language_query = isset($_SESSION[$param]) ? $_SESSION[$param] : drupal_container()->get(LANGUAGE_TYPE_INTERFACE)->langcode;

You should use $type, and not hardcode a specific type.

+++ b/core/modules/language/language.testundefined
@@ -185,23 +185,6 @@ class LanguageDependencyInjectionTest extends DrupalWebTestCase {
   /**
-   * Test dependency injected Language against the GLOBAL language object.
-   *
-   * @todo Once the PHP global is gone, we won't need this test as the same
-   * test is done without the PHP global in the following test.
-   */
-  function testDependencyInjectedLanguage() {
-    // Initialize the language system.
-    drupal_language_initialize();
-
-    $expected = $GLOBALS[LANGUAGE_TYPE_INTERFACE];
-    $result = drupal_container()->get(LANGUAGE_TYPE_INTERFACE);
-    foreach ($expected as $property => $value) {
-      $this->assertEqual($expected->$property, $result->$property, t('The dependency injected language object %prop property equals the $GLOBAL language object %prop property.', array('%prop' => $property)));
-    }
-  }

Why are you removing the test?

vasi1186’s picture

Assigned: cosmicdreams » vasi1186
Issue tags: +sprint
vasi1186’s picture

Status: Needs work » Needs review
Issue tags: -D8MI, -sprint, -language-base, -Dependency Injection (DI)

I added a patch that:
1. Uses $type instead of LANGUAGE_TYPE_INTERFACE from the patch in 8
2. Uses the drupal_container() also in language_test_store_language_negotiation().

The testDependencyInjectedLanguage() does not make sense after the php global is gone, as it says in its comment, so probably that's why cosmicdreams removed it from core.

vasi1186’s picture

for some reason, the patch was not attached in the previous comment...

vasi1186’s picture

Add again the missing tags.

RobLoach’s picture

Actually, I think we're covering this in #1510686: Replace remaining references to global $language_interface with drupal_container(). Correct me if I'm wrong, but should this be set as duplicate? Removing the test should probably happen when we remove the GLOBALS initialization code block.

RobLoach’s picture

Didn't mean to remove tags.

Gábor Hojtsy’s picture

Rob: I've had the same thought about the test, but the DI issues do overlap in many ways :) This one is about the language module, that one is about a language global. The code they cover overlap, so whichever does the removal should be fine IMHO. Otherwise I think this patch looks good.

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

#15:

Yep, that was the sum total of my reasoning. Reviewed #16 and it looks solid. I vote RTBC.

Gábor Hojtsy’s picture

#16: 1539614_16_language_di.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

this looks like more of the same at #1510686: Replace remaining references to global $language_interface with drupal_container() which Dries already committed. I was confused about the missing test but #15 clarified. (Thanks!)

Courtesy of the #d8mi sprint at #drupaldevdays, committed and pushed to 8.x! :)

Gábor Hojtsy’s picture

Issue tags: -sprint

Done, moving off sprint.

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