Problem/Motivation

In #2791405: When installing a site in a language besides English, the site name is not saved and reverts to "Drupal" it was discovered that \Drupal\locale\LocaleConfigSubscriber::onConfigSave() can result in data loss during an install. Specifically it was affecting the site name. That issue was resolved because a default for site name does not really make sense so there is nothing to automatically translate from localise.drupal.org. However, if an install form did set a translatable value which had a translatable default value we'd have a problem. In core there are no values like this but distributions might have a form like this.

This is a major bug because of the potential data input loss. That said apart from #2791405: When installing a site in a language besides English, the site name is not saved and reverts to "Drupal" I don't know of any other issues caused by this.

Proposed resolution

It does not look like there is a simple fix because it is not possible to determine if a configuration save comes from a form submit or a module install or part of configuration translation. One idea would be change how LocaleConfigSubscriber determines whether or not it fires properly during installation and only disables itself on particular steps. This feels risky though. Maybe we need to reconsider what #2468767: English config source strings are saved as custom translations in locale in foreign install was fixing and see if there is another way.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#24 2925203-3-24.patch59 KBalexpott
#24 20-24-interdiff.txt1.33 KBalexpott
#20 2925203-3-20.patch58.91 KBalexpott
#20 19-20-interdiff.txt7.3 KBalexpott
#19 2925203-3-19.patch11.32 KBalexpott
#19 2925203-8.9.x-19.patch10.45 KBalexpott
#19 17-19-interdiff.txt2.95 KBalexpott
#18 2925203-8.9.x-18.patch9.74 KBalexpott
#17 2925203-3-17.patch11.87 KBalexpott
#17 15-17-interdiff.txt9.84 KBalexpott
#15 2925203-14.patch6.77 KBalexpott
#15 2925203-14.test-only.patch2.63 KBalexpott
#13 2925203-13.patch2.06 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

svdhout’s picture

svdhout CreditAttribution: svdhout at Calibrate commented

We are experiencing issues where the mails send to the user (e.g. password recovery mails) use the config inside the translated configuration yml file (at config/default/languages/en/user.mail.yml)

The user.mail.yml exists both in config/default as in config/default/languages/en (as well for all the other languages)
We change the password recovery text in the interface, and press save, the correct text is shown inside form.
After exporting configuration, the config/default/user.mail.yml is updated, but config/default/languages/en/user.mail.yml is not.

When Drupal then tries to send the password recovery mail, it uses the configuration inside config/default/languages/en/user.mail.yml

After i delete the config files at config/default/languages/en and export configuration again, new config files are created, but they still have the wrong configuration

As a workaround we manually update the translated config files, but that means our clients cannot update the email text.

We did not enable the english interface text to be translatable, so i guess there shouldn't be any config files inside the config/default/languages/en directory

Could this issue be related to the problem mentioned here?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Calibrate commented

#4 doesn't seem related in when this happens compared to the IS. The problem might very well be related, and is probably easier to test compared to writing a test for the installer.

svdhout’s picture

svdhout CreditAttribution: svdhout at Calibrate commented

I did some more tests, and cannot reproduce on a clean drupal install.
Will investigate further, but it will probably be unrelated

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Here are concrete steps to reproduce the problem. Being able to install from configuration makes this quite easy to hit.

Pre-requisites:
composer require drush/drush

Steps:

  1. Install a monolingual non english site: vendor/bin/drush si minimal --locale=de
  2. Export the config: vendor/bin/drush cex -y
  3. Adjust a translation: Edit the exported block.block.stark_admin.yml and change label from Verwaltung to something else
  4. Re-install the site from config: vendor/bin/drush si --existing-config

The label should be whatever you set it to in step 3 but it will be back to Verwaltung because we've not stored the override during a run of \Drupal\locale\LocaleConfigSubscriber::onConfigSave() because we disable it during installation.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
2925203-13.patch2.06 KB

Here's an initial fix that needs tests and much work

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

That's great that after many years this bug has new issue again!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
2925203-14.test-only.patch2.63 KB
2925203-14.patch6.77 KB

Here's a test and a different fix from #13. It moves the processing in a config import listener that is triggered after a config import that installs locale. I think given locale_modules_installed() and the install check in locale_system_update() that this might be doing unnecessary work for the config import not during an install case. Need to add test coverage (or see if there is test coverage). Plus maybe locale_system_update() is points towards what we should be doing in the installer. Maybe need to go back to something closer to #13 - but at least we have the test.

The last submitted patch, 15: 2925203-14.test-only.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
15-17-interdiff.txt9.84 KB
2925203-3-17.patch11.87 KB

I've proved that this doesn't not affect config installs after Drupal has been installed. This is because \Drupal\locale\LocaleConfigSubscriber::onConfigSave() will run during a config install. Even if the locale module is installed - so I think we're okay there. Given that I've move all of this back into the installer and mostly out of \Drupal\locale\LocaleConfigSubscriber - we still need access to some of the code it has though.

I've added an additional batch process to the configuration install so that we can process all the config for translation overrides and save any customisations to the locale table.

I do have a concern about regular configuration import - what happens if a module is installed prior to locale that has configuration overrides so I'm going to work on adding test coverage for that.

Additionally I've discovered something very funky - when installing locale via the config importer it'll set a batch process because it assumes that it is part of a module install via the UI. There's nothing thats going to pick this batch up and run with it as far as I know. The same happens via Drush compare installing Locale on a site with French enable via drush or via the UI. If you trigger the install via the UI you get some translations imported via drush you do not.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
2925203-8.9.x-18.patch9.74 KB

Here's the current patch on top of 8.9.x since that's what I need for a project I'm involved with.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue tags: +Needs tests
Related issues: +#2871357: Installer tasks using multiple batch sets in non-interactive mode do not get executed
FileSize
17-19-interdiff.txt2.95 KB
2925203-8.9.x-19.patch10.45 KB
2925203-3-19.patch11.32 KB

In further testing with real translated sites we've discovered that #17/#18 is not enough.

The problem with the current approach is that it occurs prior to the locale tables being loaded so all translatable strings in configuration are treated as customised translations even if they came from locale.drupal.org - that's not right. Another issue is that if the configuration contains strings that make the translation source and this configuration is processed after something that has set the translation correctly - it'll save the source string as customised string. Definitely not what you want.

In order to fix this we need to:

  • Move the configuration processing after the translations have been imported
  • Do not set a translated string to the source value during installation.

Also if you are testing this using drush you will run into #2871357: Installer tasks using multiple batch sets in non-interactive mode do not get executed because the installer is broken when processing multiple batches in the same step non interactively!

I need to add more test coverage for this.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue tags: -Needs tests
FileSize
19-20-interdiff.txt7.3 KB
2925203-3-20.patch58.91 KB

Here's additional test coverage that proves that with the changes translations are created in the expected state - ie. customized or not. And that once the site has been installed you can use the translation updating as expected.

Note the interdiff does not cover the changes to core/tests/fixtures/config_install/multilingual.tar.gz - things config export now contains views configuration and has views enabled.

daniel.bosen’s picture

daniel.bosen
he/him
CreditAttribution: daniel.bosen at Thunder commented
Status: Needs review » Reviewed & tested by the community

I tested this by installing a german installation of the Thunder profile, with lots of contrib-modules enabled.
Calling drush si --locale=de thunder -y with the path in #20 resulted in:

 [notice] Starting Drupal installation. This takes a while.
 [notice] Performed install task: install_select_language
 [notice] Performed install task: install_select_profile
 [notice] Performed install task: install_load_profile
 [notice] Performed install task: install_verify_requirements
 [notice] Performed install task: install_verify_database_ready
 [notice] Performed install task: install_base_system
 [notice] Performed install task: install_bootstrap_full
 [notice] Performed install task: install_profile_modules
 [notice] Performed install task: install_profile_themes
 [notice] Performed install task: install_install_profile
 [notice] Translations imported: 10612 added, 0 updated, 0 removed.
 [notice] Performed install task: install_import_translations
 [notice] Performed install task: install_configure_form
 [notice] Performed install task: thunder_module_configure_form
 [notice] Performed install task: thunder_module_install
 [notice] Performed install task: thunder_finish_installation
 [notice] Translations imported: 3831 added, 1138 updated, 0 removed.
 [notice] Performed install task: install_finish_translations
 [notice] Performed install task: install_finished

All translations were correctly imported (without the patch the second "Translations imported" line is completely missing, and all contrib-translations are missing).
I am not sure if the number of updated translations might be a concern. Counting the customized translations after installation resulted in 0. So it looks good to me.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/includes/install.core.inc
@@ -2436,3 +2443,88 @@ function install_config_revert_install_changes() {
+ *
+ * The ensures that the logic from LocaleConfigSubscriber::onConfigSave() is run
+ * on sites after installing from configuration so updating translations from
+ * PO files does not result in overwriting customizations.
+ *

    Nit: 'The ensures', probably should be 'This ensures' or just 'Ensures'. 

  2. +++ b/core/includes/install.core.inc
@@ -2436,3 +2443,88 @@ function install_config_revert_install_changes() {
+function _install_config_locale_overrides() {
+  // Somehow the config cache gets filled up with junk.
+  // @todo fix this.
+  \Drupal::service('cache.config')->deleteAll();
+

    Needs an issue to link to. 

  3. +++ b/core/includes/install.core.inc
@@ -2436,3 +2443,88 @@ function install_config_revert_install_changes() {
+    // together to improve the overall performance of the process.
+    if ($i % 20 == 0) {
+      $batch_builder->addOperation('_install_config_locale_overrides_process_batch', [$batch_names, $langcodes]);

    Probably would have built a big array, then chunked it instead of using modulo, but this is more concise so +1.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Reviewed & tested by the community
FileSize
20-24-interdiff.txt1.33 KB
2925203-3-24.patch59 KB

Thanks for the review @catch - I created the follow-up #3252244: Work out why _install_config_locale_overrides() needs to clear the config cache and linked it in the @todo.

Given both review fixes are to comments setting back to RTBC.

  • catch committed 86e9f19 on 10.0.x 
    Issue #2925203 by alexpott: LocaleConfigSubscriber can result in data...

  • catch committed 011c394 on 9.4.x 
    Issue #2925203 by alexpott: LocaleConfigSubscriber can result in data...

  • catch committed 8ee62fe on 9.3.x 
    Issue #2925203 by alexpott: LocaleConfigSubscriber can result in data...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

OK. Committed/pushed to 10.0.x/9.4.x/9.3.x, thanks! #3169756: Installing Drupal from configuration should not result in configuration translations being updated I think we can mark as duplicate now. I also have a suspicion there's another duplicate around, but can't locate it right now.

Status: Fixed » Closed (fixed)

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

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Tag1 Consulting for Google commented

Linking #3150185: Locale is very slow to import strings, which is a major performance regression that seems related to this.