Problem/Motivation
install_finish_translations() in install.core.inc passes array_keys($languages) from a full language list directly to LocaleFetch::buildUpdateBatch() and LocaleConfigBatch::buildBatch(). When a profile sets keep_english: true in its info file, English is retained as a language entity, and it ends up in this $languages array. This causes the installer to attempt to download and import translations for English, even though that is futile even if locale.settings.translate_english is enabled.
Steps to reproduce
https://git.drupalcode.org/project/drupal_cms/-/work_items/3591421 introduces this option by default to the Drupal CMS installer, so this bug will be encountered by default by people installing Drupal CMS.
With core:
- Create an installation profile with
keep_english: truein its.info.ymlfile (or see core'stesting_multilingual_with_englishtest profile). - Install in a non-English language (e.g. French).
- Observe that the installer has steps to fetch English translations during the "finish translations" phase.
Proposed resolution
There was an existing check in LocaleFetch::batchStatusCheck() to avoid pinging the default localization server for English but the same check did not exist in LocaleFetch:: getFetchOperations(). For download and import we already know ahead of batch building if we should add the operation. For status checking there are other actions done even if the remote file is not checked, so we keep it there.
Remaining tasks
Review.
User interface changes
None. (Installation will be faster when keep_english is TRUE).
Introduced terminology
None.
API changes
None.
Data model changes
None.
Release notes snippet
Not needed,
| Comment | File | Size | Author |
|---|
Issue fork drupal-3618547
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3618547-when-keeping-english
changes, plain diff MR !16803
- 3618547-11.4.x
changes, plain diff MR !16808
Comments
Comment #3
gábor hojtsyAdded MR with tests, needs review :)
Comment #6
gábor hojtsyComment #7
gábor hojtsyComment #8
oily commentedI have been trying to reproduce this manually. I tried install using drush with Drupal 12.0-dev. Then via the UI. Both times only install language that works is English.
I think this is still valid:
https://www.drupal.org/project/drupal/issues/3443682
Not sure which version you are testing on gábor hojtsy? Could update the IS with the version..
I have an 11.4 Drupal site, can re-test on that. The command could look like:
drush site:install testing_multilingual_with_english --locale=fr --account-name=admin --account-pass=admin -y -vvv
(I copied the testing_multilingual_with_english profile into the core/profiles directory.)
Comment #9
nitinkumar_7 commentedprobably covers the normal multilingual translation behavior, so this may be sufficient.
But before marking RTBC, I'd inspect what InstallerTranslationMultipleLanguageTest::testTranslationsLoaded() actually asserts, because that's what determines whether the regression test fully covers both sides:
.
Comment #10
oily commentedWithout the code fix, tail of the output of the command at #8 is:
[info] testing_multilingual_with_english module installed. [3.32 sec, 20.46 MB]
[notice] Performed install task: install_install_profile [3.33 sec, 20.86 MB]
[notice] Translations imported: 30114 added, 0 updated, 0 removed. [22.71 sec, 22.85 MB]
[notice] Performed install task: install_import_translations [22.71 sec, 21.15 MB]
[info] update module installed. [23.16 sec, 25.85 MB]
[notice] Performed install task: install_configure_form [23.38 sec, 26.34 MB]
[notice] The configuration was successfully updated. 127 configuration objects updated. [23.61 sec, 29.41 MB]
[notice] Performed install task: install_finish_translations [23.61 sec, 29.39 MB]
[notice] Performed install task: install_finished [23.8 sec, 22.91 MB]
[success] Installation complete. (Admin) [23.81 sec, 22.95 MB]
Comment #11
oily commentedTo manually test on Drupal 11.4.x with the code fix to see how the output looks in relation to #10 a new branch based on 11.4.x would be useful. As mentioned in #8 it is not possible to install using any language except en, unless there is a workaround?
Comment #15
oily commentedNot sure a new branch is necessary. Will try checking out the changed files from the branch with the fix to my local 11.4.x branch.
Comment #16
gábor hojtsyUpdated issue summary with simplified recommendation and cross-linking to Drupal CMS issue.
Comment #17
gábor hojtsy@oily: let's not get ahead of ourselves opening 11.4 backport branch until the main MR is agreed on because its a lot of extra work to keep them in sync. I closed the 11.4 backport branch for now that you opened to keep us focused.
I understand the UI install would not work on main currently due to #3605895: Drupal core main branch does not have corresponding .po files on localize.drupal.org until first alpha is tagged, fall back one major version, you can try the same code change on 11.x, which is where the backport would land (I don't know if it would go down as far as to 11.4, maybe). The meat of the change in one line :)
Comment #18
berdirThe loadInclude() calls were kept on purpose previously, that's the approach we've landed on and while I don't think there's a case in the installer where this could be an issue, removing that is not in scope for this isue.
Comment #19
gábor hojtsyI will add them back. Why do we need them? I don't think the autoloaded classes need them? Do we consider them providing installer API functions that we want to keep providing? (I personally would not). That said, I agree removing them is not needed to resolve this issue.
Comment #20
gábor hojtsyPut it back :)
Comment #21
berdirThe reason we kept them is that in theory someone could rely on it being loaded later in the chain, due to a hook or batch alter or what not and call those deprecated functions. Just recently someone reported a problem like that about a views_ui function in an include file.
That said, I think I'd prefer moving the functions into the .module file instead of this, but either way, out of scope for this issue.
I'm not sure if those comment updates are actually helpful. It makes sense in the context of this MR, but later on, anyone reading that without the context of the *removed code* is possibly just going to be confused why it talks about EN when it doesn't do anything with it. But leaving that for a second opinion.
Also note that there is likely more than one option to have EN at this point. We have a test install profile (or module that an install profile depends on) in core that ships with multiple languages as default config.
Comment #22
oily commented#17: Agreed. Sorry, it was a mistake. Was not considering backporting, just testing your code fix on 11.x branch. Got confused. As you say can just test on my local 11.4.x branch.
I was looking at doing that eg today to get a comparison with stats in #10. Does not seem necessary now that berdir has moved to RTBTC.
Comment #23
gábor hojtsyRe the code comments, I wrote them all by hand carefully :) Good point about keep_english not being relevant if the profile was installed with config install for example. There is #3545306: Installation from config removes English if the installed profile does not have keep_english for a related bug actually from recently. I made a very minor code comment update to be more general there, keeping at RTBC though.
Re the include files, I see they are deprecated for removal in Drupal 13, but that may not mean Drupal 12 needs to include them in the installer still :) Unrelated anyway, I agree.
Comment #24
nicxvan commentedThat's my plan when we finish the deprecations.
Comment #25
gábor hojtsyI slept on this and realized this solution is wrong :/ Ok we don't fetch the English translations but we still create the English overrides. Conceptually this would only make sense if we also allow the user to edit the English overrides.
Take this situation. Drupal CMS ships with a dashboard, that would be translated to whatever language the user chose in the installer. Drupal CMS also defaults to an English admin UI. The dashboard therefore should have the English overrides. But then the user modifies the dashboard, they will need to modify the translated dashboard which is the default config and would need to add the English overrides for the labels they would need to see on the English admin UI.
So "translate to English" should be enabled, which means the English .po files will still be attempted to be fetched and imported. Core already has a check for this in the fetch job itself, but not while building the fetch jobs, so core happily builds a fetch job then it knows to not run :D We should instead not build the fetch job that it will not run and that allows us to enable English translatability without .po files being pulled uselessly.
Comment #26
gábor hojtsyFurthermore 9 months ago #3518992: Config overrides are loaded for English even when translate_english is false introduced a container parameter to track whether English overrides should be loaded in the first place and when locale is enabled, that is now tied to locale module's setting. So even to get English overrides to load / make sense, we need the translate English option enabled, which would check the .po files with the current core code.
Comment #27
gábor hojtsyUpdated the fix, so needs review again :) I generalized an existing English checker section from locale update checking, now also applied on download/import. I don't think this can be tested either as it would be testing the network. It is highly tied to a condition about localize.drupal.org being true.
Comment #28
gábor hojtsyUpdated issue summary based on updated approach.
Comment #29
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.