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
Comment | File | Size | Author |
---|---|---|---|
#24 | 2925203-3-24.patch | 59 KB | alexpott |
#24 | 20-24-interdiff.txt | 1.33 KB | alexpott |
#20 | 2925203-3-20.patch | 58.91 KB | alexpott |
#20 | 19-20-interdiff.txt | 7.3 KB | alexpott |
#19 | 2925203-3-19.patch | 11.32 KB | alexpott |
Comments
Comment #4
svdhout CreditAttribution: svdhout at Calibrate commentedWe 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?
Comment #5
borisson_#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.
Comment #6
svdhout CreditAttribution: svdhout at Calibrate commentedI did some more tests, and cannot reproduce on a clean drupal install.
Will investigate further, but it will probably be unrelated
Comment #12
alexpottHere 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:
vendor/bin/drush si minimal --locale=de
vendor/bin/drush cex -y
Verwaltung
to something elsevendor/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.Comment #13
alexpottHere's an initial fix that needs tests and much work
Comment #14
andypostThat's great that after many years this bug has new issue again!
Comment #15
alexpottHere'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 inlocale_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 maybelocale_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.Comment #17
alexpottI'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.
Comment #18
alexpottHere's the current patch on top of 8.9.x since that's what I need for a project I'm involved with.
Comment #19
alexpottIn 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:
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.
Comment #20
alexpottHere'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.
Comment #21
daniel.bosenI 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: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.
Comment #23
catchNit: 'The ensures', probably should be 'This ensures' or just 'Ensures'.
Needs an issue to link to.
Probably would have built a big array, then chunked it instead of using modulo, but this is more concise so +1.
Comment #24
alexpottThanks 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.
Comment #28
catchOK. 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.
Comment #30
heddnLinking #3150185: Locale is very slow to import strings, which is a major performance regression that seems related to this.