Problem/Motivation

Configuration translations are not imported if Interface translation module is enables after adding a language.

Steps to reproduce:

  1. Install in English.
  2. Enable Language module.
  3. Add a language. Enable Locale module.
  4. Go to report > Translation updates and run a translation update.
  5. Observe: Configuration translations are not updated.

Configuration translation is imported when:

  1. Install in English.
  2. Enable Language and Locale modules at once.
  3. Add a language.
  4. Observe: The batch that runs downloads translations and updates configuration translations.

Proposed resolution

Update configuration translations as well from the localization update process.

Remaining tasks

  • Review. Commit.
  • Add tests

User interface changes

The batch will run longer.

API changes

None.

CommentFileSizeAuthor
#30 2344967-config-translation-update-30.patch3.9 KBGábor Hojtsy
#30 interdiff.txt1.15 KBGábor Hojtsy
#28 2344967-config-translation-update-27.patch3.91 KBGábor Hojtsy
#25 interdiff.txt740 bytesGábor Hojtsy
#25 2344967-config-translation-update-25.patch3.9 KBGábor Hojtsy
#23 interdiff.txt1.73 KBGábor Hojtsy
#23 2344967-config-translation-update-23.patch3.9 KBGábor Hojtsy
#18 2344967-config-translation-update-18.patch3.92 KBSutharsan
#18 interdiff-2344967-16-18.txt975 bytesSutharsan
#16 2344967-config-translation-update-15.patch3.92 KBSutharsan
#13 2344967-config-translation-update-12.patch286.42 KBSutharsan
#13 interdiff-2344967-10-12.txt908 bytesSutharsan
#1 2344967-1.patch1.2 KBGábor Hojtsy
#10 2344967-config-translation-update-10-must-fail.patch5.13 KBSutharsan
#10 2344967-config-translation-update-10.patch3.92 KBSutharsan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
2344967-1.patch1.2 KB

This should fix it.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: +D8MIAMS

This is needed for our Lab. Tagging D8MIAMS.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented
Status: Needs review » Reviewed & tested by the community

Code looks good and fixes the problem.

Désiré’s picture

Désiré CreditAttribution: Désiré commented

I tested it twice (patch #1): once with a minimal, once with a standard install profile.
First I enabled language and locale module and added a language, dumped the language related config from the database.
Then I enabled only language module, added a language and then enabled locale module and imported the translations, dumped the language related config from the database.

As I experienced the language related config were the same on both cases, so I assume that the patch works well.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This needs tests, but I can see writing them would be difficult. OTOH it would be good to get this fixed prior to beta. Therefore, I think I'm going to let this in, but leave it needs work for tests. If folks could please follow back up with that during/after Amsterdam, that would be really awesome. This is definitely something we don't want to break a second time. :)

Committed and pushed to 8.x. Thanks!

Now back to needs work.

  • webchick committed 1848068 on 8.0.x 
    Issue #2344967 by Gábor Hojtsy: Fixed Localization update does not...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: Localization update does not update configuration translations » Tests for localization update does not update configuration translations
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: +Amsterdam2014
Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented
Assigned: Unassigned » Sutharsan
Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented
Assigned: Sutharsan » Unassigned
Status: Needs work » Needs review
FileSize
2344967-config-translation-update-10.patch3.92 KB
2344967-config-translation-update-10-must-fail.patch5.13 KB

This patch adds a test that follows the steps from the issue summary to reproduce the problem.
For the 'must-fail' patch I've added the reverse of #1 patch.

The last submitted patch, 10: 2344967-config-translation-update-10-must-fail.patch, failed testing.

lazysoundsystem’s picture

lazysoundsystem CreditAttribution: lazysoundsystem commented
Status: Needs review » Needs work

Tests look good. Just one tiny change in the comments:

+++ b/core/modules/locale/src/Tests/LocaleConfigTranslationImportTest.php
@@ -0,0 +1,86 @@
+    // Check and update the translation status. This will import the African

'Afrikaans' for 'African' here.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
interdiff-2344967-10-12.txt908 bytes
2344967-config-translation-update-12.patch286.42 KB

Thanks for the review.
Only a new patch this time, no 'test only' patch for this trivial change.

lazysoundsystem’s picture

lazysoundsystem CreditAttribution: lazysoundsystem commented
Status: Needs review » Reviewed & tested by the community

Thanks. RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2344967-config-translation-update-12.patch, failed testing.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented
Status: Needs work » Needs review
FileSize
2344967-config-translation-update-15.patch3.92 KB

Ignore the patch in #13. New patch containing the comment in #12 is included.

Status: Needs review » Needs work

The last submitted patch, 16: 2344967-config-translation-update-15.patch, failed testing.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented
Status: Needs work » Needs review
FileSize
interdiff-2344967-16-18.txt975 bytes
2344967-config-translation-update-18.patch3.92 KB

Test failing due to removal of url(). For now replaced by _url().

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented
Issue summary: View changes
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Needs work

Looks good, some minor comments:

  1. +++ b/core/modules/locale/src/Tests/LocaleConfigTranslationImportTest.php
@@ -0,0 +1,85 @@
+ * Tests translation of configuration strings.

    This is a bit too generic :) I would try to get the same 1 line text that is on the test method. 

  2. +++ b/core/modules/locale/src/Tests/LocaleConfigTranslationImportTest.php
@@ -0,0 +1,85 @@
+   * Tests configuration translation import when locale module is enabled after
+   * a language was added.

    Would be great to get this on one line, eg. "Test localization update will change configuration translations if enabled after language." (did not check if this fits on one line in fact :D) 

  3. +++ b/core/modules/locale/src/Tests/LocaleConfigTranslationImportTest.php
@@ -0,0 +1,85 @@
+    // Add translation permissions now the locale module has been enabled.

    "now that the" 

  4. +++ b/core/modules/locale/src/Tests/LocaleConfigTranslationImportTest.php
--- /dev/null
+++ b/core/modules/locale/tests/test.af.po

    There is no code apparent in this patch that makes this file be the result of update checking, where is that located? I suspect preexisting in core.

Status: Needs work » Needs review

Gábor Hojtsy queued 18: 2344967-config-translation-update-18.patch for re-testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

@Sutharsan: this seemed like was so close, are you still around to fix this one?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Reviewed & tested by the community
FileSize
2344967-config-translation-update-23.patch3.9 KB
interdiff.txt1.73 KB

The answer to #20/4 is this snippet from locale_translate_test which module is used in the test. The other minor text fixes I made myself. That should not disqualify me from RTBC-ing it since the changes are so minor.

'interface translation project': locale_test_translate
'interface translation server pattern': core/modules/locale/tests/test.%language.po

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: 2344967-config-translation-update-23.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
2344967-config-translation-update-25.patch3.9 KB
interdiff.txt740 bytes
Gábor Hojtsy’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2344967-config-translation-update-25.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Reviewed & tested by the community
FileSize
2344967-config-translation-update-27.patch3.91 KB

Minor PHP issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/locale/src/Tests/LocaleConfigTranslationImportTest.php
@@ -0,0 +1,84 @@
+    \Drupal::config('update.settings')->set('fetch.url', _url('update-test', array('absolute' => TRUE)))->save();

Is it necessary to use _url() here?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff.txt1.15 KB
2344967-config-translation-update-30.patch3.9 KB

@alexpott: the problem with the update-test path/route is it would expect some arguments, namely project and version. Then it actually ignores version later on (haha). For this test, all we need is a URL that would not return much valid info, because we only work off of local stuff here (see locale_test_translate.info.yml), so this override is for the rest of the projects so they don't interfere with the test. To fix the update-test route that requires unrelated changes, so we can just use the front page here just as well.

Better?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

I wished we had opened a follow-up for this - anyway nice to have additional test coverage for this. This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 3f28dd7 and pushed to 8.0.x. Thanks!

  • alexpott committed 3f28dd7 on 8.0.x 
    Issue #2344967 followup by Gábor Hojtsy, Sutharsan: Tests for...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -sprint

Thanks a lot!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: Tests for localization update does not update configuration translations » Localization update does not update configuration translations

Also restoring original title.

Status: Fixed » Closed (fixed)

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