The time for replacing our globals and global constants has come. This issue is to clean up the remaining $GLOBALS['language_interface'] and LANGUAGE_TYPE_INTERFACE references from #1497230: Use Dependency Injection to handle object definitions . It should use 'language.interface', as Symfony namespaces their variables with a "." .

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Status: Active » Postponed
RobLoach’s picture

Title: Replace global $language_interface with Dependency Injection » Replace remaining references to global $language_interface with drupal_container()
plach’s picture

Issue tags: +language-base
cosmicdreams’s picture

Status: Postponed » Active

Now that the dependency injection patch is in we can actively work on this.

sun’s picture

cosmicdreams’s picture

Think I got them all

cosmicdreams’s picture

Status: Active » Needs review

I'd like to know how this tests.

Status: Needs review » Needs work

The last submitted patch, 1510686_6_di_language_interface.patch, failed testing.

cosmicdreams’s picture

Same patch but I added back the code that kept $GLOBALS working properly for language_content and language_url

cosmicdreams’s picture

Status: Needs work » Needs review

try again

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

The last submitted patch, 1510686_9_language_interface.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review

#9: 1510686_9_language_interface.patch queued for re-testing.

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

The last submitted patch, 1510686_9_language_interface.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
5.46 KB
25.36 KB

Attached patch is an interdiff for the above patch that failed testing. I also fixed a few more instances that the above patch missed.

vasi1186’s picture

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

The patch in #14 looks good, it seems to replace all the global $language_interface with drupal_container()->get(LANGUAGE_TYPE_INTERFACE), I found only 2 small things:
1. In language.admin.inc file, language_admin_overview_form_submit(), it adds

$language_interface = drupal_container()->get(LANGUAGE_TYPE_INTERFACE);

but the variable is not used anymore in that function.

2. This is not coming actually from the patch, it existed before.. in core/modules/system/tests/upgrade/upgrade.test, in the setUp() function, the same issue as 1., the $language_interface is declared, but not used in the function.
I attached the patch that should solve these two issues.
Otherwise, everything looks fine for me...

Gábor Hojtsy’s picture

Assigned: vasi1186 » Unassigned
Issue tags: -sprint

On review, this looks good. I have two questions about the end though:

+++ b/core/update.phpundefined
@@ -383,16 +383,11 @@ update_prepare_d8_bootstrap();
-// The interface language global has been renamed in D8, we must ensure that it
-// contains a valid value while language settings are upgraded.
-// @todo Remove this globals reference entirely: http://drupal.org/node/1510686
-$GLOBALS[LANGUAGE_TYPE_INTERFACE] = language_default();

The comment says it would be important for the upgrade. Is that not anymore valid after this patch? What's the fix in the upgrade path in the patch that makes this unnecessary?

+++ b/core/update.phpundefined
@@ -383,16 +383,11 @@ update_prepare_d8_bootstrap();
 // Ensure the default language is properly registered within the Dependency
 // Injection container during the upgrade process.
 $default = language_default();
-drupal_container()->register(LANGUAGE_TYPE_INTERFACE, 'Drupal\\Core\\Language\\Language')
-  ->addMethodCall('extend', array($default));
+drupal_container()->register(LANGUAGE_TYPE_INTERFACE, 'Drupal\\Core\\Language\\Language');

$default is retrieved but then not used. Also the comment still references it. Why can you now get away with not initializing it here yet (how did this change with this patch)?

vasi1186’s picture

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

So, from what I see, the idea is to make sure we have a valid value for the former $GLOBALS[LANGUAGE_TYPE_INTERFACE] variable. The current code is somethings like this:

// The interface language global has been renamed in D8, we must ensure that it
// contains a valid value while language settings are upgraded.
// @todo Remove this globals reference entirely: http://drupal.org/node/1510686
$GLOBALS[LANGUAGE_TYPE_INTERFACE] = language_default();

// Ensure the default language is properly registered within the Dependency
// Injection container during the upgrade process.
$default = language_default();
drupal_container()->register(LANGUAGE_TYPE_INTERFACE, 'Drupal\\Core\\Language\\Language')
  ->addMethodCall('extend', array($default));

and what I see it does is basically to put the default language in $GLOBALS and in the same time register the LANGUAGE_TYPE_INTERFACE service with the Language class from core.
The "$GLOBALS[LANGUAGE_TYPE_INTERFACE] = language_default();" we do not need anymore, after this issue is closed, and also the

drupal_container()->register(LANGUAGE_TYPE_INTERFACE, 'Drupal\\Core\\Language\\Language')
  ->addMethodCall('extend', array($default));

can be replaced by

drupal_container()->register(LANGUAGE_TYPE_INTERFACE, 'Drupal\\Core\\Language\\Language');

because the Language class in core already has the same default values for its public attribtues.

As a conclusion, I think that:
- "$GLOBALS[LANGUAGE_TYPE_INTERFACE] = language_default()" should just be removed (what the patch is actually already doing)
- "$default = language_default();" should be removed because as you said, it is not used anywhere else.
- "drupal_container()->register(LANGUAGE_TYPE_INTERFACE, 'Drupal\\Core\\Language\\Language')
->addMethodCall('extend', array($default));" can be replaced by "drupal_container()->register(LANGUAGE_TYPE_INTERFACE, 'Drupal\\Core\\Language\\Language');" because the class has already the default values defined as in language_default() (what that patch is also already doing).
Maybe language_default() could even be removed from the core, since the same values can already be found in the Language class, but that can be a subject for another issue.

vasi1186’s picture

As discussed, I added the patch that just removes the "$GLOBALS[LANGUAGE_TYPE_INTERFACE] = language_default();" and does not change the "drupal_container()->register()" call anymore.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

We discussed in person with @vasi1186 that although it looked it just initializes the language to English, in fact it initializes it to the site default language which is a setting and would therefore not always be English. I think this looks good to go now.

RobLoach’s picture

Nicely done! I grepped through and couldn't find any other instances of the $language_interface global. +1

catch’s picture

#20: drupal-replace_global_language-1510686-20.patch queued for re-testing.

Don't think this applies anymore, sending for retest.

Also

-    global $language_interface;
+    $language_interface = drupal_container()->get(LANGUAGE_TYPE_INTERFACE);

This is very much more verbose, I opened #1630050: Replace language() with Drupal::language(). to discuss a bit.

Status: Reviewed & tested by the community » Needs work
Issue tags: +D8MI, +symfony, +sprint, +language-base, +Dependency Injection (DI)

The last submitted patch, drupal-replace_global_language-1510686-20.patch, failed testing.

webflo’s picture

Assigned: vasi1186 » webflo
Status: Needs work » Needs review
FileSize
26.56 KB

Rerolled.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks.

webflo’s picture

cosmicdreams’s picture

I can confirm RTBC once this passes.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Done, moving off sprint.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.