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

<?php
// Before
global $language_url;
// After
$language_url = drupal_container()->get(LANGUAGE_TYPE_URL);
?>
Files: 
CommentFileSizeAuthor
#31 drupal-fix-lang-injection-1512310-31.patch965 bytesvasi1186
PASSED: [[SimpleTest]]: [MySQL] 36,810 pass(es).
[ View ]
#25 drupal-fix-lang-injection-1512310-25.patch613 bytesdisasm
PASSED: [[SimpleTest]]: [MySQL] 36,819 pass(es).
[ View ]
#24 drupal-fix-lang-injection-1512310-24.patch563 bytesdisasm
PASSED: [[SimpleTest]]: [MySQL] 36,819 pass(es).
[ View ]
#13 1512310.patch6.73 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#7 1512310_language_url.patch918 bytescosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 35,413 pass(es).
[ View ]
#4 1510686_6_di_language_url.patch1.56 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 34,666 pass(es), 113 fail(s), and 244,518 exception(s).
[ View ]

Comments

Issue tags:+D8MI, +language-base

Status:Postponed» Active

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

StatusFileSize
new1.56 KB
FAILED: [[SimpleTest]]: [MySQL] 34,666 pass(es), 113 fail(s), and 244,518 exception(s).
[ View ]

I 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

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, 1510686_6_di_language_url.patch, failed testing.

StatusFileSize
new918 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,413 pass(es).
[ View ]

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

Status:Needs work» Needs review

+++ b/core/includes/common.inc
@@ -2128,8 +2128,8 @@ function _format_date_callback(array $matches = NULL, $new_langcode = NULL) {
- *     for the URL. If $options['language'] is omitted, the global $language_url
- *     will be used.
+ *     for the URL. If $options['language'] is omitted, the $language_url will
+ *     be used.

That'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"...?

Issue tags:+coding standards

fwiw, looping in coding standards people.

Status:Needs review» Needs work

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

??

the drupal_container()->get() is using this syntax instead:

drupal_container()->get(LANGUAGE_TYPE_URL)

which is a constant.

Status:Needs work» Needs review
StatusFileSize
new6.73 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Let's get rid of the LANGUAGE_TYPE_URL constant while we're at it.

Status:Needs review» Needs work

The last submitted patch, 1512310.patch, failed testing.

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

Why, instead of removing the constant which has a reason to exist, don't we move it to the language object?

<?php
drupal_container
()->get(Language::URL);
?>

similar 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:

drupal_container()->get('language.type.url');
drupal_container()->get('language.type.content');
drupal_container()->get('language.type.interface');

I was hesitant about switching to the "." notation for the service name as the Language system does weird things like:

<?php
  variable_set
('language_negotiation_' . LANGUAGE_TYPE_URL, language_language_negotiation_info());
?>

@Rob Loach how about platch's proposal to move the constants into Language?

Yes, #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 :)

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

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

I liked your idea of moving it to the Language class itself.

StatusFileSize
new563 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,819 pass(es).
[ View ]

Attached patch fixes the issue.

StatusFileSize
new613 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,819 pass(es).
[ View ]

Attached patch is same as above with removed global line.

Status:Needs work» Needs review

Status:Needs review» Needs work

This is about converting the URL language types not content.

Foobar 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 :

<?php
drupal_container
()->get(LANGUAGE_TYPE_URL);
?>

So, is this issue now about replacing that with:

<?php
drupal_container
()->get('language.url');
?>

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:

core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php:82:    variable_set('language_negotiation_' . LANGUAGE_TYPE_URL, language_language_negotiation_info());
core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php:113:    $language_negotiation = language_negotiation_method_get_first(LANGUAGE_TYPE_URL) == LANGUAGE_NEGOTIATION_DEFAULT;
core/modules/locale/locale.install:460:    LANGUAGE_TYPE_URL => FALSE,
core/modules/language/language.module:284:    LANGUAGE_TYPE_URL => array(
core/modules/language/language.module:299:    'types' => array(LANGUAGE_TYPE_CONTENT, LANGUAGE_TYPE_INTERFACE, LANGUAGE_TYPE_URL),
core/modules/language/language.module:352:    'types' => array(LANGUAGE_TYPE_URL),

My apologies for the newbie questions. My first day submitting patches to core. Helped with the windsprint earlier today.

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

Assigned:Unassigned» vasi1186
Issue tags:+sprint

Status:Needs work» Needs review
StatusFileSize
new965 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,810 pass(es).
[ View ]

Attached a patch the removes the reference to the global $language_url from docs.

Component:language system» documentation
Assigned:vasi1186» jhodgdon
Status:Needs review» Reviewed & tested by the community

We 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 :)

Status:Reviewed & tested by the community» Needs review

This documentation change is not consistent with the issue summary. Which is correct?

Status:Needs review» Reviewed & tested by the community

http://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.

Status:Reviewed & tested by the community» Fixed

Thanks for clarifying! Committed to 8.x. This doesn't look like a 7.x-portable patch.

Issue tags:-sprint

Done, moving off sprint.

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

Issue summary:View changes

update for current status