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;
}
}
Comment | File | Size | Author |
---|---|---|---|
#5 | drupal.locale-get-plural.5.patch | 810 bytes | sun |
#4 | drupal.locale-get-plural.4.patch | 780 bytes | sun |
Comments
Comment #1
Gábor HojtsyOMG, 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.
Comment #2
Georgii CreditAttribution: Georgii commentedGá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.
Comment #3
fietserwinOr 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:
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.
Comment #4
sunOh my.
Comment #5
sunbleheheheh. global $language FTW :P
Comment #6
fietserwinFine with me.
Comment #7
Gábor HojtsyLooks good to me too.
Comment #8
catchJust 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 ;)
Comment #9
Gábor Hojtsy@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 :)
Comment #10
catchOops, that'll teach me not to read the issue first :p
Comment #11
David_Rothstein CreditAttribution: David_Rothstein commentedPatch 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.
Comment #12
Gábor HojtsyThat would be needs work then?
Comment #13
fietserwinTests 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.
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedWell, 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.
Comment #15
fietserwinImprovements 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.
Comment #16
David_Rothstein CreditAttribution: David_Rothstein commentedJust 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.
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks everyone! http://drupalcode.org/project/drupal.git/commit/988597f
Comment #18
Georgii CreditAttribution: Georgii commentedThanks!