\Drupal\locale\LocaleConfigManager::updateConfigTranslations() should not remove non-existing EN translations

I can see how for config that was shipped in English but then turned to be a foreign language due to how the site was set up, yet you still want an English variant, you don't want that English variant to disappear just because it may be the same as the original shipped config. So I think I understand the problem.

But! While localize.drupal.org does not provide English translations, it is entirely feasible in a Drupal deployment scenario that people would deploy English translations for stuff with .po files, that is at least an existing core feature. This change would not be backwards compatible with the capabilities offered currently. Is there a way to target the bug fix better at when the active config is not English or would not fall back on the string we expect it would fall back on, we don't remove that from the config.

I am a bit uneasy about this patch because it sounds like we keep the values in place for this process. But then when you save new locale strings for example, is it also kept then. The config and locale translations are supposed to be continually kept in sync and this changes that in one place. So the process can still come around and hurt you elsewhere. I don't know exactly where yet, but I think we need to consider more angles.

Hm, where do we ensure that we don't save back translations to overrides for keys that don't exist anymore in the active config? Let's say you remove a display from a view, the config gets saved so locale jumps in to update its translations. If the English translations would still be kept in the override for the config, even though the view display is not anymore in place, that is a problem. When the English translation is requested the phantom items will get array merged on top of it and you would get a broken view config loaded with stray keys from a display that does not exist anymore.

+++ b/core/modules/locale/src/LocaleConfigManager.php
@@ -584,7 +584,7 @@ public function updateConfigTranslations(array $names, array $langcodes = []) {
           // Filter out locale managed configuration keys so that translations
           // removed from Locale will be reflected in the config override.
-          $data = $this->filterOverride($override->get(), $translatable);
+          $data = $langcode != 'en' ? $this->filterOverride($override->get(), $translatable) : $override->get();

Some more commentary would be helpful here.

I think @Gábor Hojtsy's point in #11 is valid - what happens if the old keys have an effect like in the example given. If there no way to detect if old keys

really should be removed

OTOH I also agree that the data loss on module install is as serious so I'm +1 on calling this critical.

Thanks @berdir for a fix for this long standing issue.

I basically only work with non-english Drupal installation. When ever I installed a module, tons of translations in views and other config files got reverted to english.
This patch finally fixes this issue. I'm happy that from now on I only have to care about "changed" config files and not manually have to revert overridden config files.

Thanks a lot!

Rerolled #4 after #3079330: LocaleConfigSubscriberTest has many assertions that don't run.

Seems like the above patch failed, because after #3079330: LocaleConfigSubscriberTest has many assertions that don't run in \Drupal\Tests\locale\Kernel\LocaleConfigSubscriberTest::assertNoConfigOverride the second assert actually runs.

This patch then fails in $this->deleteLocaleTranslationData($config_name, 'test', 'English test', 'en'); which also calls $this->assertNoConfigOverride() (side note: I guess calling testEnglish() after deleteLocaleTranslationData() in \Drupal\Tests\locale\Kernel\LocaleConfigSubscriberForeignTest::testEnglish is redundant?).

To add to the confusion the arguments passed to \Drupal\Tests\locale\Kernel\LocaleConfigSubscriberTest::assertNoConfigOverride are wrong in all the places where called except in the test we are changing (\Drupal\Tests\locale\Kernel\LocaleConfigSubscriberForeignTest::testEnglish).

+++ b/core/modules/locale/src/LocaleConfigManager.php
@@ -584,7 +584,7 @@ public function updateConfigTranslations(array $names, array $langcodes = []) {
           // Filter out locale managed configuration keys so that translations
           // removed from Locale will be reflected in the config override.
-          $data = $this->filterOverride($override->get(), $translatable);
+          $data = $langcode != 'en' ? $this->filterOverride($override->get(), $translatable) : $override->get();

Also comment should be extended about why 'en' used... some sites can have no such langcode

Created an issue for the wrong arguments mentioned in #18: #3126195: Fix LocaleConfigSubscriberTest::assertNoConfigOverride method

I am trying with the patch from #3126195: Fix LocaleConfigSubscriberTest::assertNoConfigOverride method. Test were still failing locally though. So I replaced the deleteLocaleTranslationData() call with what that method does minus the ending assert. Because if I correctly understand this patch, wouldn't we then expect the en translation not to be deleted? I might be wrong though, because I am not that familiar with the issue.

I also added the comment mentioned in #19. I used #7 as reference.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.