The language_count variable tracks how many languages are enabled and is used to tell if a site is multilingual and hence trigger the language negotiation process. It is updated everytime a new language is enabled or disabled, but it is not decremented when removing a language, hence it might hold a wrong value.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet’s picture

Component: language system » locale.module
Status: Active » Needs review
FileSize
672 bytes
plach’s picture

Component: locale.module » language system

Thanks for the patch, but please don't change the component: the language_count variable is at the very core of the whole language subsystem.

However:

+++ modules/locale/locale.admin.inc
@@ -469,6 +469,7 @@ function locale_languages_delete_form_submit($form, &$form_state) {
+    variable_set('language_count', variable_get('language_count') - 1);

Minor: variable_get() is missing the default value.

We should update the tests to capture this bug: checking that the language_count is equal to the number of enabled languages after deleting one should be enough.

Powered by Dreditor.

droplet’s picture

Status: Needs review » Needs work

we are called locale_languages_delete_form_submit which in locale module to remove a language. It is happened in locale module ??

If not, where should tests go into? simpltest/tests? system module ? thanks.
(but if you only run locale tests, then it won't catch errors ?)

plach’s picture

Part of the language system code is in the locale.inc / locale.admin.inc files and all the simpletests are in locale.test, so updating LocaleConfigurationTest should be fine.

We probably need also an update function ensuring that language_count is equal to the number of enabled languages.

Désiré’s picture

Here is a patch for test this bug.

Désiré’s picture

Status: Needs work » Needs review

for the testbot

plach’s picture

Status: Needs review » Needs work

@Désiré:

Thanks for the patch, the test looks good except for a couple of minor things. Anyway we need a unique file shipping the fix and the related test. Could you roll a patch merging #1+#2 and #5+#7?

+++ b/modules/locale/locale.test
@@ -145,6 +145,12 @@ class LocaleConfigurationTest extends DrupalWebTestCase {
+    // Make sure, "locale_count" variable is equal with the count of enabled languages

The comment should wrap at column 80 and should end with a dot. A rephrase is also needed here:
"Make sure the "locale_count" variable is equal to the number of enabled languages."

+++ b/modules/locale/locale.test
@@ -145,6 +145,12 @@ class LocaleConfigurationTest extends DrupalWebTestCase {
+    $language_list = locale_language_list();
+    $enabled_languages = count($language_list);

Normally to get a list of enabled languages we use language_list('enabled'). For consistency we might do it also here.

Powered by Dreditor.

Désiré’s picture

Status: Needs work » Needs review
FileSize
1.89 KB

Here is a patch for the bug and for the test.
I think both patches are good, but the test fails... So pleas review it.

Status: Needs review » Needs work

The last submitted patch, 1099396-language_count-8.patch, failed testing.

plach’s picture

Thanks!

+++ b/modules/locale/locale.admin.inc
@@ -468,6 +468,11 @@ function locale_languages_delete_form_submit($form, &$form_state) {
     module_invoke_all('multilingual_settings_changed');
+    $enabled = language_list('enabled', TRUE);
+    if (isset($enabled['1'][$form_state['values']['langcode']])) {
+      unset($enabled['1'][$form_state['values']['langcode']]);
+    }
+    variable_set('language_count', count($enabled['1']));

Please move this hunk before the hook invocation. However I guess it's safer to simply decrement the language_count variable to avoid tricky static cache issues (see below).

+++ b/modules/locale/locale.test
@@ -145,6 +145,13 @@ class LocaleConfigurationTest extends DrupalWebTestCase {
+    // Make sure the "locale_count" variable is equal to the number of
+    // enabled languages.

Comment lines should wrap at column 80 or the nearest lesser value: "enabled" can be moved one line up.

+++ b/modules/locale/locale.test
@@ -145,6 +145,13 @@ class LocaleConfigurationTest extends DrupalWebTestCase {
+    $enabled = language_list('enabled');

Probably before this line you need to clear the static cache through drupal_static_reset('language_list') to make the test pass.

Powered by Dreditor.

plach’s picture

Désiré’s picture

Status: Needs work » Needs review
FileSize
1.86 KB

Here is the good patch.
I tested it on D7 and D8.

@plach thx for your help!

Désiré’s picture

reupload it, without the D8 extension, to run the testbot

Désiré’s picture

I found a better fix for the bug.

Here is the new patch (tested on D8 but should work on D7).

plach’s picture

Status: Needs review » Needs work

Nice catch! Now we should update the test to check that the language_count variable is not altered when deleting a disabled language.

Désiré’s picture

Status: Needs work » Needs review
FileSize
3.16 KB

Here is it.

plach’s picture

Status: Needs review » Needs work

Err, sorry but I meant we should test both scenarios:

  1. we should have three installed language, for instance: en, fr, it (disabled)
  2. we would remove "it" and check that language_count has not changed
  3. we would remove "fr" and check that language_count has been updated correctly

This way we are sure that both behaviors are correct.


+++ b/modules/locale/locale.test
@@ -125,6 +125,15 @@ class LocaleConfigurationTest extends DrupalWebTestCase {
+    // Disable an enabled language. Disabling a language will recount the
+    // "locale_count" variable. It must be done before deleting any language,
+    // to test correctly the changing of the variable.

We need a rephrase here: "Disable an enabled language. Disabling a language will update the "locale_count" variable. This must be done before deleting any language to test that the variable is updated correctly."

Powered by Dreditor.

plach’s picture

One thing that had been forgot from #4:

We probably need also an update function ensuring that language_count is equal to the number of enabled languages.

This should be as easy as putting in locale.install the following code, but only for the D7 patch:

<?php
function locale_update_7003() {
  $languages = language_list('enabled');
  variable_set('language_count', count($languages[1]));
}
?>
Dries’s picture

+++ b/modules/locale/locale.admin.inc
@@ -467,6 +467,9 @@ function locale_languages_delete_form_submit($form, &$form_state) {
+    if ($languages[$form_state['values']['langcode']]->enabled) {
+      variable_set('language_count', variable_get('language_count', NULL) - 1);
+    }

Would be good to add a code comment describing what the variable is for. Some context would help.

plach’s picture

Honestly I never quite understood the need for this variable but never worried about it too much. Probably we need Gabor or Jose on this, I ain't sure who introduced it.

In D7 core it is read only while updating it and by drupal_multilingual(), where it could be replaced by:

<?php
function drupal_multilingual() {
  $languages = language_list('enabled');
  return count($languages[1]) > 1;
}
?>
plach’s picture

Got it. It avoids the need of querying the {languages} table on monolingual sites:

<?php
function language_list($field = 'language') {
  $languages = &drupal_static(__FUNCTION__);
  // Init language list
  if (!isset($languages)) {
    if (drupal_multilingual() || module_exists('locale')) {
      $languages['language'] = db_query('SELECT * FROM {languages} ORDER BY weight ASC, name ASC')->fetchAllAssoc('language');
      // Users cannot uninstall the native English language. However, we allow
      // it to be hidden from the installed languages. Therefore, at least one
      // other language must be enabled then.
      if (!$languages['language']['en']->enabled && !variable_get('language_native_enabled', TRUE)) {
        unset($languages['language']['en']);
      }
    }
    else {
      // No locale module, so use the default language only.
      $default = language_default();
      $languages['language'][$default->language] = $default;
    }
  }

  // ...
}
?>
Désiré’s picture

Here is two patches:
for D8 with the test for all scenarios in #17
D7 only with the update hook

Désiré’s picture

Status: Needs work » Needs review
plach’s picture

Status: Needs review » Needs work
+++ b/modules/locale/locale.admin.inc
@@ -467,6 +467,10 @@ function locale_languages_delete_form_submit($form, &$form_state) {
+      // Update the "language_count" variable (count of enabled languages).

Personally I don't think this is the right place to document the variable. For consistency we would need to add similar/identical comments around all the other code updating it. I guess the right place to document it is drupal_multilingual(). Anyway we need a more meaningful explanation, something like "The language_count variable stores the number of enabled languages, to avoid unnecessarily querying the database when building the list of enabled languages on monolingual sites."

+++ b/modules/locale/locale.test
@@ -144,6 +144,31 @@ class LocaleConfigurationTest extends DrupalWebTestCase {
+    $this->assertEqual(variable_get('language_count', NULL), count($enabled[1]), t('Language count is correct.'));

Sorry, I missed this one previously: the default "language_count" value should be "1" and not "NULL".

+++ b/modules/locale/locale.test
@@ -144,6 +144,31 @@ class LocaleConfigurationTest extends DrupalWebTestCase {
+    // Make sure the "language_count" variable has not changed.
+    $this->assertEqual(variable_get('language_count', NULL), count($enabled[1]), t('Language count is correct.'));

As above the default "language_count" value should be "1" and not "NULL".

Powered by Dreditor.

droplet’s picture

variable_get('language_count', NULL) - 1);

this "language_count" default value should be "2"

we always have 1 language actives.

plach’s picture

The default value for a variable should be always the same. The default for language_count is 1:

<?php
/**
 * Return true if there is more than one language enabled.
 */
function drupal_multilingual() {
  return variable_get('language_count', 1) > 1;
}
?> 
Gábor Hojtsy’s picture

Yes, the idea is that this would avoid extra queries where not needed, since we already have the default language as a variable for other reasons (eg. to work on a site without locale enabled)

Désiré’s picture

Status: Needs work » Needs review
FileSize
3.26 KB
3.73 KB

Here is the fixed patches.

And I found a comment for the variable in locale.inc, I think no other documentation needed:

487-  if ($enabled) {
488-    // Increment enabled language count if we are adding an enabled language.
489:    variable_set('language_count', variable_get('language_count', 1) + 1);
490-  }
491-
plach’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.81 KB
4.27 KB

The patches look good and fix the issue (tested both on D7 + update and on D8).

Since Dries explictly asked for additional documentation I introduced a comment as suggested in #24. Aside from this the patches are the same as those in #28 so setting RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks all.

plach’s picture

Version: 8.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)
Issue tags: +Needs backport to D6

This can be easily backported to D6.

droplet’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.31 KB

it's maybe my first D6 branch patch :)

Anonymous’s picture

Status: Needs review » Closed (won't fix)

I'm going to guess this is a won't fix for D6, since D6 commits focus on security fixes at this point.

Anonymous’s picture

Version: 6.x-dev » 7.x-dev
Status: Closed (won't fix) » Fixed

Changing issue status to reflect that it was fixed in 7.x.

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D6, -Needs backport to D7

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