locale_get_plural() only works for a single language within a request and does not work for English

Please do not merge other issues/patches. The patch from #1221254: locale_get_plural() only works for a single language within a request was bogus, so this one is too.

In case this patch depends on the other issue, then we need to mark this issue as postponed.

It seems you are missing the point of what's happening in the individual conditions of this function. I can see how not understanding functionality can be frustrating, but that's not a reason to make utterly wrong claims and bash on the general quality of code in Drupal core.

1. The plural formula is only retrieved once per language. (or "should", that's what the other patch fixes) The result is statically cached for performance.
2. Which plural form to use is only determined once per language and $count. The result is statically cached for performance. Please note that eval(), and especially error suppression is slow.
3. In case there is no plural formula, -1 is returned. This result is statically cached, too, because it will always be identical per language and count.

This is what you intended to do.

#1221254: locale_get_plural() only works for a single language within a request has been marked as duplicate, since this patch is superior.

+++ b/modules/locale/locale.module
@@ -736,18 +736,25 @@ function locale_get_plural($count, $langcode = NULL) {
+      $plurals[$langcode][$count] = (int) $count != 1;

Forgot parenthesis here.

Fixed in attached patch.

Looks like good progress here. This is a pretty forgotten place of the API I must say...

I tested the patch in #8 on multilingual Drupal 7.10 and it works. Thank you.

- I think you can remove the comment on why we import .po files to set the plural form. At least don't include the alternate PHP code :)
- hr is Croatian, not Hungarian
- not sure the naming of the PO file helper functions is good in this context (I guess you copied from somewhere else). Most of their content have no usefulness for the test and whether they have or not have contexts is irrelevant also, right?

Tagging as current focus.

@fietserwin: can you help with updating the tests? I think this looks like it would be pretty close otherwise.

Anybody else want to look into this?

All right, I thought this would be simple for someone to take, but apparently not. Some notes for this update:

1. The issue that locale_get_plural() cannot be used with multiple languages in the same request was solved earlier by pulling in the whole list of plural configurations. That is not a bug to solve here anymore. However, the non-existent English plural formula is a bug for the 80% case where you start translating English but obviously just start on the UI, you don't have a plural form to import from a .po file.

2. Renamed the variables inside locale_get_plural() for clarity. There was probably too much dark magic going on in the function names, hopefully now much better. Also took all comment from the above patch and then added some :)

3. Took the test, updated it to the current API and improved it. Almost only left the relevant parts of the .po files plus added a non-existent Hungarian language as well so we can test for the -1 error condition (and fixed the code comment in format_plural() to better explain why it does what it does). Fixed comments and test feedback to be better English (hopefully).

Please review!

Adding UI language translation tag.

Hi,

I reviewed and tested the #18 patch and it seems OK!

I change the status to RTBC.

Kálmán

All the summaries should start with third-person verbs. ;)

@xjm: sampling of comments from locale.test outside of this patch:

/**
 * @file
 * Tests for locale.module.
 * ...
 */

/**
 * Functional tests for language configuration's effect on negotiation setup.
 */
class LocaleConfigurationTest extends DrupalWebTestCase {
}

  /**
   * Functional tests for adding, editing and deleting languages.
   */
  function testLanguageConfiguration() {
  }


  /**
   * Adds a language and tests string translation by users with the appropriate permissions.
   */
  function testStringTranslation() {
  }


  /*
   * Adds a language and checks that the JavaScript translation files are
   * properly created and rebuilt on deletion.
   */
  function testJavaScriptTranslation() {
  }

  /**
   * A user able to create languages and import translations.
   */
  protected $admin_user = NULL;


  /**
   * Test import of standalone .po files.
   */
  function testStandalonePoFile() {
  }


  /**
   * Test automatic import of a module's translation files.
   */
  function testAutomaticModuleTranslationImportLanguageEnable() {
  }


  /**
   * Helper function: import a standalone .po file in a given language.
   *
   * @param $contents
   *   Contents of the .po file to import.
   * @param $options
   *   Additional options to pass to the translation import form.
   */
  function importPoFile($contents, array $options = array()) {
  }


  /**
   * Helper function that returns a proper .po file.
   */
  function getPoFile() {
  }


  /**
   * Verify that function signatures of t() and st() are equal.
   */
  function testFunctionSignatures() {
  }


  /**
   * Functional tests for the language switcher block.
   */
  function testLanguageBlock() {
  }

In short it is VERY hard to find anything in this file that conforms to that rule. Even the two examples I cited above which do start with a third person verb are at error. The first one was above 80 chars, the second is two lines of summary. Both are invalid, right?

So (a) I don't seem to find examples of how to word test class descriptions, helper function descriptions, class variable descriptions, etc. starting with 3rd person verbs to apply to the patch and (b) I don't know if it makes sense if we fix it in the 3 comments added in this patch and not elsewhere. I do want to keep this backportable though because the bug exists in Drupal 7 and before and it would be useful to backport. (Edit: Also keep the issue scope focused instead of turning into a doc cleanup task).

Any tips?

Also @catch previously communicated that we should mark issues where others depend on solutions as major. The following two bugs were marked as postponed on this issue earlier, so this should be major with that logic: #655048: Plural formula information blanked when importing a poorly-formed .po file and #1220964: Number field prefix/suffix get t()'ed through format_plural()

#1326618: Clean up API docs for locale module will take care of correcting docs in the rest of the module. We just need to make sure that new docs we add or change follow the standards in:
http://drupal.org/node/1354#functions
http://drupal.org/node/1354#classes

Here is a reroll with the docs changed as suggested. Interdiff attached. Please help move this forward again. Two issues depend on it, and its a pretty small issue at that.

Yay! Thanks Gábor.

+++ b/core/modules/locale/locale.test
@@ -586,6 +586,125 @@ class LocaleTranslationFunctionalTest extends DrupalWebTestCase {
+    $this->importPoFile($this->getPoFileWithSimplePlural(), array(
+      'langcode' => 'fr',
+    ));
+    $this->importPoFile($this->getPoFileWithComplexPlural(), array(
+      'langcode' => 'hr',
+    ));
+
+    // Test locale_get_plural() for English (no formula presnt).
+    $this->assertIdentical(locale_get_plural(1, 'en'), 0, t("Computed plural index for 'en' with count 1 is 0."));
+    $this->assertIdentical(locale_get_plural(0, 'en'), 1, t("Computed plural index for 'en' with count 0 is 1."));
+    $this->assertIdentical(locale_get_plural(5, 'en'), 1, t("Computed plural index for 'en' with count 5 is 1."));

This is a wrong way to test this functionality.
Import is happen in different process but asert() is happen within old instance operationg with old variables and statics

By the same way we have broken test within testStandalonePoFile()

At least 3 statics should be reseted

    // Reset static cache because languages are imported in other requests.
    drupal_static_reset('language_list');
    drupal_static_reset('locale_get_plural');
    drupal_static_reset('locale_get_plural:plurals');

Got two issues related to this on D7:
1. Incorrect plural form when using
format_plural(3, '@count drupaler', '@count drupalers', array(), array('context' => 'Multilingual', 'langcode' => 'ru'))
2. Notices about incorrect language parameter.

These patches includes next changes:
1. Updated tests with separate page implementation
2. Backported to D7

D8 patch without suffix to allow testing

@andypost, @gumanist: can you elaborate on why did it pass before for the validation of those complex formulas if it was using the wrong data? I'd rather not do this whole proxy webpage thing for testing if possible (we can clear/reinitialize variables/caches for testing if we need to).

@gumanist: can you elaborate on the two errors you got on D7 and what you added to the patch to overcome them?

@andypost, @gumanist: I gave more thought to this test data issue. If you put simple debug() statements on the variable in the test before and after the .po files are initialized, you'll see that there are no plural formulas before so the default array is used, and then the plurals are properly initialized. I think the test can assume that it starts from a blank slate and that there is no locale_get_plural() cache to clear since it was not cached (neither the variable). Also, locale_get_plural() does not use language_list in the D8 version so no need to touch that either.

All-in-all looks like you are trying to inflict extra pain on the D8 patch/test for something you've experienced with D7, but this issue is not about D7 at the moment. Why do we need to complicate this so much when we can assume the environment we are in. We don't have any languages at first in this test, then add two, so we can assume what kind of environment is available and don't need to do all this extra stuff you suggested AFAIS.

After some running the tests and debug I think I have some explanation to all of these questions. My conclussions first:

- Though D8 tests in Gabor's patch work and they do test what they try to test (that assert happens in the same db instance, the only difference are static variables) ...

- it wouldn't be bad some drupal_static_reset() to make sure we start from a clean state for these variables -we actually don't, but we are simply lucky the variable is not yet set to a non empty value (#25), that empty($plural_formulas). -

This is what is happening:

- The locale_get_plural() function is called during test initialization. Don't ask me why but it is just called, you can see it adding some debug. It may be the po import.

- For D7 as we rely on language_list(), which has only English the first time the function is called, $plural_formulas is initialized to some non empty value, array ( 'en' => ''), so it doesn't get the right value later.

- For D8, $plural_formulas is initialized with some variable and it gets an empty value the first time it's called, so it is initialized again on next call using the new imported languages.

I think this explains why tests work for D8 and not for D7. Also tests without further page requests are ok though to be safe about static variables maybe using some more page requests could be better, Which doesn't mean we should use tests in #28 because these don't really tests requests with multiple languages as they just try a single language per request.

So I'd leave this to 'needs work' (minimal, just some static resets to make sure we have a clean start and we are better future-wise in case something changes later with test initializations that lets us with some 'polluted' static variables).

And, but maybe for another issue, we should add some function that can be reused for other tests to fully reset locale variables because this kind of things just happen too often when testing locale features, and that's because normal test initialization starts the locale system too.

Ok here is a new version of the patch with the locale_get_plural() caches reset before they are tested (although as discussed above this does not actually do anything in the test, since they were not yet invoked). If their static caches would contain outdated data, the majority of the tests would fail, which is clearly not the case.

Since this patch is the same as was RTBC for 10 days minus some docs fixes plus a minor cache clears added that do not change the behavior of the tests at all, I think this should be back to RTBC once it proves it passes (still).

Back to RTBC as per above.

I don't understand why we're static caching the plural_formulas, since that's just coming from variable_get() anyway?

Having said that this pre-exists the patch and it just happens those lines are being moved around, so we can look at that in a followup (and it'll need reconsidering with CMI anyway).

Committed/pushed to 8.x, moving to 7.x for backport.

Right, the variable/static issue is going to be revisited IMHO when we do the CMI migration. This commit unblocked #655048: Plural formula information blanked when importing a poorly-formed .po file and #655048: Plural formula information blanked when importing a poorly-formed .po file so following up on those.

#655048: Plural formula information blanked when importing a poorly-formed .po file is marked postponed on this one in D7 too as it needs this backported first to apply tests just like in D8.

@fietserwin or @kalman.hosszu: can you help with backporting the functional change and the test for now? I think cleanup can be done later once we ensure the bug is fixed.

Unfortunately I don't have hordes of people to do D7 backports. Getting the D8 patches done (and committed) is hard enough. Removing sprint tag.

@Gábor Hojtsy:
About #1
Don't know why, but I couldn't reproduce it now. Hope this is good and it was something with configuration :)

About #2
Example:
When you don't have a russian language (ru) in system this code

format_plural(67, '1 drupaler', '@count drupalers', array(), array('langcode' => 'ru'));

or this

locale_get_plural(2, 'ru');

returns notices

Notice: Undefined index: ru in locale_get_plural() (line 748 of Z:\home\d7-tmp\www\modules\locale\locale.module).
Notice: Trying to get property of non-object in locale_get_plural() (line 748 of Z:\home\d7-tmp\www\modules\locale\locale.module).

About tests: imho you are creating 'synthetic' enviroment by clearing static variables, etc. Changing anything with static variables will cause incorect tests, which could pass, but maybe not. My tests are slower and more 'resource eater', but they test same as user gets. And it is up to you which approach to use.

My patch is rewritten version of yours, so it could be used as backported version or as a base for it.

@gumanist: your test does not test the original problem, that when calling format_plural() with multiple languages in the same request, it will not take the plural formula for the second, third, etc. languages anymore. It only takes the first in D7.

Will it be enough if I will just add a page with that number of calls to the test? Or you have a suggestions to the patch itself too?

I guess if you do all the plural generation on one page and then do all the checking of the HTTP output in the test, that would also do it. But this is essentially a very slim API function, so I still think that looks like big overkill just for the tests to test it via HTTP.

I'm agree with you on one hand, but on other the only one HTTP request is nothing compare to Drupal installation from setUp() method.

Patch is updated and all requests done on one page.

Changing status to run tests

Patch looks good except code-style, probably it's ok because this arrays are long. But please format this same way

+++ b/modules/locale/locale.testundefined
@@ -694,6 +694,133 @@ class LocaleTranslationFunctionalTest extends DrupalWebTestCase {
+  function assertPluralFormat($count, $lang, $expected_result) {
+    $message = t("Computed plural index for '@lang' with count @count is @expected_result.",
+                  array(
+                    '@lang' => $lang,
+                    '@count' => $count,
+                    '@expected_result' => $expected_result
+                  ));
+    $this->assertText(format_string('Language: @lang, locale_get_plural: @expected_result.',
+                                    array('@lang' => $lang, '@expected_result' => $expected_result)),
+                      $message);

this hard to read

+++ b/modules/locale/tests/locale_test.moduleundefined
@@ -113,3 +129,110 @@ function locale_test_store_language_negotiation() {
+function locale_test_plural_format_page() {
+  $tests = _locale_test_plural_format_tests();
+  $result = array();
+  foreach ($tests as $test) {
+    $result[] = array(
+      '#prefix' => '<br/>',
+      '#markup' => format_string('Language: @lang, locale_get_plural: @locale_get_plural.',
+                                 array(
+                                      '@lang' => $test['language'],
+                                      '@locale_get_plural' => locale_get_plural($test['count'], $test['language']))),
+    );
+  }
+  return $result;

same

+++ b/modules/locale/tests/locale_test.moduleundefined
@@ -113,3 +129,110 @@ function locale_test_store_language_negotiation() {
+}
\ No newline at end of file

add new line

The modified patch is attached.

Looks good to me, and the actual bugfix looks well ported from D8. Any more concerns @andypost?

Good except minor code-style

+++ b/modules/locale/locale.moduleundefined
@@ -737,30 +737,50 @@ function locale_reset() {
  * @param $langcode
  *   Optional language code to translate to a language other than
  *   what is used to display the page.
+ * @return

@return requires blank line before

@andypost Thanks for your feedback! Anything else?

Yes, good to go

Great, folks. Thanks!

Committed and pushed to 7.x.

Submitted a follow up issue regarding to this fix #1567662: The 1st plural index on a page is not calculated correctly

Changed the proposed solution part.