English config source strings are saved as custom translations in locale in foreign install

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();
       }

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.

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.

@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? :)

This is a fallout of #2212069: Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated.

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: 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.

- 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:

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.

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.

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 :)

Added this to that said distro: http://cgit.drupalcode.org/multilingual_demo/commit/?id=eac51fb0d07cfdc1...

Superb, thanks!