Translations are skipped when importing

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.

The last submitted patch, 53: 3126929-53.test-only.patch, failed testing. View results →

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