Problem/Motivation

Drupal core was made to use semantic versioning in #2170443: [meta] Create a plan for implementing semantic versioning for core. The local translation file detection was not updated to account for that.

Proposed resolution

Update local file detection. Update tests.

Remaining tasks

Review. Commit.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#28 interdiff-2323259-28-23.txt1019 bytessutharsan
#28 2323259-translation-file-version-28.patch10.1 KBsutharsan
#23 2323259-translation-file-version-23.patch10.08 KBsutharsan
#22 interdiff-2323259-15-19.txt1.88 KBsutharsan
#19 interdiff-2323259-15-19.txt7.09 KBsutharsan
#19 2323259-translation-file-version-19.patch7.32 KBsutharsan
#15 2323259-diff-6-15.txt4.47 KBvijaycs85
#15 2323259-translation-file-version-15.patch9.59 KBvijaycs85
#6 2323259-semver-core-translation-file-6.patch6.42 KBgábor hojtsy
#6 interdiff.txt2.59 KBgábor hojtsy
#5 2323259-semver-core-translation-file-5.patch6.43 KBgábor hojtsy
#5 interdiff.txt3.5 KBgábor hojtsy
#3 2323259-semver-core-translation-file-3.patch2.92 KBgábor hojtsy
#3 interdiff.txt1.56 KBgábor hojtsy
#1 2323259-semver-core-translation-file.patch1.37 KBgábor hojtsy

Comments

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new 2323259-semver-core-translation-file.patch1.37 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2323259-semver-core-translation-file.patch, failed testing.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.56 KB
new 2323259-semver-core-translation-file-3.patch2.92 KB

So we do have tests for this :)

Status: Needs review » Needs work

The last submitted patch, 3: 2323259-semver-core-translation-file-3.patch, failed testing.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt3.5 KB
new 2323259-semver-core-translation-file-5.patch6.43 KB

Fixing the translation based tests, so they have a local file. Now that the new pattern does not match remote downloads. Fix still needed to cover more possible version formats instead of just this rigid number format. Next up. But this one passes locally.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes
StatusFileSize
new interdiff.txt2.59 KB
new 2323259-semver-core-translation-file-6.patch6.42 KB

This now supports files that have suffixes and updates the test to support them.

The last submitted patch, 5: 2323259-semver-core-translation-file-5.patch, failed testing.

Gábor Hojtsy queued 5: 2323259-semver-core-translation-file-5.patch for re-testing.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Priority: Normal » Major

If Drupal would know its version number this would be critical because in fact translation downloads don't work anymore without this patch. This is the pre-patch regex:

'!drupal-\d+\.[^\.]+\.' . (!empty($langcode) ? preg_quote($langcode, '!') : '[^\.]+') . '\.po$!'

This 'accidentally' worked for versions like drupal-8.0-alpha13 because '0-alpha13' matched the sequence of non-dots at the end of version number. However, once the version number is drupal-8.0.0-alpha14 as is now, this will not match. The result being the translations ARE downloaded, but then they are not loaded back from locally because they are not recognized as valid translation files. So you get the translation screen again.

This is why in #3 this was failing on two installer tests. In #5 I fixed this by providing local files (that matched the rigid number scheme at the time). In #6 I fixed the regex so it matches extra version elements like alpha/beta and expanded tests.

So practically the only reason this is not critical is because Drupal cannot identify its version number at this time properly and falls back on alpha12 (lack of git deploy and lack of concrete version number in DRUPAL_VERSION). Not something we can fix, so we need to cope with that.

vijaycs85’s picture

vijaycs85
Location London, UK
commented

Over all, looks good. just one comment is that covering the version number with various correct/incorrect values.

  1. +++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.php
@@ -75,7 +75,7 @@ protected function getLanguage($langcode) {
+    $files = file_scan_directory($this->directory, '!drupal-[0-9a-z\.-]+\.' . (!empty($langcode) ? preg_quote($langcode, '!') : '[^\.]+') . '\.po$!', array('recurse' => FALSE));

    Would be great to get this unit test covered with various values by @dataProvider method. 

  2. +++ b/core/modules/system/src/Tests/Installer/InstallerLanguageDirectionTest.php
@@ -27,17 +27,16 @@ class InstallerLanguageDirectionTest extends InstallerTestBase {
-    // @todo Instead of actually downloading random translations that cannot be
...
+    $this->assertEqual((string) current($elements), 'Save and continue Arabic');

    Nice improvement by providing our own translation instead of 'notEqual' check. 

  3. +++ b/core/modules/system/src/Tests/Installer/InstallerTranslationTest.php
@@ -28,17 +28,16 @@ class InstallerTranslationTest extends InstallerTestBase {
+    // Place a custom local translation in the translations directory.
+    mkdir(DRUPAL_ROOT . '/' . $this->siteDirectory . '/files/translations', 0777, TRUE);
+    file_put_contents(DRUPAL_ROOT . '/' . $this->siteDirectory . '/files/translations/drupal-8.0.0.de.po', "msgid \"\"\nmsgstr \"\"\nmsgid \"Save and continue\"\nmsgstr \"Save and continue German\"");
+
...
-    // @todo Instead of actually downloading random translations that cannot be

    Nice to see this todo removed :)

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Yeah the InstallerLanguageTest could probably be redone as a unit test but I only wanted to fix the bugs instead of trying to refactor it. It is true that we could include some .po files with wrong names and prove they are not found. That's a good one. If you can help refactor that as a unit test, that is definitely welcome, I'm not planning :)

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented 
+++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.php
@@ -75,7 +75,7 @@ protected function getLanguage($langcode) {
diff --git a/core/modules/simpletest/files/translations/drupal-8.0.0-beta2.hu.po b/core/modules/simpletest/files/translations/drupal-8.0.0-beta2.hu.po

diff --git a/core/modules/simpletest/files/translations/drupal-8.0.0-beta2.hu.po b/core/modules/simpletest/files/translations/drupal-8.0.0-beta2.hu.po
new file mode 100644

new file mode 100644
index 0000000..e69de29

index 0000000..e69de29
diff --git a/core/modules/simpletest/files/translations/drupal-8.0.0.de.po b/core/modules/simpletest/files/translations/drupal-8.0.0.de.po

diff --git a/core/modules/simpletest/files/translations/drupal-8.0.0.de.po b/core/modules/simpletest/files/translations/drupal-8.0.0.de.po
new file mode 100644

new file mode 100644
index 0000000..e69de29

index 0000000..e69de29
diff --git a/core/modules/simpletest/files/translations/drupal-8.0.de.po b/core/modules/simpletest/files/translations/drupal-8.0.de.po

diff --git a/core/modules/simpletest/files/translations/drupal-8.0.de.po b/core/modules/simpletest/files/translations/drupal-8.0.de.po
deleted file mode 100644

deleted file mode 100644
index e69de29..0000000

index e69de29..0000000
diff --git a/core/modules/simpletest/files/translations/drupal-8.0.hu.po b/core/modules/simpletest/files/translations/drupal-8.0.hu.po

diff --git a/core/modules/simpletest/files/translations/drupal-8.0.hu.po b/core/modules/simpletest/files/translations/drupal-8.0.hu.po
deleted file mode 100644

deleted file mode 100644
index e69de29..0000000

Is this on purpose?

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Yeah its just renamed files, but my git diff is not properly configured to show they are renamed. While the pattern used would support a .po file like drupal-8.0.hu.po, it should be testing more realistic versions like drupal-8.0.0-beta2.hu.po.

vijaycs85’s picture

vijaycs85
Location London, UK
commented

leave the rename part with me. I'm working on unit testing and I can see the rename part properly in my local:

diff --git a/core/modules/simpletest/files/translations/drupal-8.0.de.po b/core/modules/simpletest/files/translations/drupal-8.0.0-beta2.hu.po
similarity index 100%
rename from core/modules/simpletest/files/translations/drupal-8.0.de.po
rename to core/modules/simpletest/files/translations/drupal-8.0.0-beta2.hu.po
diff --git a/core/modules/simpletest/files/translations/drupal-8.0.hu.po b/core/modules/simpletest/files/translations/drupal-8.0.0.de.po
similarity index 100%
rename from core/modules/simpletest/files/translations/drupal-8.0.hu.po
rename to core/modules/simpletest/files/translations/drupal-8.0.0.de.po
diff --git a/core/modules/system/src/Tests/Installer/InstallerLanguageDirectionTest.php b/core/modules/system/src/Tests/Installer/InstallerLanguageDirectionTest.php
index 74b8be2..cd7bce5 100644
--- a/core/modules/system/src/Tests/Installer/InstallerLanguageDirectionTest.php
+++ b/core/modules/system/src/Tests/Installer/InstallerLanguageDirectionTest.php
vijaycs85’s picture

vijaycs85
Location London, UK
commented
StatusFileSize
new 2323259-translation-file-version-15.patch9.59 KB
new 2323259-diff-6-15.txt4.47 KB

Updating with unit tests..

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Needs work

I love those unit tests!

  1. +++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.php
@@ -75,11 +75,25 @@ protected function getLanguage($langcode) {
+    return '!drupal-[0-9a-z\.-]+\.' . (!empty($langcode) ? preg_quote($langcode, '!') : '[^\.]+') . '\.po$!';

    I would love a comment here so we know what the regex+ternary are doing three months from now. 

  2. +++ b/core/modules/system/src/Tests/Installer/InstallTranslationFilePatternTest.php
@@ -0,0 +1,81 @@
+ * @file
+ * Definition of Drupal\system\Tests\Installer\InstallTranslationFilePatternTest.

    Contains. 

  3. +++ b/core/modules/system/src/Tests/Installer/InstallTranslationFilePatternTest.php
@@ -0,0 +1,81 @@
+  /**
+   * @dataProvider providerValidTranslationFiles
+   */
+  public function testFilesPatternValid($langcode, $filename) {
+    $pattern = $this->filePatternMethod->invoke($this->fileTranslation, $langcode);
+    $this->assertNotEmpty(preg_match($pattern, $filename));
+  }

    That's very clever, vijaycs85++ 

  4. +++ b/core/modules/system/src/Tests/Installer/InstallTranslationFilePatternTest.php
@@ -0,0 +1,81 @@
+  public function providerValidTranslationFiles() {
+    return array(
+      array('hu', 'drupal-8.0.0-alpha1.hu.po'),
+      array('ta', 'drupal-8.10.10-beta12.ta.po'),
+      array('hi', 'drupal-8.0.0.hi.po'),
+    );

    AFAIK, we support de.po. Can we add this case?

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

@penyaskito: yeah, we don't support de.po, look at the regex you quoted in your first point :)

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

As exporting translation use the "hu.po" pattern, should we allow it on importing?

sutharsan’s picture

sutharsan commented
Status: Needs work » Needs review
StatusFileSize
new 2323259-translation-file-version-19.patch7.32 KB
new interdiff-2323259-15-19.txt7.09 KB

Included #16 comments (1 and 2).

@penyaskito, this code is only ment to be used at installation:

 * Translates a string when some systems are not available.
 *
 * Used during the install process, when database, theme, and localization
 * system is possibly not yet available.

During installation we only have drupal-core, that's where the "drupal-" prefix comes from. Do not mistake this with importing translation files by the admin via the interface. There any file name is permitted.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

@Sutharsan: the interdiff seems to be as big as the diff?! Hard to figure out what changed.

Status: Needs review » Needs work

The last submitted patch, 19: 2323259-translation-file-version-19.patch, failed testing.

sutharsan’s picture

sutharsan commented
StatusFileSize
new interdiff-2323259-15-19.txt1.88 KB

This is the correct interdiff.

sutharsan’s picture

sutharsan commented
Status: Needs work » Needs review
StatusFileSize
new 2323259-translation-file-version-23.patch10.08 KB

And this is the correct patch. I missed the files in simplenews/test/files/translations.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented 
+++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.php
@@ -60,8 +60,8 @@ protected function getLanguage($langcode) {
-   *  - 'drupal-[number].*.[langcode].po
-   *  - 'drupal-[number].*.*.po
+   *  - 'drupal-*.[langcode].po (if langcode is provided)
+   *  - 'drupal-*.*.po (if no langcode is provided)

I don't get the change of not requiring a number after drupal- because later on in the code we require the number to the be the same as the major number of the local install no? Is that not the case? If we don't ensure there is a number there, then there may be false positives.

sutharsan’s picture

sutharsan commented

Not sure either, was introduced in #6.

vijaycs85’s picture

vijaycs85
Location London, UK
commented 
+++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.php
@@ -75,11 +75,28 @@ protected function getLanguage($langcode) {
+    return '!drupal-[0-9a-z\.-]+\.' . (!empty($langcode) ? preg_quote($langcode, '!') : '[^\.]+') . '\.po$!';

As per regex, it is either number or string or dot(.) or hyphen (-)to represent release version. so, I guess the comment can be updated to:

+   *  - 'drupal-[version].[langcode].po (if langcode is provided)
+   *  - 'drupal-[version].po (if no langcode is provided)
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Needs work

Ok, lol, I made the version number change in #6 :D So looking at how we use this, we don't care for the version number for these files, so it does not actually matter. We are fine, the regex is good. The docs improvements would be nice from #26 :) Otherwise this looks good IMHO.

sutharsan’s picture

sutharsan commented
Status: Needs work » Needs review
StatusFileSize
new 2323259-translation-file-version-28.patch10.1 KB
new interdiff-2323259-28-23.txt1019 bytes

Changed documentation.
I guess this is what @vijaycs85 meant. In #26 the language wildcard is missing when no langcode is provided.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Excellent work!

Committed and pushed to 8.x. Thanks!

  • webchick committed 53b7274 on 8.0.x 
    Issue #2323259 by Sutharsan, vijaycs85, Gábor Hojtsy: Fixed Local...
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: -sprint

Thanks!

Status: Fixed » Closed (fixed)

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