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 "." .
Comments
Comment #1
RobLoachPostponed till #1497230: Use Dependency Injection to handle object definitions .
Comment #2
RobLoachComment #3
plachComment #4
cosmicdreams CreditAttribution: cosmicdreams commentedNow that the dependency injection patch is in we can actively work on this.
Comment #5
sunComment #6
cosmicdreams CreditAttribution: cosmicdreams commentedThink I got them all
Comment #7
cosmicdreams CreditAttribution: cosmicdreams commentedI'd like to know how this tests.
Comment #9
cosmicdreams CreditAttribution: cosmicdreams commentedSame patch but I added back the code that kept $GLOBALS working properly for language_content and language_url
Comment #10
cosmicdreams CreditAttribution: cosmicdreams commentedtry again
Comment #12
cosmicdreams CreditAttribution: cosmicdreams commented#9: 1510686_9_language_interface.patch queued for re-testing.
Comment #14
disasm CreditAttribution: disasm commentedAttached patch is an interdiff for the above patch that failed testing. I also fixed a few more instances that the above patch missed.
Comment #15
vasi1186 CreditAttribution: vasi1186 commentedComment #16
vasi1186 CreditAttribution: vasi1186 commentedThe 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
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...
Comment #17
Gábor HojtsyOn review, this looks good. I have two questions about the end though:
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?
$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)?
Comment #18
vasi1186 CreditAttribution: vasi1186 commentedComment #19
vasi1186 CreditAttribution: vasi1186 commentedSo, 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:
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
can be replaced by
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.
Comment #20
vasi1186 CreditAttribution: vasi1186 commentedAs 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.
Comment #21
Gábor HojtsyWe 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.
Comment #22
RobLoachNicely done! I grepped through and couldn't find any other instances of the
$language_interface
global. +1Comment #23
catch#20: drupal-replace_global_language-1510686-20.patch queued for re-testing.
Don't think this applies anymore, sending for retest.
Also
This is very much more verbose, I opened #1630050: Replace language() with Drupal::language(). to discuss a bit.
Comment #25
webflo CreditAttribution: webflo commentedRerolled.
Comment #26
Gábor HojtsyGreat, thanks.
Comment #27
webflo CreditAttribution: webflo commentedRemoved the todo in bootstrap.inc
Comment #28
cosmicdreams CreditAttribution: cosmicdreams commentedI can confirm RTBC once this passes.
Comment #29
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #30
Gábor HojtsyDone, moving off sprint.
Comment #31.0
(not verified) CreditAttribution: commentedUpdated issue summary.