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_url global 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_url global.
Remaining tasks
#1497230: Use Dependency Injection to handle object definitions
User interface changes
None.
API changes
// Before
global $language_url;
// After
$language_url = drupal_container()->get(LANGUAGE_TYPE_URL);
Comment | File | Size | Author |
---|---|---|---|
#31 | drupal-fix-lang-injection-1512310-31.patch | 965 bytes | vasi1186 |
#25 | drupal-fix-lang-injection-1512310-25.patch | 613 bytes | disasm |
#24 | drupal-fix-lang-injection-1512310-24.patch | 563 bytes | disasm |
#13 | 1512310.patch | 6.73 KB | RobLoach |
#7 | 1512310_language_url.patch | 918 bytes | cosmicdreams |
Comments
Comment #1
plachComment #2
cosmicdreams CreditAttribution: cosmicdreams commentedNow that the dependency injection patch is in we can actively work on this.
Comment #3
sunComment #4
cosmicdreams CreditAttribution: cosmicdreams commentedI couldn't find any remaining use of global $language_url and the few places where the variable is used the value is already being pulled off of the DI.
Here's a patch that removes a bit of code that was used as a temporary helper for translating $_GLOBALS and some documentation
Comment #5
cosmicdreams CreditAttribution: cosmicdreams commentedComment #7
cosmicdreams CreditAttribution: cosmicdreams commentedThe patch failed because I removed the bandaid we have in place until all language specific globals are gone. Since this fixes part of the problem I'm keeping that in for now.
So this patch is just a documentation change.
Comment #8
cosmicdreams CreditAttribution: cosmicdreams commentedComment #9
sunThat's... interesting. $language_url is not really correct (points nowhere).
How do we refer to variables/services in the DI container?
Should we refer to "the language.url service"...?
Comment #10
sunfwiw, looping in coding standards people.
Comment #11
jhodgdonWhat is this "language.url service" that we want to point to? Reading the issue summary above, maybe that doc should just say
If $options['language'] is omitted, drupal_container()->get('language.url') will be used.
??
Comment #12
cosmicdreams CreditAttribution: cosmicdreams commentedthe drupal_container()->get() is using this syntax instead:
which is a constant.
Comment #13
RobLoachLet's get rid of the LANGUAGE_TYPE_URL constant while we're at it.
Comment #15
jhodgdonHm. I wonder if "Drupal installation failed" should be put on #1549534: Random failures (the Random test Failures issue), or if it is due to this patch? The error reported in http://qa.drupal.org/pifr/test/261468 is
Installing: failed to arrive at database configuration page using install path http://drupaltestbot659-mysql/checkout/install.php?profile=standard&langcode=en
Comment #16
plachWhy, instead of removing the constant which has a reason to exist, don't we move it to the language object?
Comment #17
cosmicdreams CreditAttribution: cosmicdreams commentedsimilar to platch's proposal is the dot notation found in other parts of symfony (for example event registration). We could register class namespaces into the DI like this:
Comment #18
RobLoachI was hesitant about switching to the "." notation for the service name as the Language system does weird things like:
Comment #19
cosmicdreams CreditAttribution: cosmicdreams commented@Rob Loach how about platch's proposal to move the constants into Language?
Comment #20
plachYes, #18 and similar pieces of code are the main reason I'd be concerned about removing the constant.
Edit: I'm not specifically concerned about mixing the dot and underscore notations, since variables are likely to move to the dot notation anyway in CMI, but those values are used in many places. I think it's safer to rely on constants instead of having to rewrite the string value everytime, which might result in hard-to-spot mistypings and all of this fun stuff.
For instance I regularly write "langauge_url", it's easy to predict what my "lanugage.url" strings would look like :)
Comment #21
sunTechnically, there is no problem with such variable names in the variable system. I once actually played with some code in a contrib module that intentionally used a dot to namespace the module's variable names, in order to simplify hook_uninstall() without running into false-positives. The variable system ate those variable names just fine.
Even more so, the names are legit and perfectly in line with the new configuration system that's using dots in configuration object names as well as keys anyway.
Since Symfony uses a dot notation for services, it would make sense to do the same.
Comment #22
plachI have no objection on moving the internal string value to the dot notation, but I don't see the benefit to remove the constant at all. Look at:
http://api.drupal.org/api/drupal/core!includes!language.inc/function/language_fallback_get_candidates/8
Why having to type the string would be better?
Comment #23
RobLoachI liked your idea of moving it to the Language class itself.
Comment #24
disasm CreditAttribution: disasm commentedAttached patch fixes the issue.
Comment #25
disasm CreditAttribution: disasm commentedAttached patch is same as above with removed global line.
Comment #26
ZenDoodles CreditAttribution: ZenDoodles commentedComment #27
plachThis is about converting the URL language types not content.
Comment #28
disasm CreditAttribution: disasm commentedFoobar on my part. I got issues crossed. Anyways, after a quick grep of core code, it appears all the global variables referencing language_url have been replaced by :
So, is this issue now about replacing that with:
Or is it about removing the LANGUAGE_TYPE_URL constant from the code base.
If it's the latter, What should be replaced by LANGUAGE_TYPE_URL constant in these cases below:
My apologies for the newbie questions. My first day submitting patches to core. Helped with the windsprint earlier today.
Comment #29
RobLoachLANGUAGE_TYPE_URL is actually
language_url
;-) . If we are moving/changing the constant itself, that will be discussed over at #1620010: Move LANGUAGE constants to the Language class. I agree we should definitely keep the scope down in these issues.Comment #30
vasi1186 CreditAttribution: vasi1186 commentedComment #31
vasi1186 CreditAttribution: vasi1186 commentedAttached a patch the removes the reference to the global $language_url from docs.
Comment #32
Gábor HojtsyWe both looked and have not found any other mention of a global language_url, so looks good for the scope of this issue. The replacement doc is the new behavior explained in terms of a short code snippet like before, so IMHO good to go :)
Comment #33
jhodgdonThis documentation change is not consistent with the issue summary. Which is correct?
Comment #34
Gábor Hojtsyhttp://drupal.org/node/1512310#comment-6096732 (comment #29 above) corrected the direction. I've also updated the issue summary now. The patch should be good.
Comment #35
jhodgdonThanks for clarifying! Committed to 8.x. This doesn't look like a 7.x-portable patch.
Comment #36
Gábor HojtsyDone, moving off sprint.
Comment #37.0
(not verified) CreditAttribution: commentedupdate for current status