Installing a module causes translations to be overwritten

Hi @kristiaanvandeneynde I basically agree with your idea #51 about keeping the default config in the language it comes - ok, let's assume English.... but also, maybe, for the future... #74

About the excelent diagram in @Gábor Hojtsy #81 just outlines the current situation / behavior, not a goal nor a solution in itself, it is just a must read to understand this issue.

So, if we can agree about keeping the default config in English, that would be a good starting point, there are a lot of pieces we have to adjust for that to happen and produce a solution:

- The initial configuration import, we should do no translation on that one. This looks like the easier one...
- The Config edit UI -which is all around the site needs to 'be aware' of that too, and should edit only the right language configuration, not the default one.
- This may require redefining how config / config_translation -which is a different module, may be enabled or not - work together.
- Then what happens with newly created config, i.e. a new field... ? How do we store that one, should we create translation right away, mark it somehow, etc.. ?

Then we have multiple site-building paths like 'what happens if' you start single language but not English, then add some more, then enable / disable config / interface translation, then change default language, etc... Whatever we do, in whatever order, it needs to produce some consistent state so my guess is that will need some harder coupling / requirements between modules - language / config translation / interface translation -
... Not that it happens now which is also a bug by itself but if we are working on this we should try to get it right at once...

So... those are a lot of things... that's why I insist we need to agree on some basic approach before wasting too much time on patches.

Here's another odd consequence of the current system: Have a site with `da` as default language, enabled a module which set `da` as langcode for all config, even though the current texts are English. OK, they're translated by locale, so things work.

Now I create a text format and export it. Which causes something to update `user.role.authenticated.yml` and `user.role.anonymous.yml` to contain Danish strings. Which makes the language code of the file more correct, but as far as I understand (someone please correct me if I'm wrong) this means that the user roles names will *not* be updated if the translation changes? I think this goes against the intention in the graph of #81. Why on earth were these updated?

Anyway, while I understand wish to make the "core" configuration english and layer translations on top, it'll have another problem: configuration without an english default. If a danish user on a danish site (with danish as the default language) creates a new role, it's obviously in danish. As it's a new role, we don't have an english string to put in the configuration.

Is a dual mode feasible? When configuration is imported on install, it's installed with `en`, but if a user saves a configuration page, it'll be assumed to be the default language, unless there's already a translation overlay?

> Why on earth were these updated?

Turns out that, when configuring a role to have access to a text format, it adds a config dependency from the role to the text format. So that explains why the role configuration was touched. But I'll still consider it a bug that it changes the untouched label from English to Danish.

I've got this bug on a multilingual site I'm working on so I spent a few hours in the debugger trying to work out why this is happening and I tracked it down to my old friend \Drupal\locale\LocaleConfigSubscriber. What was interesting is that this was being triggered by a config save in locale_system_set_config_langcodes() which is called whenever a module is installed. There's no way that the strings from configuration which is having its langcode alter here should end up in as translations. Fortunately LocaleConfigManager already has a way for us to disable the effects of the config subscriber. Once we do this on the site I'm working we're no longer losing translations and in fact more things that should be translated are!

Let's see if this fix breaks any tests.

Adding @jhodgdon's test from #12 which still fails and is great outline of the issue that #93 fixes.

The last submitted patch, 93: 2806009-93.patch, failed testing. View results →

#93's first test failed due to 🐛 Drupal\Tests\datetime\Functional\DateTimeWidgetTest fails when run at midnight Fixed - lucky me.

The last submitted patch, 94: 2806009-94.test-only.patch, failed testing. View results →

1. +++ b/core/modules/locale/locale.module
   @@ -370,28 +370,7 @@ function locale_cron() {
    function locale_system_set_config_langcodes() {
   -  // Need to rewrite some default configuration language codes if the default
   -  // site language is not English.
   -  $default_langcode = \Drupal::languageManager()->getDefaultLanguage()->getId();
   -  if ($default_langcode != 'en') {
   -    // Update active configuration copies of all prior shipped configuration if
   -    // they are still English. It is not enough to change configuration shipped
   -    // with the components just installed, because installing a component such
   -    // as views or tour module may bring in default configuration from prior
   -    // components.
   -    $names = Locale::config()->getComponentNames();
   -    foreach ($names as $name) {
   -      $config = \Drupal::configFactory()->reset($name)->getEditable($name);
   -      // Should only update if still exists in active configuration. If locale
   -      // module is enabled later, then some configuration may not exist anymore.
   -      if (!$config->isNew()) {
   -        $langcode = $config->get('langcode');
   -        if (empty($langcode) || $langcode == 'en') {
   -          $config->set('langcode', $default_langcode)->save();
   -        }
   -      }
   -    }
   -  }
   +  Locale::config()->systemSetConfigLangcodes();
    }
   

   If we commit this (and I think we should) we should open a follow-up to deprecate locale_system_set_config_langcodes() in 10.x because we no longer need it.

2. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -649,4 +649,34 @@ protected function filterOverride(array $override_data, array $translatable) {
   +  /**
   +   * Updates default configuration when new modules or themes are installed.
   +   */
   +  public function systemSetConfigLangcodes() {
   

   LocaleConfigManager has no interface so adding the method is allowed under our BC policy.

3. +++ b/core/modules/locale/src/LocaleConfigManager.php
   @@ -649,4 +649,34 @@ protected function filterOverride(array $override_data, array $translatable) {
   +    $this->isUpdatingFromLocale = TRUE;
   +    // Need to rewrite some default configuration language codes if the default
   +    // site language is not English.
   +    $default_langcode = $this->languageManager->getDefaultLanguage()->getId();
   +    if ($default_langcode != 'en') {
   +      // Update active configuration copies of all prior shipped configuration if
   +      // they are still English. It is not enough to change configuration shipped
   +      // with the components just installed, because installing a component such
   +      // as views or tour module may bring in default configuration from prior
   +      // components.
   +      $names = $this->getComponentNames();
   +      foreach ($names as $name) {
   +        $config = $this->configFactory->reset($name)->getEditable($name);
   +        // Should only update if still exists in active configuration. If locale
   +        // module is enabled later, then some configuration may not exist anymore.
   +        if (!$config->isNew()) {
   +          $langcode = $config->get('langcode');
   +          if (empty($langcode) || $langcode == 'en') {
   +            $config->set('langcode', $default_langcode)->save();
   +          }
   +        }
   +      }
   +    }
   +    $this->isUpdatingFromLocale = FALSE;
   

   There are no changes to the logic from the original function here. The "magic" is wrapping it in $this->isUpdatingFromLocale = TRUE and $this->isUpdatingFromLocale = FALSE. This prevents LocaleConfigSubscriber from making incorrect changes to the configuration.

4. +++ b/core/modules/locale/tests/src/Functional/LocaleConfigTranslationImportTest.php
   @@ -305,4 +305,66 @@ public function testLocaleRemovalAndConfigOverridePreserve() {
   +  /**
   +   * Tests setting a foreign language as default and importing configuration.
   +   */
   +  public function testConfigTranslationWithForeignLanguageDefault() {
   

   @jhodgdon thanks for the test that still works and proves the fix :)

Hi @alexpott, this looks a pretty interesting cool fix...

The part I'm missing though is that isUpdatingFromLocale = TRUE ... "Whether or not configuration translations are being updated from locale"...
... because this is not actually updating configuration translations from locale, it's a module install and we are just setting the config language to the default one... so even if it is somehow working (yes, we are preventing the locale system from attempting to automatically update strings from that or do anything else with that configuration), maybe we should revisit the concept of that variable....

Not sure, but maybe just changing the description to "Whether or not configuration updates are being triggered from locale, either for saving a configuration translation of for setting the configuration language." would do... ?

Re #90 I agree with the starting point of keeping default config in a single language and that it should be English.

What I would suggest is this:

• Config folder (e.g.: /config/sync) contains all the config that ships with core/contrib and (as already is the case) that should be in English
• You configure your site to use whatever language you want (English is also a deliberate choice)
• This creates the corresponding config folders, e.g.: /config/sync/en, /config/sync/de, etc.
• New config handling:
  • Whenever config is created via installing new code, it copies the config ONLY to the sync folder
  • Whenever config is created via the site, it puts it in the sync folder AND the language folder of the current route's language
• Config requesting:
  • Request in the active language, if none is found show the sync folder one
  • When adding a translation, it copies all values at first, but you can then modify as you see fit
  • This fixes the concern from #91 because you are free to add your own new config in Spanish or whatever. As long as distributed code (aka D.O. code) ships in English, you can do what you want.

So there is no hard requirement for personally created config in your sync folder to contain English strings. Core should therefore not contain any code that assumes the central folder has English text. But, to make everyone's life easier, core and contrib are still required to ship their config in English.

Some months ago i took a day off to find a pragmatic solution for this WTF for our team.
It ended up creating the MultiLangNG module. Which we successfully used in some projects now.

Which makes me confident to throw its hat into the future-of-core ring.

What does it do? It stops the sync magic (translations / config overrides) and does runtime config overrides instead.
This brings back the (remember D7?) contextual translations, and brings a cozy empty language override directory.

Please lets keep us focused here.

(And yes I am the first one guilty of trying to extend the scope of this thread rebuilding the whole config translation thing but...)

* If a simple solution like @alexpott #94 works and fixes this issue, we should go for that.
* I still want some better config translation @kristiaanvandeneynde but that is a complex and lengthy one, maybe for a new task.
* Loving some of the ideas in MultiLangNG module, @geek-merlin, you had it well hidden - lack of module description.... you'll see me around that one... :)

@Jose Reyero thanks for #102. I agree that completely re-structuring how config stores translations and deals with a default language other than english needs a separate issue and #101 also would need a completely separate issue.

Re #99 I'm not sure that we need to update the description for the flag. This flag is also why all the configuration changes made by calling locale_system_update() don't result in LocaleConfigSubscriber making changes.

In fact I think that we should move the call to locale_system_set_config_langcodes() inside locale_system_update() - it's not really a separate thing.

Part of the problem here is the distance between when locale_system_set_config_langcodes() runs and when the call to locale_system_update() eventual does the config update in the batch process using locale_config_batch_refresh_name(). The code in #94 reduces this gap massively for module and themes post site install but the gap is still huge during a site install. This can result in unexpected changes during site installation.

The solution here is to move the updating of the default langcodes into the batch created by locale_config_batch_build(). This makes the updating of the default langcodes run at the correct time regardless of when locale_system_update() is called.

Turns out that locale_config_batch_update_components() is never called with a list of components so we need to pass along a flag...

@Jose Reyero # 102
> Please lets keep us focused here.

You are right and wrong. Up to some days ago this was effectively a meta issue with a fruitful discussion of potential approaches. I messed the shift in the discussion, and i think it was wrong, because it buries a good conversation. And we do that too often.

Rant aside, here's the two issues.

@geek-merlin
I like being right and wrong at the same time, sounds like quantum computing... :)

So yes, and no... :D .. We need to fix this issue ASAP, which I can understand is a major headache for users. And we need to get a cleaner config translation workflow so we developers can have some peace of mind with Drupal translation. Thinking about creating another issue like "Do not mess with the language of the default configuration" or similar... and maybe some copy & paste and linking of comments here will help..

So, about @alexpott patch:

* Ok, about flag description, not that important anyway, it's the flag itself that's some hack.
* Not so sure about changing also locale_config_batch, that makes the patch 10x more complex
I think that batch is invoked from more places, unlike locale_system_set_config_langcodes() which was called only on module/theme install
* I liked more the previous patch #96.. but not sure about what else this latest one fixes - besides saving a function call, but that two steps also made the logic cleaner... (?)

What #106 does is reduce the time between when the langcodes of default config and the locale_config_batch_refresh_name() is called. This vital to ensure that problems do not occur. When this called after site install the implementation in #94 is okay. But all this code also runs during a site_install. Where the gap between updating the langcodes of the config and doing the config translation involves the chance of other modules making changes that will break the assumptions that we are making by doing locale_system_set_config_langcodes() and then locale_system_update() in the install hook.

We should never have put locale_system_set_config_langcodes() outside of the batch triggered by locale_system_update() - when we implemented that we just did it in the simplest way possible without thinking about the difference between site install and extension install after site install.

@Jose Reyero # 102
> We need to fix this issue ASAP, which I can understand is a major headache for users.

Did you understand that #101 fixes this bug completely and thoroughly? And is there today?
(So if that's tha focus, i'm more ;-), so please forgive my insistance). (You can answer in the other issue if you want)

Well, the more I test this, and think about it... yes... installing new modules when your default language is not English is a terrifying experience !!! Stll worse when you are doing it while deploying to production... :(_

So reconsidering a little bit... I think there is only one time when we should run that locale_system_set_config_langcodes() and that is *after a site/profile install* ... and never triggered after single extra modules are installed. That should also fix your issue "to ensure that problems do not occur" @alexpott?

(I've also noticed what happens when first you switch default language... nothing yet... but, then you configure something else, then you install some new module... then... boom!... configuration translations kind of implode... :.oO )

So I'm wondering, what if:

* We fix this annoying thing with locale/config_translation re-trans-translating itself, that would be #94 (still preferred) or #104 (makes some sense, but also makes it way more complex)
* We also kick that locale_system_set_config_langcodes() function out of hook_modules_installed() to somewhere after profile install.

?

I mean would this make everybody happier? - Not 'happy', just 'happier'... ok?

And yes @geek-merlin #101 will fix this issue for some people now you've added a description to the module ;) thx

@Jose Reyero I agree that a follow-up needs to be filed to determine what to do when a site changes it's default language. FWIW this is a super scary operation that is really not well supported. Also people are trying to share configuration across sites with different languages - see ✨ Configuration langcode is forced to site default language Needs work - that's just incredibly difficult if we're going to save translations in configuration - the approach that issue takes feels incredibly wrong and points us towards a solution like 🌱 Translate config at runtime (aka MultiLangNG in core) Postponed

With respect to changing how the current system works - I don't think we can change the fact that module install sets the default language for new configuration. What we can do and this patch moves toward is to make it more targeted and have less time when the system can possibly interact with configuration where the langcode has changed but locale has not had a chance to translate it yet.

The patch in #106 reduces the time between the langcode update and config translation on an install for both site install and extension install and therefore I think this is the patch we should commit to stop the rot and stop breaking people's configuration.

Totally agree with #111 and #112 (+ 1mio ;)) as someone who runs into the issues since day 1 and on all German projects. Thank you all for your great work and investigations.

Especially I agree it's better to improve the situation safely ASAP and proceed in follow-ups instead of keeping it for further years. This is surely one of the most critical issues for non-english Drupal projects of all sizes.

Ok,

I agree @alexpott's patch, any of them, is an improvement over what we've got atm and it should fix the main issue on this task.

Code looks good, though I didn't have the time for thorough testing yet.

As a side note, and ok, for a new task, the workflow when changing default language is badly broken... just wondering whether to create a new task or there's one already. (?)

@Jose Reyero I created 🐛 Configuration language is not adjusted after changing the site default language Active as I could not find a specific issue.

I will try to explain again why #106 is the correct patch, but I'll do it with a test that would be broken with #94. Patch incoming...

Here's the additions to the test that prove that the changes to the batch are necessary to fix this problem.

The last submitted patch, 116: 2806009-94-with-new-test-additions.will-fail.patch, failed testing. View results →

Created the follow-up 📌 Deprecate locale_system_set_config_langcodes() Needs work

@alexpott

Thanks for creating the other tasks, I'll follow there..

Only, about your new test... so a module edits its own configuration on hook_modules_installed... and that makes its own translations get messed up... I mean LOL! ...

But OK, let's say it's a valid use case... It only makes an stronger case for #111 ... We should never run that locale_system_set_config_langcodes() upon module installs... and it would also pass this test ... :D

Anyway... this is a +1 from me, or maybe a +2 seen that it can also pass the trickiest install test ;)

More reasons for this patch @geek-merlin... by moving the locale_system_set_config_langcodes() into the LocaleConfigManager service, it will be much easier to knock it out using service overrides. (!)

But OK, let's say it's a valid use case... It only makes an stronger case for #111 ... We should never run that locale_system_set_config_langcodes() upon module installs... and it would also pass this test ... :D

No it wouldn't pass this test. Because config would not be translated correctly. Stopping the langcode of default configuration from being changed on module install will take a pretty hefty re-write of how config translation works. Just doing that on its own would actually result in a more broken system. Because you'd have a mix of languages in the default config collection. Config created during site install would have 'en' whereas config created by the user after would have the site's default langcode.

@Jose Reyero is a +2 enough for a rtbc? It would be great to stop the rot this problem causes and also move on 🐛 Configuration language is not adjusted after changing the site default language Active which really needs this change in order to work well.

@alexpott

> is a +2 enough for a rtbc?

I'd really like someone else to give some more feedback here, maybe that RTBC. I mean it's been 5+ years discussing this one, it feels like while closing the original bug report, we are leaving a lot of issues unanswered.

Also I share some of @geek-merlin #107 concerns:

>Up to some days ago this was effectively a meta issue with a fruitful discussion of potential approaches. I missed the shift in the discussion, and i think it was wrong, because it buries a good conversation. And we do that too often.

@Jose Reyero well if we want to keep this ticket open we can move this patch to 🐛 Configuration language being overwritten during module install Needs work which is a duplicate critical issue. In my opinion we need to fix this bug ASAP (on whichever d.o nid we like) and then the changing default language issue. The ideas to completely change configuration translation come with their own set of issues and will need extremely complex update paths and deprecations of existing code. Also it is very likely that some of the existing functionality is untested and relied upon by some projects so whatever we do will create a new set of criticals.

@alexpott and @Jose Reyero: Strongly agree with #122 AND #123. You're both totally right and I think @alexpott's suggestion in #123 to improve things as far as possible NOW and improve things carefully later, is the best thing we can do here (in my oppinion).

Worst thing we can do here, is to make things worse in code (definitely not the case, from my perspective with that fix) or wait for 5 more years (or even if less) to improve things.

I'd have given the RTBC, but I'm not experienced enough to be 100% sure everything is fine. Perhaps @andypost, @geek-merlin or someone from the core team, who has a lot of experience on this topic, could sign this off?

+1 for any improvement. Looked into the patch and it looks reasonable, but a thorough code review or rtbc is beyond my capacities these days.

FYI.

I have this issue.
Tried #76 and #106 patches.

My environment:
+ Drupal 9.5
+ Default langcode is Ja, monolingual
+ Check on '/admin/content/media'

Steps to reproduce the error:
+ install site
+ import configurations (views.view.media.yml in this case) which is langcode 'ja' and label of 'Status' changed.
+ `drush locale:check`
+ `drush locale:update` <-- issue appeared here

The result is `Status` label is fallback to the translate of string `Status`, not the already translate string in `views.view.media.yml`
=> The translation of views change unintentional.

About patch #76: It not change the text in this case, but it is too brutal.
About patch #106: The configuration is changed.
I have a check on `vendor/drush/drush/src/Drupal/Commands/core/LocaleCommands.php::update`
it call locale/locale.fetch.inc::locale_translation_batch_update_build
Seems patch #106 does not take affect to the processing of this function

I agreed to @Anybody in #124. I raise a case when the patch may failed to deeper improvement

@tra.duong your issue is not the same. The bug this issue describes and the fix only affects sites with more than one configurable language.

@alexpott, it not totally the same yes. Because for monolingual, which is not English, it try to switch back to English and override the translation in View
I mean it override the configuration of the already overrided view.
This behavior answer why patch #76 works. It ignore the 'switch back' in locale_system_set_config_langcodes()
The thing to consider here is, which is made by user must be kept, even when switch back to English is not identical.
I put my issue here, to give another factor of this issue to easier to evaluate.

It'd be great if we could land this issue.

Updating issue summary with the solution.

I've added a CR to cover the API additions necessary to fix this bug - https://www.drupal.org/node/3348540 →

Simplest steps to reproduce:
1. Install minimal in German
2. Enable the views module after install.
3. Run the following query on the database: select language, source, translation from locales_source ls, locales_target lt where ls.lid = lt.lid and source = 'Items per page';

Before you will see:

+----------+----------------+----------------+
| language | source         | translation    |
+----------+----------------+----------------+
| de       | Items per page | Items per page |
+----------+----------------+----------------+

With the patch

+----------+----------------+--------------------+
| language | source         | translation        |
+----------+----------------+--------------------+
| de       | Items per page | Elemente pro Seite |
+----------+----------------+--------------------+

FWIW what makes this bug even worse is that prior to installing views on the site on HEAD if you run the query you will see:

+----------+----------------+--------------------+
| language | source         | translation        |
+----------+----------------+--------------------+
| de       | Items per page | Elemente pro Seite |
+----------+----------------+--------------------+

So it's not just that we don't translate the new configs correctly we ALSO break existing translations.

@alexpott we can definitely confirm that from our daily experience ;)
One of the most critical things in Drupal currently still existing...

Tested on Drupal 10.1 standard install with english starting off
Added German as a 2nd language
Set German to default language
Deleted English
Installed activity tracker
Went into User interface translation searched for Content and got the English word. See screenshot

Applied patch

Does not automatically change Content back. Think that's expected. Please confirm

Added English back as a language
Made default
Deleted German and I get this error

Deprecated function: Automatic conversion of false to array is deprecated in Drupal\locale\LocaleDefaultConfigStorage->read() (line 93 of core/modules/locale/src/LocaleDefaultConfigStorage.php).

May be unrelated Please confirm
Readded German
Verified on User interface translation the word content is translated
Made German default
Deleted English
Installed Activity tracker again
Verified on User interface translation the word content is translated still.

I'm good to mark this if those questions could be answered.

Actually may have found an issue.

Readded English
Made English the default
Went to Content types page. Things are showing as German still

@smustgrave changing the default language via the UI is fraught there is an issue for that 🐛 Configuration language is not adjusted after changing the site default language Active - we need to fix this first in order to make that worth fixing.

Spoke to @alexpott about the issues in bold and they are not issues or issue that needs to be fixed here. So marking it.

I've added instructions about how to fix an affected site to the CR - see https://www.drupal.org/node/3348540 → - unfortunately this can not be automated.

@alexpott re #141 perhaps our module https://www.drupal.org/project/l10n_tools → can help. It has the ability to remove equal translations and re-import them.

Of course, it's not an end user tool, but if it works for this case, it MIGHT be helpful to add it to the CR as contrib helper?

The last submitted patch, 116: 2806009-94-with-new-test-additions.will-fail.patch, failed testing. View results →

@Anybody sounds like a good idea to me - go for it! If you can update with tested instructions for how it might work that'd be great. There's no real end user way of fixing a site.

Notes to committers - this could be backported to 9.5.x - all the API changes are additions and have sensible defaults that align to the current behaviour.

+++ b/core/modules/locale/tests/src/Functional/LocaleConfigTranslationImportTest.php
@@ -305,4 +305,66 @@ public function testLocaleRemovalAndConfigOverridePreserve() {
+  /**
+   * Tests setting a foreign language as default and importing configuration.
+   */
+  public function testConfigTranslationWithForeignLanguageDefault() {
+    /** @var \Drupal\Core\Extension\ModuleInstallerInterface $module_installer */

Everything looks good, except this should be 'a non-English language' and the same with the method naming, which nod_ pointed out in #53.

Just a rename/docs change so fine to set straight back to RTBC with that change and I think I'm happy to commit otherwise.

Actually no - that's fixable on commit, so I've gone ahead and done that.

Committed/pushed to 10.1.x, thanks!

I agree this can be backported to 10.0.x and 9.5.x but will wait until we decide about a hotfix/early patch release for 🐛 [regression] Language switcher block throws exception when no route is matched Fixed first.

The release for 🐛 [regression] Language switcher block throws exception when no route is matched Fixed is out, so I backported this to 10.0.x and 9.5.x and published the change record

Maybe change record needs changes as it was backported to 9.5??

Adjusting issue credit as credit is not provided for button-click rebases without additional contribution.

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

Looks it caused random failure in 10.1 after upgrade to SF 6.3 #3361121-17: [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync →