Summary

In #1221276: locale_get_plural() only works for a single language within a request and does not work for English the locale_get_plural was updated and the issue was introduced.
There is a section in this function to statically cache plural formulas

    // Retrieve and statically cache the plural formulas for all languages.
    if (empty($plural_formulas)) {
      $language_list = language_list();
      foreach ($language_list as $langcode => $lang) {
        $plural_formulas[$langcode] = $lang->formula;
      }
    }

When the locale_get_plural function is called for the 1st time the variable $langcode is overwritten inside the foreach loop causing all the rest of this function to work improperly.

This is valid only for the 1st call of this function while "empty($plural_formulas)" is true.

Solution

rename $langcode var in foreach to something else. E.g.

    // Retrieve and statically cache the plural formulas for all languages.
    if (empty($plural_formulas)) {
      $language_list = language_list();
      foreach ($language_list as $lc => $lang) {
        $plural_formulas[$lc] = $lang->formula;
      }
    }
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

OMG, that sounds like a very basic error there. Doh. I verified this does not apply to Drupal 8 as there is no data collection like this in Drupal 8 anymore. http://api.drupal.org/api/drupal/core%21modules%21locale%21locale.module...

$lang is used to avoid collisions with the global $language. We should likely extend on the names, not shorten them, eg $item_langcode and $item_language instead of shortening them further. Drupal does not favor obscure shortening.

Georgii’s picture

Gábor Hojtsy,
Thanks for jumping in here! Can you please commit this change with your suggested extended names to D7? I cannot do it by myself, sorry.

fietserwin’s picture

Or remove it altogether, see comment #1221276-37: locale_get_plural() only works for a single language within a request and does not work for English. The caching is twice: in language_list() and in this function. So just fetch it - on every call - with something like:

  $language_list = language_list();
  if (isset($language_list[$langcode]->formula)) {
    ...
    // eval with $language_list[$langcode]->formula
  }
  else {
    // take default formula ($count != 1)
  }

pros:
- simpler
- doesn't process for languages that will not be used during this request (most of the times it will only be called for 1 language per
- saves a call to drupal_static() (though this will now actually be done by language_list())
request)
cons:
- adds a function call to language_list() on each invocation, but we are only talking the overhead of the function call itself here as language_list will only call drupal_static() which we saved on.

sun’s picture

Version: 7.14 » 7.x-dev
Priority: Normal » Major
Status: Active » Needs review
Issue tags: +Regression
FileSize
780 bytes

Oh my.

sun’s picture

bleheheheh. global $language FTW :P

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

Fine with me.

Gábor Hojtsy’s picture

Looks good to me too.

catch’s picture

Just in case webchick looks at this and wonders if it's a bug in 8.x too, it's not 'cos I just did that and checked ;)

Gábor Hojtsy’s picture

@catch: great, I did the same check in #1 right away and linked in the D8 equivalent code for anyone to verify so now its double checked :)

catch’s picture

Oops, that'll teach me not to read the issue first :p

David_Rothstein’s picture

Issue tags: +7.15 release blocker

Patch looks good. Any thoughts on whether we should be writing a test for this, though? I would tend to think we should, especially if the effect is (as described in the original report) "causing all the rest of this function to work improperly"... :)

Tentatively marking this as a release blocker, since it's a regression from earlier versions of Drupal 7.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

That would be needs work then?

fietserwin’s picture

Tests were added in #1221276: locale_get_plural() only works for a single language within a request and does not work for English. The tests seem to do a good coverage of what the function is about, but tests will never be able to catch all errors. So I'm not sure that adding (or in this case probably rearranging the tests in a specific order) to uncover this error, adds any value (to a version that is in maintenance mode and will not see much refactoring).

So IMO, it is back to RTBC.

David_Rothstein’s picture

Status: Needs work » Needs review

So I'm not sure that adding (or in this case probably rearranging the tests in a specific order) to uncover this error, adds any value (to a version that is in maintenance mode and will not see much refactoring).

Well, if we improved the tests, that part wouldn't just be for Drupal 7; the improvements would go into Drupal 8 also.

However, thanks for pointing out the existing tests. I do see what you mean that they already provide very good test coverage of this function. However, if all it takes to expose this bug is rearranging the tests, then that probably means it isn't very hard to do either :)

Anyway, I'll leave it up to you folks whether or not you think it's worth going ahead and changing the tests or not.

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

Improvements woudn't go in D8 as this error is not in D8 (#1, #8). So I still think that this is a case of bad luck that we didn't catch this error with the test as they are. So IMO RTBC and let's not hold up 7.15.

David_Rothstein’s picture

Improvements woudn't go in D8 as this error is not in D8 (#1, #8).

Just because a bug doesn't exist currently doesn't mean you can't have a test to prevent it from ever happening!

In any case, I'll leave this RTBC for a few days in case it gets more reviews, then commit it afterwards. Thanks.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.15 release notes
Georgii’s picture

Thanks!

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