The locale module does not import the plural forms from the compact files

Indeed. Affects D7 too, bumping there for correction.

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

could you place the comment above the code line?

@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?

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

Not critical, but major.

     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.

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: 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).

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

@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?

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

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

Marked as duplicate #769698: Incorrect work _locale_import_read_po()

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.

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

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

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

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 :)

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

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?

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

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.

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

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

+1 to commit

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

Suppose now this critical too

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!