Problem/Motivation

Originally reported by @eiriksm at #2457551-23: Regression: optional default configuration is not translatable anymore in locale.

When installing Drupal in a foreign language, several English config source strings are saved as custom interface translations to locale and make it impossible to get overwritten from community translations. When installing in Hungarian for example, the following English source strings end up in locale as custom translations from LocaleConfigSubscriber::saveCustomizedTranslation:

Recent comments
Master
No comments available.
Block
Files
Filter
Reset
Sort by
Items per page
- All -
Offset
Name
Size
Status
Used in
Filename
Entity
Entity type
Use count
All
Adding languages
Language code
Language name
Language
Languages
Translation
User interface translation
Translation language
Search
Translate
Content
Apply
Page
Find and manage content.
Title
Author
Updated
Operations
Published status
Published
Type
No content available.
Recent content
Edit
Delete
More
Feed
Last update
Attachment
Taxonomy term
Users
People
Bulk update
Username
Roles
Member for
Last access
Role
Permission
Active
Blocked
No people available.
more
List
Manage user accounts, roles, and permissions.
User
Output format
Fields
Preview

Proposed resolution

Do not save new translation if its the same as source string. This situation arose now that we store translations in active config, so the values in active config may be the source or the translation and we need to tell those apart. We should only save the same string as the source as a translation if it was changed back.

Remaining tasks

Review. Commit.

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#22 interdiff-17-22.txt394 byteseiriksm
#22 english_config_source-2468767-22.patch6.88 KBeiriksm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,123 pass(es). View
#18 2468767-18-test-only.patch3.31 KBeiriksm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,388 pass(es), 2 fail(s), and 0 exception(s). View
#17 2468767-test-only.patch2.92 KBeiriksm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,386 pass(es). View
#17 2468767-17.patch6.5 KBeiriksm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,377 pass(es). View
#16 after enabling module with user related strings.png29.99 KBeiriksm
#16 before enabling module with user related strings.png29.75 KBeiriksm
#14 2468767-14.patch6.7 KBeiriksm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,584 pass(es), 2 fail(s), and 0 exception(s). View
#14 Screen Shot 2015-04-14 at 07.45.14.png26.92 KBeiriksm
#12 2468767-12-test-only.patch3.48 KBeiriksm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,542 pass(es). View
#6 2468767-6.patch3.58 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,355 pass(es), 1 fail(s), and 0 exception(s). View
#6 interdiff.txt2.29 KBGábor Hojtsy
#3 2468767.patch1.54 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,361 pass(es). View

Comments

Gábor Hojtsy’s picture

My fist thought was this would be due to non-translated strings but this fix did not work and in fact exposed that the 69 affected strings are already translated in locale when they get rewritten with their English values. So what's happening here is that the locale data is not propagated immediately to config. Continuing the debugging.

diff --git a/core/modules/locale/src/LocaleConfigSubscriber.php b/core/modules/locale/src/LocaleConfigSubscriber.php
index ee20daf..02c0027 100644
--- a/core/modules/locale/src/LocaleConfigSubscriber.php
+++ b/core/modules/locale/src/LocaleConfigSubscriber.php
@@ -204,12 +204,12 @@ protected function resetExistingTranslations($name, $translatable, $reference_co
    *   The source string value.
    * @param string $context
    *   The source string context.
-   * @param string $translation
+   * @param string $new_translation
    *   The translation string.
    * @param string $langcode
    *   The language code of the translation.
    */
-  protected function saveCustomizedTranslation($name, $source, $context, $translation, $langcode) {
+  protected function saveCustomizedTranslation($name, $source, $context, $new_translation, $langcode) {
     $locale_translation = $this->localeConfigManager->getStringTranslation($name, $langcode, $source, $context);
     if (!empty($locale_translation)) {
       // Save this translation as custom if it was a new translation and not the
@@ -217,10 +217,11 @@ protected function saveCustomizedTranslation($name, $source, $context, $translat
       // source). Or if there was an existing translation and the user changed
       // it (even if it was changed back to the original value). Otherwise the
       // translation file would be overwritten with the locale copy again later.
-      if (($locale_translation->isNew() && $source != $translation) ||
-          (!$locale_translation->isNew() && $translation != $locale_translation->getString())) {
+      $existing_translation = $locale_translation->getString();
+      if (($locale_translation->isNew() && $source != $new_translation) ||
+          (!$locale_translation->isNew() && ((empty($existing_translation) && $source != $new_translation) || ((!empty($existing_translation) && $new_translation != $existing_translation))))) {
         $locale_translation
-          ->setString($translation)
+          ->setString($new_translation)
           ->setCustomized(TRUE)
           ->save();
       }
Gábor Hojtsy’s picture

So the problem is with SiteConfigureForm::submitForm()'s side effect. That installs the update.module at a point where locale is already installed and LocaleConfigSubscriber is loaded. Prior to that, locale module is installed last right before the configure form (see install_profile_info()) so this does not happen earlier.

Gábor Hojtsy’s picture

Issue tags: +Needs tests
FileSize
1.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,361 pass(es). View

So a possible fix is for LocaleConfigSubscriber to not respond to events in the installer. This may be a bit too drastic if profiles save some kind of translations to config files, but that sounds like an edge case, and they can always do the save via locale instead of config directly. Needs tests of course.

Gábor Hojtsy’s picture

Status: Active » Needs review
eiriksm’s picture

This does indeed fix the problem.

Just as a note for others (and a note generally):

Seemed as if I installed Drupal with the locale file pre-downloaded, this bug does not occur (so for example if I tried to install Drupal and then just delete all tables and reinstall, the bug does not appear). Will do a double check on that.

Just let me know if you want to delegate test writing :)

Gábor Hojtsy’s picture

FileSize
2.29 KB
3.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,355 pass(es), 1 fail(s), and 0 exception(s). View

@eiriksm: tests would be great. I think this is still a problem if module A comes with some default config that it modifies in its hook_install() (assuming hook_install() runs after config import), the fix in #1 would be suitable for that, so I think we should bundle up the two fixes in this issue. Here is an updated patch that includes that too plus a little code comment update.

It definitely does not help if the given source string was not available as a community translation and it was modified but not translated. We cannot distinguish between those. But at least this fix catches the case, when the source was saved before but the target is not yet available and we don't save the same string as the target, so this bug will not occur later on after install either. Wanna take a crack at the tests? :)

Gábor Hojtsy’s picture

BTW the reason install and runtime need very different fixes is normally when a .po file is imported (automated or manually) or locale translation changes, there is an immediate config update following. In install, we import the .po file THEN display the site configuration form and then update the config, so there are some requests in-between that are affected. (Sidenote: We don't necessarily need to do it in these places, but its more practical to update config in one pass later because config may change with profile hooks. We can look into importing the .po files later in the same pass as config updates, but that is a different approach).

The runtime fix needs to be different because that is for yet nonexistent community translations that are then still empty, and we should not pollute locale with fake English translations there either.

eiriksm’s picture

New patch looks good. Tried to break it with the couple of cases I could think of, but still worked good :)

Not sure where to add the test though? LocaleConfigSubscriberTest? I would have to monkey patch $GLOBAL to get the one if to take effect it seems.

Gábor Hojtsy’s picture

@eiriksm: For the installer problem, I think a test like InstallerTranslationMultipleLanguageTest may be modified to add further assertions based on strings in this bug. For the runtime use case LocaleConfigSubscriberTest looks like a sane option yup. Save a config file and check if English sourced ended up in locale. Please post the test-only patch to demonstrate that both fails are reproduced by the test.

Status: Needs review » Needs work

The last submitted patch, 6: 2468767-6.patch, failed testing.

eiriksm’s picture

FileSize
3.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,542 pass(es). View

OK, so I ended up writing both tests in the installer, just for convenience really. I could not for the life of me trigger the bug in LocaleConfigSubscriberTest, I guess I would have to add a new test module for that actually.

As it also turns out, there is also another bug not fixed in the patch, reproduced as following:
- Install minimal install profile in another language (norwegian is what i use).
- After the installation, enable a module that has configuration with string already present. For example tour module.
- Strings are now overwritten again. For example the string "Translate" or "Language name" or... well, it is easy to spot.

So, a couple of things to note:

- The installer bug only triggers if you install in another language (obviously). So I added the checks to the "de" installation that we are testing already.
- To trigger the install related bug, one has to import a module with a config string that has already been translated. None of the modules in the testing_multilingual profile end up doing that, so I added the tour module into the mix. Might seem irrelevant, but that was the most transparent method I found to provoke the bug.
- The tests only fail if the patch in #2457551: Regression: optional default configuration is not translatable anymore in locale is applied.
- The tests still fail with the patch in this issue applied in addition to that patch, because of the additional bug mentioned above.

Since I am calling it a day now, I'll just leave the tests here for now.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

- Install minimal install profile in another language (norwegian is what i use).
- After the installation, enable a module that has configuration with string already present. For example tour module.
- Strings are now overwritten again. For example the string "Translate" or "Language name" or... well, it is easy to spot.

Not sure how is this possible to detect? The only way I can see to resolve this is to do the langcode change in the active config later in the config update batch if locale is already installed and only keep the langcode change in locale.module for the case when locale itself is installed, so previous ones are updated. Do you think that would make sense?

eiriksm’s picture

Issue summary: View changes
FileSize
26.92 KB
6.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,584 pass(es), 2 fail(s), and 0 exception(s). View

@Gábor Hojtsy Of course, I do not know the reasoning and the development of this system like you do. And you probably have a better understanding of how we would end up in this function, but bear with me for a moment.

This is what we want, right:

      // Save this translation as custom if it was a new translation and not the
      // same as the source. (The interface prefills translation values with the
      // source). Or if there was an existing (non-empty) translation and the
      // user changed it (even if it was changed back to the original value).
      // Otherwise the translation file would be overwritten with the locale
      // copy again later.
      if (($locale_translation->isNew() && $source != $new_translation) ||
        (!$locale_translation->isNew() && ((empty($existing_translation) && $source != $new_translation) || ((!empty($existing_translation) && $new_translation != $existing_translation))))) {

Here is a case (attached) where
- The locale translation is not new.
- The existing translation is not empty.
- The new translation if not the existing translation.
- The new translation is the same as the source.

This way, the contributed translation for "Master" ("Hoved") gets overwritten by the "new translation" (not really a translation) "Master".

Is there really a case where we want that to happen? Can't we just add that as a check as well?

Sorry if I am ignoring something obvious here, but it makes more sense to me this way.

Patch attached. This one passes the added tests in #12 also (tests are also included). The patch in #6 does not pass the tests (the test in question that fails is InstallerTranslationMultipleLanguageForeignTest). As said earlier, the tests only "applies" if the patch in #2457551: Regression: optional default configuration is not translatable anymore in locale is applied. Woah that last sentence was a strange one :)

Status: Needs review » Needs work

The last submitted patch, 14: 2468767-14.patch, failed testing.

eiriksm’s picture

Hm. OK then. Seems this is deliberate. Reading through issues and code, I see that this seems well thought through, so there is obviously a case I am missing here. I still don't understand why we just say that (shortened for clarity):

(!$locale_translation->isNew() && (!empty($existing_translation) && $new_translation != $existing_translation))

Also. This means that no matter how pleased your are with the state of your translated interface, some module can just be enabled and take over your interface.

To test this, I enabled a module with the words "Password" "Username" "Create new account" and so on, to see what happened to the log in form in Norwegian. All texts are now in english :( Which in turn means (sorry for being dramatic), that a module could come along with a config file containing all known user facing text, and completely wipe out all your downloaded translations. More realistically, a lot of the short strings you have translated, could at any point be overwritten by enabling a module at least.

But I am starting to think that it is out of scope of this issue, maybe?

eiriksm’s picture

Status: Needs work » Needs review
FileSize
6.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,377 pass(es). View
2.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,386 pass(es). View

Revisiting this after #2457551: Regression: optional default configuration is not translatable anymore in locale got committed.

Here is a patch that combines the fix in #6 with a test. Still find it a bit unfortunate what I point out in #16, but that is maybe out of scope.

Attached test-only and full patch.

eiriksm’s picture

FileSize
3.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,388 pass(es), 2 fail(s), and 0 exception(s). View

Hah, that test-only was meant to fail of course.

Status: Needs review » Needs work

The last submitted patch, 18: 2468767-18-test-only.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: 2468767-18-test-only.patch, failed testing.

eiriksm’s picture

Status: Needs work » Needs review
FileSize
6.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,123 pass(es). View
394 bytes

Test only failed now, updated patch including my previous mistake.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@eiriksm:

If the translation is not yet saved, we only save it if different from source (it falls back on the source automatically anyway):

if (($locale_translation->isNew() && $source != $new_translation) ||

If the translation is not new but empty (an empty translation was saved earlier), we only save if different from source (otherwise falls back on source):

(!$locale_translation->isNew() && ((empty($existing_translation) && $source != $new_translation)

If the translation is not empty, we only save it if the new translation is different from the prior one:

|| ((!empty($existing_translation) && $new_translation != $existing_translation)))))

Your change in this later one to not allow changing the translation back to the source string IMHO is incorrect. We cannot say that in 100% of the cases a translation should be different from its source. It should be possible to change a translation back to its source later on. The earlier logic items use the assumption that if the string was empty, it falls back on source (see LocaleLookup::resolveCacheMiss()), which is why it does not save a new translation as same as the source. However, it should be able to set a protected custom translation even to the source value, which is why we don't have the same check later. I don't think your changed logic is correct.

Gábor Hojtsy’s picture

Also. This means that no matter how pleased your are with the state of your translated interface, some module can just be enabled and take over your interface.

To test this, I enabled a module with the words "Password" "Username" "Create new account" and so on, to see what happened to the log in form in Norwegian. All texts are now in english :( Which in turn means (sorry for being dramatic), that a module could come along with a config file containing all known user facing text, and completely wipe out all your downloaded translations. More realistically, a lot of the short strings you have translated, could at any point be overwritten by enabling a module at least.

Right, so in locale, everything may be changed with updates in translations (eg. from localize.drupal.org). If you want to protect translations, you can mark them as customized, in which case they should not be overwritten in locale. When config (translation) is changed, even protected translations will be modified of course because you are the initiator of the action. In locale, strings are shared between modules so you only translate things once. This includes default configuration. I don't think that discussion is in scope for this issue.

eiriksm’s picture

Status: Needs work » Needs review

I agree with both your comments. Especially the last one, about not being in the scope of this issue. :)

And I see your point where you disagree. I must admit I am not quite sure what the expected and best way to handle this is. But I am pretty sure about this:

These cases are:
- Probably edge cases
- Very complex to solve in the best possible way that handles all cases.

So I think my last point was. Let's solve this particular issue (which #6 does).

Setting back to needs review. The only difference between your patch in #6 and #22 is the tests. (edit: meaning it does not include the aforementioned changed logic)

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: -Needs tests

All right. The tests you provided make sense to me :) @penyaskito is looking at this issue I believe, hopefully he an RTBC it if all looks good then :)

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Suffered this problem with beta10 and an installation profile in a non-English language.
After applying the #22 patch to beta10 (it still applies), the issue was fixed.
Thanks!

Gábor Hojtsy’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed b7d1db5 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/system/src/Tests/Installer/InstallerTranslationMultipleLanguageTest.php b/core/modules/system/src/Tests/Installer/InstallerTranslationMultipleLanguageTest.php
index a7051c7..53538b1 100644
--- a/core/modules/system/src/Tests/Installer/InstallerTranslationMultipleLanguageTest.php
+++ b/core/modules/system/src/Tests/Installer/InstallerTranslationMultipleLanguageTest.php
@@ -119,7 +119,7 @@ public function testTranslationsLoaded() {
         $this->assertEqual($override_en->get('anonymous'), 'Anonymous');
       }
 
-      // Activate a module, to make sure that config is not overriden by module
+      // Activate a module, to make sure that config is not overridden by module
       // installation.
       $edit = array(
         'modules[Core][views][enable]' => TRUE,

Fixed spelling mistake on commit.

  • alexpott committed b7d1db5 on 8.0.x
    Issue #2468767 by eiriksm, Gábor Hojtsy: English config source strings...
Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks!

Status: Fixed » Closed (fixed)

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