Translations are skipped when importing

Wow what a find! How could we test this? Looks like a sample po file with 400 strings would suffice?

In other words, this patch needs a test added. :) Great catch though!!

...and here is a test.

Very meta, we are checking for the translation of "Next" in Norwegian, on the "next page" :)

The test fails as expected, and the test-plus-patch passes.

However, I did notice in the patch-plus-test that the test results show one coding standards failure:

core/lib/Drupal/Component/Gettext/PoStreamReader.php
line 44	Line indented incorrectly; expected 2 spaces, found 3

I was also wondering if maybe the test should verify that all of the strings in the test PO file were found, instead of just verifying the one that is skipped by the buggy current code? I think it might be good to add maybe 2 more strings to the PO file, and then verify that all of them got imported. This would ensure that another bug that maybe skips the first string in a batch or ?? who knows what, doesn't occur in the future.

Oh, forgot to say THANK YOU ericsm for making the test!

2 very good points, thanks!

Updated the PO file with 2 more X-Files related strings, and made the test verify all the strings in the file.

Also fixed the coding standard error and added some comments referring to this issue in the test. Sorry for no interdiff :(

Made readline to read line.

This looks great! Assuming that the test still passes (and I cannot see why it wouldn't, with a one-line docs-only change), this is RTBC. As noted above, the test-only patch failed in the expected way, and with the fix, it passes. The code and test are very clear. @hansfn tested the patch on a real site also.

Mostly looks great, thanks! A few concerns with the test:

1. +++ b/core/modules/locale/tests/src/Kernel/LocaleBatchImportTest.php
   @@ -0,0 +1,79 @@
   + * Tests building the translatable project information.
   

   Is this true? Doesn't really seem to explain what this test is actually testing. Not sure.

2. +++ b/core/modules/locale/tests/src/Kernel/LocaleBatchImportTest.php
   @@ -0,0 +1,79 @@
   +    // Import 2 batches, since this will reveal the bug.
   +    foreach (array_fill(0, 2, 'drupal') as $round_number) {
   

   A) What's 'drupal' got to do with it?
   B) What's $round_number got to do with it?

   If all we're doing is calling the inner code twice, I think this would be clearer:

   for ($i = 0; $i < 2; $i++) { 
   

   C) p.s. "will reveal the bug" is a little ambiguous. Future everyone would probably be grateful for a more specific explanation, so they don't have to git blame, do a bunch of archeology, find the issue nid and read it. E.g.:

   // Import 2 batches to ensure that subsequent imports properly seek in the .po file. 
   

   (or something... I'm only skimming the issue/patch).

3. +++ b/core/modules/locale/tests/src/Kernel/LocaleBatchImportTest.php
   @@ -0,0 +1,79 @@
   +    foreach ($strings as $string) {
   +      $item = $this->container->get('database')
   +        ->select('locales_target', 'lt')
   +        ->fields('lt')
   +        ->condition('lt.translation', $string)
   +        ->execute()
   +        ->fetchAssoc();
   +      $this->assertNotEmpty($item);
   +    }
   

   If this fails, we're just going to see:

   "Failed asserting that a boolean is not empty."

   We have no way to know which loop iteration we were on that failed. Per #3131946: [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages for assertions in loops, we should include a custom assertion message so we know exactly what failed. E.g.:

   $this->assertNotEmpty($item, "Translated string $string should exist.");
   

Two nitpicks:

1. +++ b/core/modules/locale/tests/src/Kernel/LocaleBatchImportTest.php
   @@ -0,0 +1,83 @@
   +  public function setUp(): void {
   

   protected function setUp(): void

2. +++ b/core/modules/locale/tests/src/Kernel/LocaleBatchImportTest.php
   @@ -0,0 +1,83 @@
   +   * Test that importing batches imports all translations.
   

   Tests that ...

Thank you @hansfn!

1. The test-only patch in #11 reproduced the bug, that's good.
2. All @dww's concerns in #18 were addressed in #19.

3. +++ b/core/modules/locale/tests/src/Kernel/LocaleBatchImportTest.php
   @@ -0,0 +1,83 @@
   +  protected function setUp(): void {
   

   The void return type is supported since 7.1, the min PHP version in 8.x is 7.0.8.

   As this is a bug, I think it's worthy backporting to 8.x. So let's make a patch for 8.x without returning type hint.

   Then I'd +1 to RTBC this to let committer to make the decision to backport or not.

4. +++ b/core/modules/locale/tests/src/Kernel/LocaleBatchImportTest.php
   @@ -0,0 +1,83 @@
   +    // Copy test po files to the translations directory.
   +    $this->filePath = \Drupal::service('file_system')->copy(__DIR__ . '/../../test.nb.po', 'public://', FileSystemInterface::EXISTS_REPLACE);
   ...
   +      'uri' => $this->filePath,
   

   Would suggest using vfsStream for tests. core/tests/Drupal/Tests/Component/Gettext/PoStreamWriterTest.php is an example of using it.

Thanks for the patch. It solves the problem of the translation import I faced (msgctxt was ignored for some translations in my case).

Tested and can be moved to RTBC

1. diff --git a/core/modules/locale/tests/src/Kernel/LocaleBatchImportTest.php b/core/modules/locale/tests/src/Kernel/LocaleBatchImportTest.php
   
   +++ b/core/modules/locale/tests/test.nb.po
   @@ -0,0 +1,27 @@
   +msgid ""
   

   The two files could be ignored from spelling check. core/modules/locale/tests/src/Kernel/LocaleBatchImportTest.php is a Test file, the other core/modules/locale/tests/test.nb.po is a test .po file.

   Or spelling check fails (The command to check spelling; cd core && yarn && yarn spellcheck:core)

2. +++ b/core/modules/locale/tests/src/Kernel/LocaleBatchImportTest.php
   @@ -0,0 +1,83 @@
   +    $this->filePath = \Drupal::service('file_system')->copy(__DIR__ . '/../../test.nb.po', 'public://', FileSystemInterface::EXISTS_REPLACE);
   

   Could be dirname(__DIR__, 2) . '/test.nb.po'

Addressing the above and #22.4, tagging "Global2020"

The actual bug this issue is about easily testable by pure unit test. No need for kernel test. If the kernel test is also there that is okay for me.

PoStreamReaderTest tests that if the PoStreamReader works correctly or not.

LocaleBatchImportTest tests that if the Gettext::fileToDatabase() uses the PoStreamReader correctly or not.

Wow, this issue was mindblowing. I just couldn't figure out why a single string would not be translated, while the translation was in the po file.
Crazy that this bug exists, as it fails on everty 200th / 400th / 600th / ... item on all translation files. Probabily more users have this issue, but just can't figure out what is causing this.

We applied the patch from #28 and it fixes the bug perfectly.
Also, the tests seem to be testing exactly what they should.
LGTM!

Re-roll patch #28 to the latest 9.3.x

There is an issue when the po translation file contains 2 (or more) empty lines at the end.

https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/loca...

if ($report['seek'] < filesize($file->uri)) {
The last empty line does not increase the seek value, and the filesize is bigger (by the number of extra empty lines at the end). The po file is re-started from the beginning (or form the last chunk?), an the import never finish.

Without this patch, the import (drush locale:update) does finish with files containing 2 empty lines at the end.

Rerolled #37 for 10.1.x

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I couldn't simplify the logic unfortunately, but this addresses #38 (and the edge case is added to both tests).

Sweetchuck suggested that it would be good to ensure that the line breaks at the end of the .po (fixture) files are preserved... (They might be stripped away by a properly configured IDE.) And he is right!

This patch moves both /core/modules/locale/tests/test.nb.po and /core/tests/fixtures/PoStreamReader/basic.hu.po into Nowdoc strings.

Side-effect: same patch can be applied on 10.1.x and 9.5.x 🙃

Tried testing by importing Norwegian Bokmål and got 99.55%

The issue summary should be updated with the default template, what's the proposed solution? remaining tasks? any api changes?

Tried to give this a proper summary.

I haven't closely reviewed all the new test coverage, so not RTBC'ing, but moving back to NR...

Is this still an issue though? I got up to 99% without the fix when the patch mentions I should get to 95

I Can retest also if there’s a missing step

Yeah, not totally sure about steps to reproduce manually and how to really see the bug in obvious ways. Back in #11 we had a failing test-only (vs. 8.9.x). Perhaps we should do another test-only here to see if all the tests still fail in 10.1.x without the fix. 😅

Also, removing credit from a couple of 1-off rerolls that didn't work and were then abandoned by the author. 😢

Yeah, locally, the new Kernel test still fails in 10.1.x:

% ./vendor/bin/phpunit -c core/phpunit.xml core/modules/locale/tests/src/Kernel/LocaleBatchImportTest.php
PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\locale\Kernel\LocaleBatchImportTest
F                                                                   1 / 1 (100%)

Time: 00:01.324, Memory: 10.00 MB

There was 1 failure:

1) Drupal\Tests\locale\Kernel\LocaleBatchImportTest::testImportBatches
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
     3 => 'Brukergrensesnitt'
     4 => 'Tittel'
     5 => 'Brødtekst'
-    6 => 'Neste'
-    7 => 'Små grønne menn'
-    8 => 'Verten'
+    6 => 'Små grønne menn'
+    7 => 'Verten'
 )

The new Unit test also fails, but it's less interesting: 😂

1) Drupal\Tests\Component\Gettext\PoStreamReaderTest::testReadItemsInMultipleSequence
Error: Call to undefined method Drupal\Component\Gettext\PoStreamReader::getSeekLastItem()

But if I restore the fix, then intentionally break PoStreamReader::getSeekLastItem(), the unit test fails more interestingly:

There was 1 failure:

1) Drupal\Tests\Component\Gettext\PoStreamReaderTest::testReadItemsInMultipleSequence
PO item 3
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'msgid "id-03"
-msgstr "str-03"
+'msgid ""
+msgstr ""
+"Project-Id-Version: Drupal core (9.2.7)\n"
+"POT-Creation-Date: "
+"2021-10-27 10:29+0000\n"
+"PO-Revision-Date: YYYY-mm-DD "
+"HH:MM+ZZZZ\n"
+"Language-Team: Hungarian\n"
+"MIME-Version: "
+"1.0\n"
+"Content-Type: text/plain; "
+"charset=utf-8\n"
+"Content-Transfer-Encoding: 8bit\n"
+"Plural-Forms: "
+"nplurals=2; plural=(n!=1);\n"
+""

 '

So I'm 99.9% sure this is still a bug, but the steps to manually reproduce it could be better explained. 😅 Tagging for that.

Grrr, I just did a new fairly long dreditor review, which refused to paste. 😢 But maybe it's for the best, since I decided it was easier to just fix everything and upload a new patch/interdiff than to re-do the review. 😂 Hopefully the interdiff mostly speaks for itself.

While I'm at it, here's a new test-only.

This is the only hunk of the interdiff that might need explanation:

+++ b/core/tests/Drupal/Tests/Component/Gettext/PoStreamReaderTest.php
@@ -232,14 +232,7 @@
-    $item = $reader->readItem();
-    $this->assertSame(
-      $expected['items'][5],
-      (string) $item,
-      'PO item 5',
-    );
-
-    for ($i = 6; $i < count($expected['items']); $i++) {
+    for ($i = 5; $i < count($expected['items']); $i++) {

The removed lines are identical to the body of the for() loop. I saw no reason to do item 5 manually, then a loop for 6 and up. So I folded item 5 into the loop.

Moving to NW for an answer to #54 and steps to reproduce.

Even with the patch I still only get to 99%

Going to ping the subsystem maintainer too

This feels major