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);
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Issue tags: +D8MI, +language-base
cosmicdreams’s picture

Status: Postponed » Active

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

sun’s picture

cosmicdreams’s picture

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

cosmicdreams’s picture

Status: Active » Needs review

Status: Needs review » Needs work

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

cosmicdreams’s picture

FileSize
918 bytes

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.

cosmicdreams’s picture

Status: Needs work » Needs review
sun’s picture

+++ 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"...?

sun’s picture

Issue tags: +Coding standards

fwiw, looping in coding standards people.

jhodgdon’s picture

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.

??

cosmicdreams’s picture

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

drupal_container()->get(LANGUAGE_TYPE_URL) 

which is a constant.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
6.73 KB

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.

jhodgdon’s picture

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

plach’s picture

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);
?>
cosmicdreams’s picture

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');
RobLoach’s picture

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

  variable_set('language_negotiation_' . LANGUAGE_TYPE_URL, language_language_negotiation_info());
cosmicdreams’s picture

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

plach’s picture

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

sun’s picture

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.

plach’s picture

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?

RobLoach’s picture

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

disasm’s picture

Attached patch fixes the issue.

disasm’s picture

Attached patch is same as above with removed global line.

ZenDoodles’s picture

Status: Needs work » Needs review
plach’s picture

Status: Needs review » Needs work

This is about converting the URL language types not content.

disasm’s picture

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 :

drupal_container()->get(LANGUAGE_TYPE_URL);

So, is this issue now about replacing that with:

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.

RobLoach’s picture

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.

vasi1186’s picture

Assigned: Unassigned » vasi1186
Issue tags: +sprint
vasi1186’s picture

Status: Needs work » Needs review
FileSize
965 bytes

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

Gábor Hojtsy’s picture

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

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

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

Gábor Hojtsy’s picture

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.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

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

Gábor Hojtsy’s picture

Issue tags: -sprint

Done, moving off sprint.

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

Anonymous’s picture

Issue summary: View changes

update for current status