Problem/Motivation
Globals are bad to use as it means there's no scope and restricts us from being able to instantial two Drupal instances at the same time. The $language_content global and LANGUAGE_TYPE_CONTENT constant is referenced in many places and we should clean that up.
Proposed resolution
Following #1497230: Use Dependency Injection to handle object definitions , this issue is to remove the $language_content global, and LANGUAGE_TYPE_CONTENT with a "language.content" service.
Remaining tasks
- #1497230: Use Dependency Injection to handle object definitions
- #1515196: Standardize retrieval of "global" variables to be at beginning of function
- #1512310: Replace $language_url with Dependency Injection
- #1510686: Replace remaining references to global $language_interface with drupal_container()
- #1620010: Move LANGUAGE constants to the Language class
- #1550866: Remove obsolete drupal_language_initialize()
User interface changes
None.
API changes
// Before
global $language_content;
// After
$language_content = drupal_container()->get(LANGUAGE_TYPE_CONTENT);
Comment | File | Size | Author |
---|---|---|---|
#43 | 1512308_0.patch | 2.93 KB | RobLoach |
#40 | drupal-fix-lang-injection-1512310-0.patch | 563 bytes | disasm |
#32 | 1512308.patch | 2.93 KB | RobLoach |
#29 | langague_type_content_06.patch | 26.52 KB | August1914 |
#29 | language_type_content_03_06_interdiff.txt | 963 bytes | August1914 |
Comments
Comment #1
RobLoach#1497230: Use Dependency Injection to handle object definitions
Comment #2
plachComment #3
cosmicdreams CreditAttribution: cosmicdreams commentedNow that the dependency injection patch is in we can actively work on this.
Comment #4
sunComment #5
cosmicdreams CreditAttribution: cosmicdreams commentedIn this patch I was able to find a few remaining uses of language_content as a global variable. This patch finds and fixes them using a variant of what is described above.
Comment #6
cosmicdreams CreditAttribution: cosmicdreams commentedComment #7
sunDid you check how often $language_content is used in those functions? Ideally, if possible, we should move the DI code to the place where the variable is used -- if there is only one usage throughout the function. (if there are multiple uses, keep it at the top)
Comment #8
RobLoachLet's get rid of the constant while we're at it. The constant itself might not be available if the code is called early enough.
Fixed.
Comment #10
Tor Arne Thune CreditAttribution: Tor Arne Thune commented#8: 1512308.patch queued for re-testing.
Comment #11
cosmicdreams CreditAttribution: cosmicdreams commentedClean and excellent thanks Rob, recommend RTBC.
Comment #12
plachWhy, instead of removing the constant which has a reason to exist, don't we move it to the language object?
Comment #13
cosmicdreams CreditAttribution: cosmicdreams commentedOn second thought, I recommend the patch in #5. The conversion of the use of constants to some other system needs more discussion and can be followed up later. That issue shouldn't hold up this patch.
There's a lot of discussion on how to handle the loading of this default config for Language and we're trying to put together a better proposal here: #1550866: Remove obsolete drupal_language_initialize(). Further work to improve the handling of the language system should go there.
This issue is simply for the intermediate step of using the di for things we used to use globals for. Reviewers please review patch #5.
Comment #14
RobLoachI like the idea of moving the constant into the Language class. The less stuff in bootstrap.inc, the better.
Comment #15
cosmicdreams CreditAttribution: cosmicdreams commented#14: language_type_content.patch queued for re-testing.
Comment #17
August1914 CreditAttribution: August1914 commentedComment #18
August1914 CreditAttribution: August1914 commentedResolved conflicts with 8.x, this patch applies cleanly at 467a825, patch from comment 14 applies cleanly at 252e84ec590e15132cf2bdb635b28d1a229ba167
Comment #19
August1914 CreditAttribution: August1914 commentedComment #20
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #22
August1914 CreditAttribution: August1914 commentedComment #23
August1914 CreditAttribution: August1914 commentedRerolled from #14, this patch applies cleanly at 5b52cea
Comment #25
August1914 CreditAttribution: August1914 commentedChanged references to (7.x) DrupalWebTestCase to (8.x) WebTestCase.
Reroll from #14, with changes in attached interdiff.
This patch applies cleanly at 5b52cea.
Comment #27
August1914 CreditAttribution: August1914 commentedAdded simpletest includes.
Changed references to (7.x) DrupalWebTestCase to (8.x) WebTestCase.
Reroll from #14, with changes in attached interdiff.
This patch applies cleanly at 5b52cea.
Comment #29
August1914 CreditAttribution: August1914 commentedpatch 05 incorrectly stated "WebTestCase" instead of "WebTestBase". Same as above, rerolling from #14
Comment #31
August1914 CreditAttribution: August1914 commentedOK, the reroll is complete with langague_type_content_06.patch; the patch applies and the testcases execute. The failures now are in various of the testcases.
Comment #32
RobLoachLet's scale back the scope of this issue a bit. This is to just replace $language_content with calling drupal_container().
Moving the retrieval of it around is over at #1515196: Standardize retrieval of "global" variables to be at beginning of function.
Moving the constants around is over at #1620010: Move LANGUAGE constants to the Language class.
Comment #33
Gábor HojtsyLooks simple and right.
Comment #34
plachI ain't sure this is actually needed. I don't think content language needs to be available before language initialization.
Comment #35
RobLoachIt's okay if it's there, calling
$container->register()
doesn't actually instantiate the object. It's just the initial definition. In a follow up, we'd hopefully replace all three language objects with one language system object, allowing us to replace the language bootstrap entirely.Comment #36
plachOk, let's go then :)
Comment #37
cosmicdreams CreditAttribution: cosmicdreams commentedYes the redundancy leaves a terrible code smell. Rob can you create a follow up issue to implement the LanguageSystem and reference it here (if you haven't done that already).
Comment #37.0
cosmicdreams CreditAttribution: cosmicdreams commentedUpdated issue summary.
Comment #37.1
plachUpdated the API changes section to use the constant as in the patch
Comment #38
RobLoachI believe that #1550866: Remove obsolete drupal_language_initialize() is it. I'll add it to the issue summary.
Comment #38.0
RobLoachUpdated the remaining tasks section.
Comment #39
cosmicdreams CreditAttribution: cosmicdreams commentedRTBC++
Comment #40
disasm CreditAttribution: disasm commentedAttached patch fixes the issue
Comment #41
plachPatch to be committed in #32.
Comment #42
disasm CreditAttribution: disasm commentedOops, posted to wrong issue queue. Ignore this patch.
Comment #43
RobLoachI've done the same thing so many times :-) . Same patch from #32!
Comment #44
catchLooks fine. Committed/pushed to 8.x.
Comment #45
Gábor HojtsyDone, moving off sprint.
Comment #46.0
(not verified) CreditAttribution: commentedUpdated issue summary.