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 "." .

Files: 
CommentFileSizeAuthor
#27 drupal-replace_global_language-1510686-22.patch27.15 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 36,856 pass(es).
[ View ]
#27 drupal-replace_global_language-1510686-22.interdiff.txt540 byteswebflo
#25 drupal-replace_global_language-1510686-21.patch26.56 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 36,860 pass(es).
[ View ]
#20 drupal-replace_global_language-1510686-20.patch24.24 KBvasi1186
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-replace_global_language-1510686-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 drupal-replace_global_language-1510686-16.patch24.65 KBvasi1186
PASSED: [[SimpleTest]]: [MySQL] 36,824 pass(es).
[ View ]
#14 drupal-replace_global_language-1510686-14.patch25.36 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 36,808 pass(es).
[ View ]
#14 interdiff.txt5.46 KBdisasm
#9 1510686_9_language_interface.patch29.98 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1510686_9_language_interface.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 1510686_6_di_language_interface.patch30.64 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 35,219 pass(es), 14 fail(s), and 16,699 exception(s).
[ View ]

Comments

Status:Active» Postponed

Title:Replace global $language_interface with Dependency InjectionReplace remaining references to global $language_interface with drupal_container()

Issue tags:+language-base

Status:Postponed» Active

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

StatusFileSize
new30.64 KB
FAILED: [[SimpleTest]]: [MySQL] 35,219 pass(es), 14 fail(s), and 16,699 exception(s).
[ View ]

Think I got them all

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.

StatusFileSize
new29.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1510686_9_language_interface.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new5.46 KB
new25.36 KB
PASSED: [[SimpleTest]]: [MySQL] 36,808 pass(es).
[ View ]

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

Assigned:Unassigned» vasi1186
Issue tags:+sprint

StatusFileSize
new24.65 KB
PASSED: [[SimpleTest]]: [MySQL] 36,824 pass(es).
[ View ]

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...

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)?

Assigned:Unassigned» vasi1186
Issue tags:+sprint

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.

StatusFileSize
new24.24 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-replace_global_language-1510686-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

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

#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.

Assigned:vasi1186» webflo
Status:Needs work» Needs review
StatusFileSize
new26.56 KB
PASSED: [[SimpleTest]]: [MySQL] 36,860 pass(es).
[ View ]

Rerolled.

Status:Needs review» Reviewed & tested by the community

Great, thanks.

StatusFileSize
new540 bytes
new27.15 KB
PASSED: [[SimpleTest]]: [MySQL] 36,856 pass(es).
[ View ]

Removed the todo in bootstrap.inc

I can confirm RTBC once this passes.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks!

Issue tags:-sprint

Done, moving off sprint.

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

Issue summary:View changes

Updated issue summary.