Installing a module can delete config translations

+++ b/core/modules/locale/src/LocaleConfigManager.php
@@ -173,7 +173,15 @@ protected function getTranslatableData(TypedDataInterface $element) {
+        // Something is only translatable by Locale if there is a string in the
+        // first place.
+        $value = $element->getValue();
+        if ($value === '' || $value === NULL) {
+          return NULL;
+        }
+        else {
+          return new TranslatableMarkup($element->getValue(), array(), $options);
+        }

The return value is an array or a TranslatableMarkup, we should not return NULL. It does not matter if an element was not transltabale or was but was empty for this processing AFAIS. We should just return the empty array as we do anyway at the end.

AFAIS we should just expand the !empty($definition['translatable']) with the two conditions you wanted to add for value. (Or if that sounds like a performance concern since we'd get the data for things that are not translatable, do it first thing after !empty($definition['translatable']).

That looks like the correct fix, thanks! The key not only need to exist it also needs to be a string :)

Woohoo!

+++ b/core/modules/locale/src/Tests/LocaleConfigTranslationImportTest.php
@@ -81,4 +80,44 @@ public function testConfigTranslationImport() {
+  public function testConfigTranslationModuleInstall() {
+
+    // Enable locale, block and config_translation modules.

Nit: one extraneous \n. Can be fixed on commit.

Committer: Please also credit Gábor Hojtsy — he and Alex Pott were instrumental in tracking this down.

Reviewing this.

Drupal\editor\Tests\EditorSecurityTest seems to be a total random fail on testbot. It passed on CI.

Documenting some IRC discussion of this issue:

• alexpott: xjm: so what Locale is doing on module installation is getting all the configs which are translatable and comparing them with the latest translations from l.d.o
• alexpott: xjm: if the config is the same as the default and it is translatable it will keep you in sync. It does this by checking the translations.
• xjm: alexpott: if the active config is the same as the shipped default is the same as on l.d.o, you mean?
• alexpott: xjm: to check the translations it gets the install value of the config and populates a translation wrapper. It then attempts to translate it.
• alexpott: xjm: the key part occurs in LocaleConfigManager::updateConfigTranslations()
• alexpott: xjm: in HEAD $translatable = $this->getTranslatableDefaultConfig($name); has two entries for title and slogan (for the system.site)
• alexpott: xjm: but these field are not actually translatable by locale because they have no value to translate
• alexpott: xjm: so in HEAD if (empty($translatable)) { is not TRUE so it tries to process them
• xjm: alexpott: oh, because you have to configure site name on install os there is no *default*
• xjm: and slogan is also empty by default
• alexpott: xjm: exactly - this is all caused by translatable strings with no defaults
• alexpott: xjm: so Locale can't localise that but atm it is still trying too
• alexpott: xjm: this was not easy to work out. Hence starting with a failing test
• alexpott: xjm: so if the system.site contained a properly localisable string as well we'd be fine too because the $translatable only will contain the one that does not have a empty default
• alexpott: xjm: as I say that I realise I'm not super confident of that...

Putting this one back down pending brain reboot.

+++ b/core/modules/locale/src/LocaleConfigManager.php
@@ -563,15 +563,17 @@ public function updateConfigTranslations(array $names, array $langcodes = array(
         if ($langcode != $active_langcode) {
+          $override = $this->languageManager->getLanguageConfigOverride($langcode, $name);
           // If the language code is not the same as the active storage
           // language, we should update a configuration override.
           if (!empty($processed)) {
+            $data = NestedArray::mergeDeepArray(array($override->get(), $processed), TRUE);
             // Update translation data in configuration override.
-            $this->saveTranslationOverride($name, $langcode, $processed);
+            $this->saveTranslationOverride($name, $langcode, $data);
             $count++;
           }

I had the fear that this would keep translations that are not in the active data anymore. Looks like we filter this at 2 places. In processTranslatableData(), we only take the source strings from install storage where the keys are still in active config. So $processed would only contain keys that are in active. That is not true for $override->get() necessarily. So this may end up with random keys in the merged array and possibly fatals when code attempts to use this as config. We need to filter it down to keys present in the active config.

Looking at $this->filterOverride() though, we pass over the translatable source data and in your patch here, you make the keys not appear in this array if they were empty or null, so the filter would again filter these out (not just the ones which were actually not in active config, but also the ones that were empty).

Looking at that, I think there is another issue. So $processed only contains elements now that are (a) have the key still in active config (b) translatable (c) not empty. If $processed is entirely empty then there were no such keys. Then we do $data = $this->filterOverride($override->get(), $translatable); so we in fact potentially keep all the keys that were in the default config even if they are not in active config anymore.

So its not just your patched code that has this problem but also the existing code a few lines down.

We were about to revisit this code in #2428045: Comparing active storage to install storage is problematic, install storage may change anytime but unfortunately I did not get around to working on that given all the higher priorities :/

Looks much better, thanks a lot. Just a minor nit:

+++ b/core/modules/locale/src/LocaleConfigManager.php
@@ -562,35 +562,33 @@ public function updateConfigTranslations(array $names, array $langcodes = array(
+          else if (!$override->isNew()) {

elseif

The tests do test the expected behavior. I also reviewed the new patch all at once and it looks good too (not just the interdiff). I only found some incorrect code docs:

+++ b/core/modules/locale/src/Tests/LocaleConfigTranslationImportTest.php
@@ -130,2 +131,95 @@
+    // Enable locale, block and config_translation modules.
+    $this->container->get('module_installer')->install(['locale']);
...
+    // Enable locale, block and config_translation modules.
+    $this->container->get('module_installer')->install(['locale']);

Comment incorrect.

Looks great, pending testbot feedback :)

Note that it is not very apparent on this issue, but @alexpott and I carefully deliberated the legitimacy of each condition and action in this code block and that resulted in the two additional tests for cases not covered before.

Just for paranoia's sake I sent off tests for postgresql and sqlite - our locale tests have had amusing bugs with those databases before although this is new test coverage so should not run into the same issue.

doing manual testing now.

could not reproduce on head. added more detail to steps in the summary.
(I also did a drush cr after installing book and reloading the /es home page and still seeing the translated slogan, but didn't list that in the steps, as I wasn't sure what the intended behavior was, and if we wanted that to be necessary to see the problem)

I think we should have a tests only patch to see how it fails.

Hacked the fixes out of the patch.

@alexpott pointed out there is one in #2 but there's more tests now.

with super clean (render caching on), doing everything manually (no drush at all), then it reproduces, and no need to clear the cache after enabling book to see the problem. updated steps in the summary.

and the same steps with the patch fix the problem.

btw, alexpott thought maybe drush module install and locale_modules_installed() might be broken

[edit: https://github.com/drush-ops/drush/issues/1670 ]

The tests-only patch failing is expected, so back to "needs review".

This only went to NR because catch wanted to see it pass on SQLite and Postgres too, and then Alex uploaded a test-only patch which of course failed, which is why it was NW.

On top of that, we have manual testing in #35.

Therefore, back to RTBC per #24.

I did not manually test, but just looking at the code changes in the patch, it all makes sense to me. So RTBC+1 if folks here think it's sufficiently tested.

Minor questions:

1. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -122,13 +122,14 @@ public function __construct(StorageInterface $config_storage, StringStorageInter
   -   * Gets array of translated strings for translatable configuration.
   +   * Gets array of translated strings for Locale translatable configuration.
   

   Should we also add the "Locale" qualifier to a few other code comments, such as in updateConfigTranslations() where there's a comment that says: "If there is nothing translatable in this configuration or not supported, skip it."?

2. +++ b/core/modules/locale/tests/modules/locale_test_translate/config/install/locale_test_translate.settings.yml
   @@ -0,0 +1,3 @@
   +translatable_default_with_no_translation: 'Locale can not translate'
   

   Why can Locale not translate this?

putting the same passing patch up, so that testbot rerunning while at rtbc doesnt mark it needs work (cause of the very nice tests only patch)

@effulgentsia: "Locale can not translate" means there that locale does not have a translation for it, so it will not be able to translate it, not that it could not translate it if there was a translation.

Committed/pushed to 8.0.x, thanks!

Yay, superb, thanks! Especially for the amazing work to @alexpott.

This fix was great. I also ran through the same steps as in the issue summary, but for a nested config element in a config entity: specifically, the "Caption" element of the style settings of a table View (in my case, I tried it with the /admin/content View). And that had the same problem prior to this commit and fixed by it. In the process though, I discovered #2581399: views.style.table schema has incorrect label for 'description'.