Multilingual functionality - enable English and Spanish as part of Umami installation

@shaal,

I tested this patch and can confirm it works. I also started looking into the other issue about the warning message. This happens because of when the "content_translation" module is installed, there is only one language setup from step 1 of the install process. So when the module's "hook_install()" is triggered, it checks for more then one language and sets this warning if not. All warnings triggered stay on the screen of the profile page.

I dug around and found a ticket about this issue Create multilingual install profile which makes it sound like a lot needs to change to make this correctly possible. Starting with making a site multilingual during step 1 down to the more complex making all entities, fields, etc... translatable. There's a lot of talk in the ticket about this being an improvement for Drupal 9.

It looks like before this task can be accomplished, the install process needs to be changed along with a long list of other things.

+++ b/core/profiles/demo_umami/demo_umami.info.yml
@@ -42,6 +42,10 @@ install:
+  - config_translation
+  - content_translation
+  - locale
+  - language

To fix the issue you see in the installer, just swap the order of language to be first and then the others. Best in this order:

- language
- locale
- config_translation
- content_translation

In this case, when content translation gets enabled, the languages will already be there, so it will not display that warning.

Even then, the config translations (config/install/languages/*/*) will not get automatically imported. That just does not work in core. See https://cgit.drupalcode.org/multilingual_demo/tree/multilingual_demo.pro... which is almost entirely needed to have the config translations imported. Except the first function.

@mcannon I would not wait for the installer to be all rosy and great, that should not block demonstrating what Drupal can do. Just lift the code needed and run with it IMHO :)

Looks like some layout builder changes crept in. Also:

+++ b/core/profiles/demo_umami/demo_umami.install
@@ -34,6 +34,9 @@ function demo_umami_requirements($phase) {
+  // Ensure the translation fields are created in the database.
+  \Drupal::service('entity.definition_update_manager')->applyUpdates();

This will only be needed once you start shipping with field config files that specify the fields as translatable, eg. https://cgit.drupalcode.org/multilingual_demo/tree/config/install/field.... and I am not sure it is still needed anyway then.

Note as discussed following the initiative call, it wasn't the original intention to translate the Drupal user interface to Spanish in this initial scope of work - it was merely about demonstrating Drupal's content translation. That may not have been apparent.

We need to work out whether it is now more or less work to proceed with the UI also translated and if the apparent additional install time is acceptable.

I think its a good idea to make multilingual optional. There are two options:

1. Make two install profiles. Umami $language_user_selected and Umami Multilingual.
2. Umami injects a config screen right after the profile selection screen to pick which option to go with and then does dynamic stuff based on that.

I am not 100% sure it is possible to inject a config screen (AKA installer task) right after profile selection, but it may very well be possible.

Discussed with @shaal. Making two profiles would require #1356276: Allow profiles to define a base/parent profile unfortunately if you don't duplicate the code (which we don't want), so that is indeed, out of the way.

However option 2 can be done with https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension... theoretically. That can inject a config form right after the profile step to ask about multilingual (and any other demo related setup config that should happen before modules get installed). I personally never had a form added this early in the installer, but since the profile selection and the language selection before it are forms, this should work.

If that does not work, the demo installation can be reworked to happen after the existing built-in site config form and then hook_form_install_configure_form_alter() can be used to alter the config form to ask for additional details on that config form.

@shaal: I was reviewing this for differences with the multilingual_demo profile, and while I could not pinpoint the issue that causes content translation to be enabled before the config is imported, I found another issue. The install/languages/* files should be entirely removed from the patch, there is no reason to ship those with the profile, they will get created as translations from localize.drupal.org once the default config is included with Umami. Consequently I realized you don't need the config translation import code in the profile at all either, because once the config is part of Umami's default config, it will be translatable on localize.drupal.org and get translated as long as locale module is enabled, no need to duplicate that heavy lifting.

These will probably not solve the issues experienced but make the patch significantly simpler.

Ok it is true that config import is after modules being installed (see install_tasks()) so whatever order we do that is not going to help. We should fix the content translation module to not inform people about problems when installed as part of installing Drupal, when actual config is not there. If somebody installs the module as part of Drupal's installation, they should know what they are doing.

I think this proof of concept is probably the ugliest variant of the fix, but it should work :D We may be able to get the config storage from the container and see if its an InstallStorage instance or some more elegant looking fix. I looked if there was something in the special installer container that we can just directly query to know if we are in the installer or not, but did not find anything. If there would be an actual API that would be the best.

This will only make the first two warnings go away. The import problem would be fixed by fixing the translations themselves. We can also try to make sure that is not output but that is on locale module then. The import summary is probably also not useful to display as part of the installation I guess, it certainly distracts here.

Anyway, let's see first how/if this fixes content_translation.

Should skip both messages, not just the one that had the condition.

The locale messages actually have an API to not be printed, that is finish_callback => FALSE can be passed into the batch builder and then the finish callback is not added to the import batch (which is otherwise only about printing the messages). This can be used in the "additional translations" process in the installer which is about downloading and importing translations that are for projects enabled in the install process that were not part of core.

The fetch batch process that is used to import the initial translation does have a finish callback that couples printing the messages and updating the fetch timestamp which is unfortunate. We can add our own finish callback to only update the timestamp.

I think these should stop the messages from being shown for all locale import in the installer for all profiles. I *think* this is more in line with what people expect, after all we don't report on the imported config either even though when you explicitly import config then you get to see that reported. The installation is such a complex operation that it is hard to assume that you would be interested specifically about the translation string imports as a big box on the page :) It is a lot of technical detail anyway. I don't know why we made it appear in the installer in the first place.

Ok only the settings.php message remains. I think rather than not calling locale_translate_batch_finished() at all, we should figure out a way for that to be able to log messages but not be noisy about them and make finish_feedback respond to that. Ie. pass into the batch results somehow.

Also @alexpott points out in chat that

Nothing is that nice in the installer - the classic way is to use drupal_installation_attempted()

Which is definitely a nicer way of depending on the global install state :) https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi...

Solve this later one, the former one needs fixing still.

So this is how the result looks now:

I don't think the settings.php message is new to this patch?

Proof of concept for a better batch silencing solution where logs are saved but user feedback is not output. That was/is the documented meaning of the finish_feedback option btw, so that it does not work that way is a bug.

Hm, this fails with

Notice: Undefined index: finish_feedback in locale_translate_batch_finished() (line 392 of core/modules/locale/locale.bulk.inc).
locale_translate_batch_finished(1, Array) (Line: 221)
locale_translation_batch_fetch_finished(1, Array, Array, '5 min 24 sec') (Line: 454)

and

Notice: Undefined index: finish_feedback in locale_translate_batch_finished() (line 402 of core/modules/locale/locale.bulk.inc).
locale_translate_batch_finished(1, Array) (Line: 221)
locale_translation_batch_fetch_finished(1, Array, Array, '5 min 24 sec') (Line: 454)

I don't have time today anymore to track that down, theoretically the results so passing on to @shaal hopefully :)

The test fails are within Umami's own tests, those need resolved as well.

So the problem is we are adding the store_finish_feedback in one of the locale include files and then run this batch from another one :/ Locale import is such a mess. We can add an independent callback there, but it should ideally be actually used in the built-in batch creation in locale as well.

This should work better but is not really done for that reason.

Changing to isset() is not a good idea, that just masks the problem. The problem is that the option does not get stored on the batch so if somebody wanted to display feedback that would not work anymore. So let's not do that!

I opened #3034784: Displaying translation string counts and string errors is too much detail in the installer to break that issue out, since it happens with standard profile and is pre-existing. Let's only deal with the content translation module here which is a problem this issue exposes first in core.

Patch is same as #39 except all locale and install.core.inc changes are removed. (Those are now in #3034784: Displaying translation string counts and string errors is too much detail in the installer).

Trying to fix the config issue but having trouble running tests locally so not 100% sure this will work first time!

New patch coming in a mo - there are a couple of config files that have been added that aren't needed.

I have removed:

• language.entity.zxx.yml
• language.mappings.yml
• locale.settings.yml

These were not needed as part of the proffile, not needed for the installation to work & are added by the language/locale modules when they are enabled.

--- /dev/null
+++ b/core/profiles/demo_umami/config/install/language.entity.en.yml
--- /dev/null
+++ b/core/profiles/demo_umami/config/install/language.negotiation.yml
--- /dev/null
+++ b/core/profiles/demo_umami/config/install/language.types.yml

These are also included as default, why are you including them then if zxx was removable as it was added? That should be true for the others (with keep_english for the English as well).

language.negotiation.yml is required as we have configured that for Spanish so differs from the defaults provided by the module.

I think the other two can be removed - will give it a test!

Cheers!

I was able to remove language.entity.en.yml & language.types.yml.

I think there's a few things that need changing from the default install / this patch (e.g. English not using /en prefix) - but I think they're covered by other issues / we can work on this afterwards. This patch is holding up a lot of other multilingual work, so we might as well get this added & then fine tune if needed.

Fixed merge conflicts + setting to needs review.

Looks good to me other than:

+++ b/core/profiles/demo_umami/demo_umami.info.yml
@@ -42,7 +42,13 @@ install:
   - umami
+  ¶
+keep_english: true

This empty line should be removed.

Fixing the whitespace issue - as that's all, setting back to RTBC.

Triggering testbot again as it looks like it was having a rough time earlier...

Indeed. Very rough time.
dispatcher/testbot issue. back to rtbc.

+++ b/core/modules/content_translation/content_translation.install
@@ -17,6 +17,12 @@ function content_translation_install() {
+  // Skip the guidance messages about enabling translation features if the
+  // module was installed in the Drupal installation process.
+  if (drupal_installation_attempted()) {
+    return;
+  }

This looks like a good approach to not showing the messages during an install. Which would be the wrong time - because they'll show on the site configure form at the end of installing the modules +1.

Superb, thanks for confirming @alexpott. Since that was the only thing I contributed to in this patch, I put this in! Let's continue with making the blocks csv and importing the multilingual content.

For some reason commits did not show up here, it is in git though: https://cgit.drupalcode.org/drupal/commit/?h=8.8.x&id=afbdd37f72f480d477...