Remember the provider that selected the language for later use

Thanks! Retitling to make it more goal-oriented. Will ask @plach to take a quick look.

The patch looks good to me. IIRC I wrote some code like that in a patch of mine (don't remember exactly which one). I think it could be easily backported too.

We need tests, however.

Also not sure we want it on language_default(), at least in the form it was added, it will not be there when the default language is changed on the UI.

@Gabor:

Yeah, good point. The provider key should be populated only on the language global objects.

This looks better :)

+++ b/includes/language.inc
@@ -384,12 +384,15 @@ function language_initialize($type) {
+  $language->provider = 'default';

The default language provider id is stored in the LANGUAGE_NEGOTIATION_DEFAULT constant.

PS: Set the "needs reviews" status when you post a patch so the testbot will have a look to it and run automated tests.

-17 days to next Drupal core point release.

Looks good, thanks.

Sorry, I forgot we need some test coverage for this: a few lines in LocaleUILanguageNegotiationTest::runTest() should be enough.

Agreed.

Great stuff :) Just two small notes.

+++ b/modules/locale/locale.testundefined
@@ -2162,6 +2169,7 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+    $this->assertText($test['expected_negotiation'], "Negotiation (" . $test['expected_negotiation'] . ") provider is found.");

This might match things that are unrelated due to how short and generic the negotiation names are. I'd suggest you check for the same t()-ed string with the placeholder here. If you keep using % as placeholder, you'll need to use assertRaw() here for exact matches. See also bellow.

And one more thing, you can skip the second argument in the assert (the message), since it will be properly autogenerated especially if you use the more verbose text in t() to assert (and use it with @name and keep assertText()).

+++ b/modules/locale/tests/locale_test.moduleundefined
@@ -22,6 +22,9 @@ function locale_test_boot() {
+    drupal_set_message(t('Language negotiation provider: %name', array('%name' => $GLOBALS['language']->provider)));

You can use @name instead of %name to avoid the need to use assertRaw() above. Either works.

Keeping on needs review to see testbot feedback, then please improve the tests :)

Crosspost.

+++ b/modules/locale/locale.test
@@ -2069,6 +2069,7 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+        'expected_negotiation' => LOCALE_LANGUAGE_NEGOTIATION_URL_FALLBACK,

I'd use 'expected_provider' here and below.

+++ b/modules/locale/tests/locale_test.module
@@ -22,6 +22,9 @@ function locale_test_boot() {
+  if (isset($GLOBALS['language']) && isset($GLOBALS['language']->provider)) {
+    drupal_set_message(t('Language negotiation provider: %name', array('%name' => $GLOBALS['language']->provider)));
+  }

If this is here only for debugging purposes, perhaps debug() could be a better choice.

-26 days to next Drupal core point release.

@plach: its in the locale_test.module, so does it matter if it is debug() or something else? It is used to assert() it in the test :)

Still not assert()-ing for the whole t() string, which makes it hard to believe the test surely finds the right string it targets and not some other text on the page.

I think this looks good now.

I tested #26 and discovered a problem with it: since the provider results are cached, the same object reference is returned for each language_provider_invoke() call. As a consequence the provider of the last initialized language global is assigned to all the language globals. I added a line to clone the object returned by language_provider_invoke(), now everything seems to work as intended. Probably we need better test coverage since the actual behavior was totally broken but tests did not spot it.

The bug can be consistently reproduced with #26 applied by enabling at least two languages and assigning a language prefix to each one. After visiting a random page with no url prefix, say http://example.org/, the correct status should be:

<?php
$GLOBALS[LANGUAGE_TYPE_INTERFACE]->provider == LANGUAGE_NEGOTIATION_DEFAULT
$GLOBALS[LANGUAGE_TYPE_CONTENT]->provider == LOCALE_LANGUAGE_NEGOTIATION_INTERFACE
$GLOBALS[LANGUAGE_TYPE_URL]->provider == LOCALE_LANGUAGE_NEGOTIATION_URL_FALLBACK
?>

The actual status is:

<?php
$GLOBALS[LANGUAGE_TYPE_INTERFACE]->provider == LOCALE_LANGUAGE_NEGOTIATION_URL_FALLBACK
$GLOBALS[LANGUAGE_TYPE_CONTENT]->provider == LOCALE_LANGUAGE_NEGOTIATION_URL_FALLBACK
$GLOBALS[LANGUAGE_TYPE_URL]->provider == LOCALE_LANGUAGE_NEGOTIATION_URL_FALLBACK
?>

The case above can be coded as an additional test method for LocaleUILanguageNegotiationTest to provide full test coverage for this.

Unfortunately, the tests in #29 do not cover the issue described in #28: I tried to comment out the line returning a cloned language object but the tests still pass, where they should be failing. The reason is that in some cases the expected provider is wrong thus matching the wrong behavior. AAMOF the expected providers should match the ones described in the 'message' keys:

+++ b/core/modules/locale/locale.test
@@ -2069,6 +2069,7 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+        'expected_provider' => LOCALE_LANGUAGE_NEGOTIATION_URL_FALLBACK,

This should be LANGUAGE_NEGOTIATION_DEFAULT.

+++ b/core/modules/locale/locale.test
@@ -2085,6 +2087,7 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+        'expected_provider' => LOCALE_LANGUAGE_NEGOTIATION_INTERFACE,

This should be LOCALE_LANGUAGE_NEGOTIATION_BROWSER.

+++ b/core/modules/locale/locale.test
@@ -2101,6 +2105,7 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+        'expected_provider' => LOCALE_LANGUAGE_NEGOTIATION_URL_FALLBACK,

This should be LANGUAGE_NEGOTIATION_DEFAULT.

+++ b/core/modules/locale/locale.test
@@ -2128,6 +2133,7 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+        'expected_provider' => LOCALE_LANGUAGE_NEGOTIATION_URL_FALLBACK,

This should be LANGUAGE_NEGOTIATION_DEFAULT.

If you fix the patch with the corrections above and revert the line cloning the language objects I added in #28, you should get failing tests, while keeping that line in place should give you passing tests.

28 days to next Drupal core point release.

@Pol:

We should be very close to RTBC, any chance to get the suggestions in #30 implemented?

Great, please post two patches: one complete and one with the line cloning the object commented so we can ensure that tests pass/fail as expected.

@Pol: are you still working on this patch?

I think the advice from @plach is great. Looking forward to your patch! Thanks!

Here is the patch that should fail in a format that applies. The first hunk was not rolled properly.

Great, RTBC patch in #37.

hm

Agreed, the RTBC patch is the passing one in #37, the other one in #38 demonstrates how it would fail without cloning the object (with the same code).

+++ b/core/includes/language.incundefined
@@ -350,7 +350,9 @@ function language_provider_invoke($provider_id, $provider = NULL) {
-  return $results[$provider_id];
+  // Since objects are passed by reference we need to return a clone to prevent
+  // the providers cache to be unintentionally altered.
+  return is_object($results[$provider_id]) ? clone($results[$provider_id]) : $results[$provider_id];

It's always a fishy when the return type can be multiple different things. Is that something we could fix?

In general, I'm OK with this patch, but I don't really understand the use case.

@Dries: as you can see on the lines in the patch above, if its not an object, that means no language was identified by the provider, which is information used for fallback. Like if the URL did not contain any language, it would return FALSE and the next provider would look for language. I think encapsulating FALSE in an object that looks like a language is a bad idea. Also, this is not an error condition, it is merely a list with fallbacks, so using exceptions would not fit well either. Its not an exception if the URL did not have language information, it is perfectly valid and should result in falling back on the next provider configured, that is why we designed a list of providers in priority order to figure out the language.

Moving back to RTBC with that.

Perhaps using !empty instead of is_object would make it look better?

!empty would be clearer yeah. Should be a quick re-roll and makes it clearer that we're only dealing with an object or false. Although also, why return FALSE here instead of just no return? Wouldn't that mean the same thing? Then there'd be no need for the ternary (only return if something is set, otherwise return nothing).

Stuffing this information in the language object feels a bit wrong to me, but I definitely can't think of another backportable solution, nor a simple one for Drupal 8.

Presumably the intention will be to convert the language globals to a wscci handler at some point, when that happens this might start to look a bit different anyway.

Here is a quick reroll of the same patch with !empty(). I agree WSCII is going to modify the whole language init flow anyway, so this code is revisited in detail in #1260918: Convert language globals to contexts, there is not much point in obsessing with existing details of the API. Also, its how the API is defined now, and this issue is about adding information on the provider used, not to change how provider return values are handled, so let's keep the scope.

OK I'm happy with this personally, leaving for Dries to see if that improves things for him as well.

Sorry, not quite.

+  // Since objects are passed by reference we need to return a clone to prevent
+  // the providers cache to be unintentionally altered.

Objects are resources in PHP 5, not really passed by reference. So "Since objects are resources"?

Also should be "the providers cache from being unintentionally altered". It might also be worth mentioning that the reason this has to be done is because the same language object can be assigned to more than one language global?

You are right. Comment fixed as suggested. BTW we do have an issue at #1156576: Language negotiation is undocumented to include phpdoc documentation of the negotiation system in the codease. I've tried to recruit people to that earlier with little success. I'll try again with a set of other people :)

Back to Dries' queue.

Committed to 8.x. Moving to 7.x for @webchick.

Thanks! The committed patch should apply cleanly to D7 with -p2.

Do we need a change record?

Nope, the patch needs a reroll.

Rolled a patch for D7

Ops! Coding stadards whitespaces!

Tagging for base language system.

Thanks!

Tagging for language negotiation too.

We are finally below thresholds, so features are back on the table. Re-testing the latest patch, since it's been awhile.

#58: language-provider-1314384-58.patch queued for re-testing.

Committed and pushed to 7.x. Thanks!