Reproduction:
- install a new Drupal site
- enable the locale module
- add a new language (e.g. Hungarian)
- export the translation of the potx module from the l.d.o., using the "Compact files optimized for size, not desktop editing" format
- import the translation
- the locale module imports 55 strings, and does not import the translation of the "one day/@count day" string.

There is not a message or a watchdog entry, the plural forms simply skipped, as well as any compact files, like the Drupal core translation for example.

Files: 
CommentFileSizeAuthor
#26 locale-plural-fix-and-test.patch3.02 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 27,036 pass(es). View
#25 860154-plural-import-fix-d7.patch7.03 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,844 pass(es). View
#22 locale-plural-fix-and-test.patch3.02 KBGábor Hojtsy
None View
#19 locale-plural-test.patch1.85 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 22,777 pass(es), 1 fail(s), and 0 exception(es). View
#4 860154_locale_does_not_import_plural_forms_4.patch753 byteszserno
PASSED: [[SimpleTest]]: [MySQL] 22,141 pass(es). View

Comments

Zoltán Balogh’s picture

Status: Active » Needs review

I do not know, how can I create a patch for the core, but the solution is:

includes/locale.inc, line 1148
old: if ($context == "MSGSTR") { // End current entry, start a new one
new: if ($context == "MSGSTR" || ($context == "MSGSTR_ARR")) { // End current entry, start a new one

Damien Tournoud’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Needs work

Indeed. Affects D7 too, bumping there for correction.

Zoltán Balogh’s picture

Status: Needs review » Needs work

It is necessary to look my solution better for 6.x, because if the l10n_update module refreshes the translations, then the plural forms are left out from the database.

The l10n_update duplicate the code from include/locale.inc
#860480: This module does not import the plural forms from the compact files

zserno’s picture

Status: Needs work » Needs review
FileSize
753 bytes
PASSED: [[SimpleTest]]: [MySQL] 22,141 pass(es). View

Attached a proper patch for D7. I tested Zoltan's solution and it did work for me.

Zoltán Balogh’s picture

Priority: Normal » Critical
Status: Needs work » Needs review

Do not release D7 without this fix

aspilicious’s picture

could you place the comment above the code line?

zserno’s picture

@aspilicious Sure I could, but if you take a look at locale.inc, it has a number of lines with such documentation style.
So I'd rather clean up the whole file in a separate issue. What do you think?

aspilicious’s picture

Hmm well leave it this way, I'll fix those in D8 ;)

Damien Tournoud’s picture

Priority: Critical » Major

Not critical, but major.

Damien Tournoud’s picture

     elseif (!strncmp("msgid", $line, 5)) {
-      if ($context == "MSGSTR") {   // End current entry, start a new one
+      if ($context == "MSGSTR" || $context == "MSGSTR_ARR") {   // End current entry, start a new one.
         _locale_import_one_string($op, $current, $mode, $lang, $file, $group);
         $current = array();
       }

- There is other case (in msgctxt) where we switch to a new entry. We need the same fix there.

- We need to update the .po parsing tests that we have.

Gábor Hojtsy’s picture

Why is that this issue comes up with the compact files only? Also, how does the patch solve Drupal not importing the plural formula (if that is also happening as far as I understood)?

Zoltán Balogh’s picture

6.x: In the verbose translation files, after the plural formula there is an empty and a comment line before the next "msgid". In this case, the locale.inc works fine, and the _locale_import_one_string at the line 1123 will import the last plural formula. In the compact files after the "msgstr[" the next line immediately starts with a new "msgid". In this case the value of the $context is MSGSTR_ARR yet (last line was a msgstr[1]), and this condition is missing from the line 1148. Therefore the locale.inc ignoring all plural formulas, except if the plural formula is the last in the compact translation file.

7.x: I do not know, and I think, it would be better if we separate the versions.

Gábor Hojtsy’s picture

@Zoltán Balogh: ok, so its the plural variants for source strings, not "the plural formula" (which is in the header section of the file). The Drupal 7 import code is very similar and probably has the same issue (I did not reproduce).

andypost’s picture

Just a remark, compact files could not used for JS strings. Because JS translation depends on filenames in .po file (s.location LIKE '%.js%') in _locale_rebuild_js()

EDIT: filed #864064: Compact files format could not used for JS strings

Gábor Hojtsy’s picture

@andypost: indeed, that should actually be a bug report against l10n_server's compact file format. It should include the location info for JS files (at least ".js" as location, so it will work with the JS translation in D6/7). Can you submit a report for that in l10n_server?

Gábor Hojtsy’s picture

Bug also applies to Drupal 5, reported at #843154: Plural forms not imported into Drupal 5.22 core (marked as duplicate).

andypost’s picture

There's issue to add test for plurals #848966: Improve locale tests

andypost’s picture

Gábor Hojtsy’s picture

FileSize
1.85 KB
FAILED: [[SimpleTest]]: [MySQL] 22,777 pass(es), 1 fail(s), and 0 exception(es). View

So the bug in short is that the state machine is not implemented consistently. If a comment follows an msgstr or an msgstr[] then the previous string is saved in the DB. However, if an msgid or msgctxt is encountered, the previous string is only saved if it was an msgstr. So a plural string translation can only be followed by a comment or end of file to be saved. If it is followed by a new string (which starts with msgid or msgctxt in D7), then it will not be saved. Attached a test extension to check for this. The issue referenced by @andypost does not cover plural imports, so thats not relevant.

The attached patch should fail by definition. I'll attach the fix with the patch in another comment.

Status: Needs review » Needs work

The last submitted patch, locale-plural-test.patch, failed testing.

andypost’s picture

Gábor Hojtsy, issue I'm referencing is about export fix so I think it's just addition to import

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.02 KB
None View

Ok, we proved above the tests fail. Now this patch includes a fix with the very same tests as in #19. Should pass.

andypost’s picture

Gábor Hojtsy, any reason you are using ((==) || (==)) ? Code is more readable without expensive () and == has priority on ||
PS: http://www.php.net/manual/en/language.operators.precedence.php

Gábor Hojtsy’s picture

It is already like that at other parts of the code. I'm trying to restrict myself to the bugfix not to introduce other differences or fix other parts of that code which are not broken. Feel free to submit cleanups once this is committed. There is plenty to clean up here :)

andypost’s picture

FileSize
7.03 KB
PASSED: [[SimpleTest]]: [MySQL] 22,844 pass(es). View

This is a only function in this file with broken code-style. It's easy to fix while you on it.
This is a re-roll

If you strong against this, please, post a follow-up

Gábor Hojtsy’s picture

FileSize
3.02 KB
PASSED: [[SimpleTest]]: [MySQL] 27,036 pass(es). View

Look, I'd like to see this committed to Drupal 6, so it gets into the (imminent) upcoming D6 release. If we continue arguing about code style for D7, that's not gonna happen, and we have a broken localization stack to present at Drupalcon Copenhagen. That I don't want. I'd rather postpone minuscule code style issues when we can fix the bug. Let's get the bugfix in and then you can do whatever you want. I'd for one argue that "// A comment." looks really that helpful. Let's get that damn bug fixed, ok?

andypost’s picture

Agreed exactly! This patch hangs too long. Waiting a bot goes green

PS Filed follow-up #880278: Cleanup _locale_import_read_po()
I will re-roll patch after this issue would be commited

Gábor Hojtsy’s picture

Given I believe this is important for the D6 release that is imminent and was tested earlier on D6, committed there: http://drupal.org/cvs?commit=405676. Above code+test passed for me on local machine, and seen manual tests, so I'm confident it works well. Still needs testbot feedback and commit for D7.

Sutharsan’s picture

Does this patch still need a review? Reading Gábor's last comment this patch should be RTBC

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yes, the patch looks good, has extended test coverage and is already committed to Drupal 6.

andypost’s picture

+1 to commit

tom_o_t’s picture

#26: locale-plural-fix-and-test.patch queued for re-testing.

andypost’s picture

Priority: Major » Critical

Suppose now this critical too

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Thanks. This was totally off my radar. And because it fixes a bug that's already fixed in D6, agree with critical status.

Committed to HEAD!

Status: Fixed » Closed (fixed)

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