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.
Comment | File | Size | Author |
---|---|---|---|
#16 | 1539614_16_language_di.patch | 2.66 KB | vasi1186 |
#8 | 1539614_8_language_di.patch | 1.85 KB | cosmicdreams |
#2 | 1539614_2_language_inc_di.patch | 2.68 KB | cosmicdreams |
Comments
Comment #1
sunComment #2
cosmicdreams CreditAttribution: cosmicdreams commentedfound and fixed these issues. please review
Comment #3
cosmicdreams CreditAttribution: cosmicdreams commentedComment #4
RobLoachComment #5
Dries CreditAttribution: Dries commentedI think this may need to be re-rolled. Asking for a re-test.
Comment #6
Dries CreditAttribution: Dries commented#2: 1539614_2_language_inc_di.patch queued for re-testing.
Comment #8
cosmicdreams CreditAttribution: cosmicdreams commentedrerolled
Comment #9
cosmicdreams CreditAttribution: cosmicdreams commentedComment #10
cosmicdreams CreditAttribution: cosmicdreams commented#8: 1539614_8_language_di.patch queued for re-testing.
Comment #11
cosmicdreams CreditAttribution: cosmicdreams commentedI'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?
Comment #12
cosmicdreams CreditAttribution: cosmicdreams commentedAssigning to me so I can show these to folks easier
Comment #13
Gábor HojtsyYou should use $type, and not hardcode a specific type.
Why are you removing the test?
Comment #14
vasi1186 CreditAttribution: vasi1186 commentedComment #15
vasi1186 CreditAttribution: vasi1186 commentedI 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.
Comment #16
vasi1186 CreditAttribution: vasi1186 commentedfor some reason, the patch was not attached in the previous comment...
Comment #17
vasi1186 CreditAttribution: vasi1186 commentedAdd again the missing tags.
Comment #18
RobLoachActually, 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.
Comment #19
RobLoachDidn't mean to remove tags.
Comment #20
Gábor HojtsyRob: 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.
Comment #21
cosmicdreams CreditAttribution: cosmicdreams commented#15:
Yep, that was the sum total of my reasoning. Reviewed #16 and it looks solid. I vote RTBC.
Comment #22
Gábor Hojtsy#16: 1539614_16_language_di.patch queued for re-testing.
Comment #23
webchickthis 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! :)
Comment #24
Gábor HojtsyDone, moving off sprint.