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

  1. 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.
  2. 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.
  3. 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.

  1. 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.
  2. 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
8.83 KB

Manually tested this patch with English + 2 other languages from config and Hungarian selected + two other languages from config.

Gábor Hojtsy’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 1: 2336743-installer-actually-multilingual.patch, failed testing.

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy
YesCT’s picture

read 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?

Gábor Hojtsy’s picture

The 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.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned

Feel free to work on this btw :)

YesCT’s picture

FileSize
10.64 KB

just the beginning of a not correct test.
I might be able to look at this again later.

Status: Needs review » Needs work

The last submitted patch, 9: 2336743-tests-only.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: 2336743-tests-only.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.8 KB
10.7 KB
8.9 KB

Rerolled 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.

The last submitted patch, 13: 2336743-test-only-13.patch, failed testing.

The last submitted patch, 13: 2336743-fix-and-test-13.patch, failed testing.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy
FileSize
10.77 KB
777 bytes

Test 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 :)

Gábor Hojtsy’s picture

FileSize
5.29 KB
14.38 KB

All 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.

Status: Needs review » Needs work

The last submitted patch, 17: 2336743-more-tests-17.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
4.83 KB
14.46 KB

All 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.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

I 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.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests
FileSize
4.95 KB

@YesCT asked for a test only version. Here it is. This will FAIL.

Status: Needs review » Needs work

The last submitted patch, 21: 2336743-test-only-20-same-as-tests-in-19.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

Failed exactly on all the things expected.

Gábor Hojtsy’s picture

Patch to commit is #19 :)

YesCT’s picture

cool. looks like the tests fail in the places we expect. :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/includes/install.core.inc
    @@ -1549,19 +1550,36 @@ function install_import_translations(&$install_state) {
    +  if (count($languages) > 1 || !isset($languages['en'])) {
    +    foreach($languages as $langcode => $language) {
    +      // The installer language was already downloaded. Check downloads for the
    +      // other languages if any. Ignore any download errors here, since we
    +      // are in the middle of an install process and there is no way back. We
    +      // will not import what we cannot download.
    +      if ($langcode != $install_state['parameters']['langcode']) {
    +        install_check_translations($langcode, $install_state['server_pattern']);
    +      }
    +    }
    

    If 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?

  2. +++ b/core/modules/system/src/Tests/Installer/InstallerTranslationMultipleLanguageForeignTest.php
    @@ -0,0 +1,34 @@
    +use Drupal\simpletest\InstallerTestBase;
    

    Not used

Gábor Hojtsy’s picture

@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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
16.83 KB
5.3 KB

All 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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

While 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.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned

So 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.

penyaskito’s picture

Assigned: Unassigned » penyaskito

The last submitted patch, 28: 2336743-test-with-dedicated-profile-28.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
16.9 KB

Rerolled. Needs review for testbot. Still needs work.

Gábor Hojtsy’s picture

FileSize
16.94 KB
639 bytes

The 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.

penyaskito’s picture

Not 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.

Status: Needs review » Needs work

The last submitted patch, 36: 2336743-languages-profiles-36.patch, failed testing.

Gábor Hojtsy’s picture

The 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.

Gábor Hojtsy’s picture

Issue tags: +D8MIAMS

This is needed for our Lab. Tagging D8MIAMS.

webflo’s picture

Status: Needs work » Needs review
Issue tags: +Amsterdam2014
FileSize
17.65 KB
+++ b/core/includes/install.core.inc
@@ -1539,70 +1540,129 @@ function install_profile_themes(&$install_state) {
+  if (count($languages) > 1 || !isset($languages['en'])) {
+    $operations[] = array('_install_prepare_import', array(array_keys($languages), $install_state['server_pattern']));
+
+    // Set up a batch to import translations.
+    $projects = array_keys(locale_translation_get_projects());
+
+    foreach ($projects as $project) {
+      foreach ($languages as $langcode) {
+        if (locale_translation_use_remote_source()) {
+          $operations[] = array('locale_translation_batch_fetch_download', array($project, $langcode));
+        }
+        $operations[] = array('locale_translation_batch_fetch_import', array($project, $langcode, array()));
+      }
+    }
+

locale_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.

k4v’s picture

We found a related issue: https://www.drupal.org/node/2345985: install_import_translation() fails if the language already exists.

Gábor Hojtsy’s picture

@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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@webflo: can you post an interdiff for review?

k4v’s picture

@gabor: I don't think so, in install_import_translation there is no check.

Gábor Hojtsy’s picture

@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?

webflo’s picture

webflo’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • webchick committed 035393d on 8.0.x
    Issue #2336743 by webflo, penyaskito, YesCT, Gábor Hojtsy: Fixed When...
swentel’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Needs work

The last submitted patch, 2336743-languages-profiles-fix-head.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: -sprint