Problem/Motivation
When a language is picked in the installer, it downloads the translation files initially and later on imports the translation, enables all the modules, etc. At the end of the installer it checks if there are any more projects to deal with (ie. the profile may have enabled contrib modules/themes that need translations downloaded). It does not check if the profile added any languages though and only concerns itself with the language selected for the installer (incorrectly).
Proposed resolution
- Changes
install_tasks()
to make the translation tasks available also if the language manager says the site is multilingual. Ie. if English is used in the installer but there are also other languages on the site. This situation arises after language module is installed and the install profile or another project ships with other language entities. install_check_translations()
is used to download the translation files originally. Made this into a reusable API function by taking a langcode and server pattern, so we can use it to download more translation files later.- In
install_import_translations()
use that to download further translations if needed (other than the installer language) and build the import batch to include all the languages.
=> At this point the installer already downloaded and imported translations for ALL the languages the user selected + the profile included. Further changes are needed though to follow this up later on in the installer.
install_import_translations_remaining()
runs after the config form. This is to cover modules/themes updating possibly as a result of configuration in the config form. While this is not a feature of core, it is possible in contrib. This should consider all languages on the site too, not just the installer language.- Finally,
install_update_configuration_translations()
runs, which creates all the config translations based on the imported translations. This one also needs to consider all languages on the site.
Remaining tasks
Tests. Review. Commit.
User interface changes
Translation steps will show up even if the installer is in English BUT there are other languages added as well.
API changes
Minor changes in the installer tasks, no outside API changes.
Comment | File | Size | Author |
---|---|---|---|
2336743-languages-profiles-fix-head.patch | 717 bytes | webflo | |
#46 | 2336743-languages-profiles-46.patch | 17.66 KB | webflo |
#46 | 2336743-languages-profiles-46.interdiff.txt | 2.4 KB | webflo |
#40 | 2336743-languages-profiles-40.patch | 17.65 KB | webflo |
#36 | 2336743-languages-profiles.interdiff.35-36.txt | 3.23 KB | penyaskito |
Comments
Comment #1
Gábor HojtsyManually tested this patch with English + 2 other languages from config and Hungarian selected + two other languages from config.
Comment #2
Gábor HojtsyComment #5
Gábor HojtsyComment #6
YesCT CreditAttribution: YesCT commentedread through the patch. changes make sense.
went ahead and fixed a few nits.
@Gábor Hojtsy can you post the profile you used to test it?
Comment #7
Gábor HojtsyThe profile I am testing with is https://www.drupal.org/sandbox/aimeerae/2267517 (tip of 8.x-1.x). That includes this patch (from #1) and #2030537-39: Translations not downloaded when adding a new language as well. Test it interactively at http://simplytest.me/project/2267517/8.x-1.x.
Comment #8
Gábor HojtsyFeel free to work on this btw :)
Comment #9
YesCT CreditAttribution: YesCT commentedjust the beginning of a not correct test.
I might be able to look at this again later.
Comment #13
Gábor HojtsyRerolled since it did not apply. Your "test only" patch included the fix also. This one rolled so we have a test, a fix only and a compound patch.
Comment #16
Gábor HojtsyTest failed because the submit button was not translated for the test. This one line fix should make that pass, but obviously the test is not really testing much useful yet. I have some ideas as to how we will make the languages appear in the test at the right time. Experimenting now :)
Comment #17
Gábor HojtsyAll right, expanded tests a bit and made an attempt to make the configurable languages be created, but could not make that work. Will ask @alexpott... Changes here:
- Added a testing.install with a testing_install() which would create a few configurable languages when it is time for it (ie. when a test tells it via global)
- Added code to the test to set that global via settings.php so the profile knows to add the languages; while this is correctly saved in the settings file this happens way after the modules are installed, so does not seem to be early enough, which is puzzling to me... it should happen earlier based on looking at the code :/
- Added actual asserts to the test, so it would pass when it works as expected
- Split the test into two tests, one where English is the installer language and we still expect both German and Spanish to ALSO show up at the end and where German is the installer language, and we expect Spanish to also show up (but not English)
All will fail as expected without this patch because the connection between the test and creation of the configurable entities does not work yet.
Comment #19
Gábor HojtsyAll right, talked to @alexpott, he suggested we introduce a new install profile to avoid futzing with the profile in the test and vice versa. This now has a clear install profile that is clearly set up in a way that a multilingual profile would be (ie. it has locale as a dependency and German and Spanish added). The .po files are pre-created in the test to avoid needing to go out to the internet in the test.
This also uncovered two issues in the patch itself. First, the language should only be created anew if not already on the system (ie. if in this profile, we pick German, that is already a shipped language so no need to create it, but we need to make it default). Second, the remote translations were forcibly downloaded in all cases. We should not download translations if there is already a shipped translation.
The interdiff includes the new testing_multilingual profile. I also removed all changes to the testing profile. That later one is not in the interdiff though.
Comment #20
YesCT CreditAttribution: YesCT commentedI read through this and didn't spot anything that would needs work.
Did not test it manually with an install profile.
A tests only patch would be reassuring to make sure the fails are what we expect.
But looks good to me.
Comment #21
Gábor Hojtsy@YesCT asked for a test only version. Here it is. This will FAIL.
Comment #23
Gábor HojtsyFailed exactly on all the things expected.
Comment #24
Gábor HojtsyPatch to commit is #19 :)
Comment #25
YesCT CreditAttribution: YesCT commentedcool. looks like the tests fail in the places we expect. :)
Comment #26
alexpottIf the count of languages is 1 and $languages[en'] is not set then won't
$langcode == $install_state['parameters']['langcode']
?Couldn't we change the condition to
if ($langcode != 'en' && $langcode != $install_state['parameters']['langcode']) {
and lose the outer if?Also do we have to worry about time outs here - should this be batched?
Not used
Comment #27
Gábor Hojtsy@alexpott:
- re 1. the outer if controls whether the code after the foreach() as well. I can move the outer if later to right after the foreach but that will not loose the outer if.
- re 1. batching, yeah you are right, but this is a bit more complex, I hoped to avoid it here, since we are using slightly different rules for what is a valid .po file version in the installer since we don't have update module early enough, so this would either need to mix runtime download code with the installer import code or try to use the runtime import code which may not recognize the files as valid translations depending on your core version; or we need a parallel batch builder just for the installer which will look ugly; that is a larger problem; anyway, will try to look into batching this proper and see if I can come up with something more useful...
- re 2. will do in next roll
Comment #28
Gábor HojtsyAll right, factored the language entity / download prep code into its own batch preceding the import batch. Since I needed to move around some very ugly include file references, I fixed those too. Also fixed the extra use statement and used the suggested if without the outer condition now that the import and download are in separate installer steps.
Comment #29
Gábor HojtsyWhile this works wonders, the batch step which is not visible will make it appear no tasks run yet, so visually not acceptable. Either need to make that a displayed step or somehow integrate in the rest of the steps.
Comment #30
Gábor HojtsySo I think to fix this:
1. Re-integrate install_download_additional_translations() and install_import_translations() into install_import_translations() so there is only one installer step and it is visible in the installer tasks list.
2. Instead of using the locale batch builder, just build the batch ourselves. The batch would start off with downloads of the additional files (there is already batch builder code for this in the patch) and continue with batch steps that do the import prepare invocation and the import. The import prepare assumes the file is already on the filesystem so it should happen after the file is downloaded.
If we don't use the locale batch builder in install_import_translations() and just use our own, this can be resolved and the two batched tasks can be integrated in one and there will be much rejoicing. The test should still work as-is and all other changes should be fine as-is in the patch. Just the two installer tasks need to be merged IMHO.
@penyaskito indicated he could work on this, so unassigning.
Comment #31
penyaskitoComment #34
Gábor HojtsyRerolled. Needs review for testbot. Still needs work.
Comment #35
Gábor HojtsyThe patch currently accidentally removes the site default language setting. Duh. That should be fixed. The interdiff shows added code but that is actually NOT removed by the patch itself.
Comment #36
penyaskitoNot sure if this is what required. Installation of multilingual_demo completes, and the login box has the proper translations after switching languages, so I guess it is.
Progress messages while batching can be improved, and also OOPifying if possible.
Comment #38
Gábor HojtsyThe code changes look exactly like I envisioned. However it needs to pass the tests. Looks like it does not actually download or import the translations.
Comment #39
Gábor HojtsyThis is needed for our Lab. Tagging D8MIAMS.
Comment #40
webflo CreditAttribution: webflo commentedlocale_translation_batch_fetch_download and locale_translation_batch_fetch_import depend on the result from _install_prepare_import. Because _install_prepare_import populates the database (and locale_translation_get_projects) with project information.
Comment #41
k4v CreditAttribution: k4v commentedWe found a related issue: https://www.drupal.org/node/2345985: install_import_translation() fails if the language already exists.
Comment #42
Gábor Hojtsy@k4v: but that one is already fixed in this issue, no? We have a test that ships with German AND it picks German for installer translation too.
Comment #43
Gábor Hojtsy@webflo: can you post an interdiff for review?
Comment #44
k4v CreditAttribution: k4v commented@gabor: I don't think so, in install_import_translation there is no check.
Comment #45
Gábor Hojtsy@k4v: before the patch the creation of the languaeg is indeed in install_import_translations() BUT after the patch it is in install_download_additional_translations_operations() which is where it has the if(). So the patch fixes it, right?
Comment #46
webflo CreditAttribution: webflo commentedHere is the interdiff.
Comment #47
webflo CreditAttribution: webflo commentedComment #48
Gábor HojtsyI think this looks great. Since this was RTBC we had several people cross reviewing the changes and I believe the final fix is both technically correct and fits well with how the installer works.
Comment #49
webchickCommitted and pushed to 8.x. Thanks!
Comment #53
swentel CreditAttribution: swentel commentedComment #55
Gábor HojtsyComment #57
Gábor Hojtsy