Stale language types/negotation info after enabling/disabling modules

Yes, try deleting the 'language_types' variable. It's corrupted, but it's unclear why.

subscribing

Bugs are fixed in the development version first, backported then.

This is a major bug since after disabling a module altering language type information, the latter and language negotiation are not updated accordingly. This may lead to very tricky behaviors since a language type which was previously configurable might not be it anymore. A fixed language type is supposed to have a fixed (predictable) language negotiation configuration, while in this case it would retain the previous one which might be different.

Attatched there is a first (partial) attempt to fix the issue, posting it to see if the bot complains.

Related issue: #1060328: Removing a previously available language detection method breaks the configuration UI.

This should be taken into consideration also when enabling modules.

This one should pass. Working on some test coverage for this.

Here is an improved version with a complete test suite. Patch summary is coming tomorrow.

The test-only patch is supposed to fail to show how severe this bug is.

+++ b/modules/locale/locale.test
@@ -2373,3 +2377,158 @@ class LocaleDateFormatsFunctionalTest extends DrupalWebTestCase {
+    // Enable/disable locale_test only we did not alreay before.

Should be: "Enable/disable locale_test only if we did not already before."

Added missing tag.

Powered by Dreditor.

+++ b/includes/language.inc
@@ -84,6 +84,42 @@ function language_types_disable($types) {
+ * Update the language type configuration.

@@ -173,6 +209,30 @@ function language_negotiation_get_switch_links($type, $path) {
+ * Ensure that the enabled language providers are actually defined.

1) phpDoc summaries should be in the third-person form; i.e., "Updates...".

2) The _purge() summary should rather be along the lines of "Updates language configuration to removes any language providers that are no longer defined." -- or similar.

+++ b/includes/language.inc
@@ -84,6 +84,42 @@ function language_types_disable($types) {
+ * @param $defined_providers
+ *   (Optional) A list of defined language providers. Defaults to the
+ *   system-defined ones.

1) "... providers keyed by id." (not sure what $id actually is though).

2) (optional) should be all-lowercase.

+++ b/includes/language.inc
@@ -84,6 +84,42 @@ function language_types_disable($types) {
+function language_types_set($defined_providers = NULL) {

@@ -173,6 +209,30 @@ function language_negotiation_get_switch_links($type, $path) {
+function language_negotiation_purge($defined_providers = NULL) {

+++ b/modules/locale/locale.module
@@ -593,6 +595,22 @@ function locale_language_negotiation_info() {
+  language_types_set();
+  language_negotiation_purge();

It doesn't look like we'd ever pass something for $defined_providers. Therefore, the usefulness of this argument is unclear to me. If you have a contrib use-case in mind, then this use-case deserves to be mentioned in the phpDoc.

+++ b/includes/language.inc
@@ -84,6 +84,42 @@ function language_types_disable($types) {
+  if (empty($defined_providers)) {

Should be !isset()

+++ b/includes/language.inc
@@ -84,6 +84,42 @@ function language_types_disable($types) {
+    drupal_static_reset('language_negotiation_info');
...
+  drupal_static_reset('language_types_info');
...
+  drupal_static_reset('language_types_configurable');

Each manually required drupal_static_reset() deserves an inline comment that explains why it is required. It's close to impossible to figure out the reasoning in existing code.

Especially the last one needs at least that, but actually even more reasoning.

+++ b/includes/language.inc
@@ -84,6 +84,42 @@ function language_types_disable($types) {
+  foreach (language_types_info() as $type => $info) {
+    if (isset($info['fixed'])) {
+      $language_types[$type] = FALSE;
+      $negotiation = array();
+      foreach ($info['fixed'] as $weight => $id) {
+        if (isset($defined_providers[$id])) {
+          $negotiation[$id] = $defined_providers[$id];
+          $negotiation[$id]['weight'] = $weight;
+        }
+      }
+      language_negotiation_set($type, $negotiation);
+    }
+    else {
+      $language_types[$type] = TRUE;
+    }
+  }

@@ -173,6 +209,30 @@ function language_negotiation_get_switch_links($type, $path) {
+  foreach (language_types_info() as $type => $type_info) {
+    $negotiation = array();
+    foreach (variable_get("language_negotiation_$type", array()) as $id => $provider) {
+      if (isset($defined_providers[$id])) {
+        $negotiation[] = $id;
+      }
+    }
+    language_negotiation_set($type, array_flip($negotiation));
+  }

The internal guts of these functions could use some more explanations in inline comments. If I would have to, I'd commit this patch as is, but that would merely be, because I trust you. And because I really hope that you won't be hit by a bus anytime soon. ;) Anyone else has no idea what the assignment of FALSE or TRUE for language types means, why it happens, and why we only store language providers for "fixed" types, but not for not "fixed" (whatever fixed means).

It's also unclear why there is a language_negotiation_set(), but no language_negotiation_get(); it seems the _purge() simply uses a certain variable instead.

Speaking of, using unset() in _purge() would dramatically increase the readability of the code.

Lastly, I don't understand why _set() needs to reset a static at the end, and _purge() does not.

+++ b/modules/locale/locale.admin.inc
@@ -541,6 +541,11 @@ function _locale_languages_configure_form_language_table(&$form, $type) {
   foreach ($providers_weight as $id => $weight) {
+    // Exclude no more available providers.
+    if (!isset($language_providers[$id])) {
+      continue;
+    }

This comment could include a short pointer for "why on earth" providers can suddenly be "no more available".

+++ b/modules/locale/locale.admin.inc
@@ -680,27 +683,11 @@ function locale_languages_configure_form_submit($form, &$form_state) {
+  // Save non-configurable language types and the related language negotiation
+  // configuration.
+  language_types_set($form['#language_providers']);

Why is a form submit handler working off values in $form? $form_state['values'] contains the user-submitted data, and if that needs adjustment, stateful information like this belongs into $form_state.

Since I don't see any changes in this patch that set #language_providers, I can only guess that proper storage/compilation of stateful form values is something we need to fix for D8 (as it would be an API change for D7). If that is the case, we need to create an issue for this.

+++ b/modules/locale/locale.module
@@ -593,6 +595,22 @@ function locale_language_negotiation_info() {
+ * Implementation of hook_modules_enabled().
...
+ * Implementation of hook_modules_disabled().

"Implementation of" is outdated.

Powered by Dreditor.

Thanks! Here is one addressing #13:

It doesn't look like we'd ever pass something for $defined_providers. [...]

Why is a form submit handler working off values in $form?

In both cases the $defined_providers parameter was not actually needed: in the old code $form['#language_providers'] was used to avoid calling language_negotiation_info() again. Since it looked like over-engineering I just removed it.

It's also unclear why there is a language_negotiation_set(), but no language_negotiation_get(); it seems the _purge() simply uses a certain variable instead.

_get() is defined but, as _set(), it wraps some logic around the raw variable value.

@@ -173,6 +209,30 @@ function language_negotiation_get_switch_links($type, $path) {
+  foreach (language_types_info() as $type => $type_info) {
+    $negotiation = array();
+    foreach (variable_get("language_negotiation_$type", array()) as $id => $provider) {
+      if (isset($defined_providers[$id])) {
+        $negotiation[] = $id;
+      }
+    }
+    language_negotiation_set($type, array_flip($negotiation));
+  }

Speaking of, using unset() in _purge() would dramatically increase the readability of the code.

We cannot use unset() since the value stored in the variable is different form the data structure accepted as parameter. I tried to improve readability by removing the array_flip() call.

Lastly, I don't understand why _set() needs to reset a static at the end, and _purge() does not.

Because, if I am not mistaken, there is no static caching involved in functions getting the value of language_negotiation_$type variables, while language_types_configurable() is cached.

+ * Updates language configuration to removes any language providers that are no longer defined.

Should be "remove" I guess :(

+++ b/modules/locale/locale.admin.inc
@@ -541,6 +541,12 @@ function _locale_languages_configure_form_language_table(&$form, $type) {
+    // been disable after the last configuration saving.

"disabled"

Powered by Dreditor.

Fixed typos above and other minor polishing.

Edit: here is the language negotiation API documentation: we might want to move it into a doc block at the beginning of language.inc with some adjustments (in a separate issue).

Looks mostly good to me now, but another review from someone else would be good to have.

Reviewed the one before @sun's comments and this one too. I did not test, but looks promising that it passed tests. It looks big, but it is due to (a) tests (b) refactoring and moving code around. The patch is mostly just API-ification of some cleanup code so that it can be invoked when modules are enabled and disabled. Looks good IMHO.

Related issue: #1156576: Language negotiation is undocumented.

Committed to 8.x. Given the size of the patch, and the fact that it introduces new (helper) functions, I'm not going to commit this to 7.x. I'd like to have @webchick look into that instead. Hence, moving this issue from 8.x to 7.x.

Whew! Yeah, I don't quite have the brain-space for this sucker tonight.

plach, I'll try and catch you on IRC, though probably won't be able to until next week. :( I'll also try reading this when I'm not quite so exhausted. :)

Ok, went over this again this morning with a much fresher set of eyes and with plach to help me with a few questions. Overall, this is just taking some logic out of form submit functions, moving it to API functions, and then calling them from both the form and hook_enabled/disabled. It also greatly expands our tests for language negotiation, which is of tremendous help going forward.

Committed to 7.x. Thanks!

webchick++

Removing tags (needs backport to D7 queue cleanup)