Add back the ability for install profiles (or at least distros) to pre-select a language or modify the language-selection screen

#1351352: Distribution installation profiles are no longer able to override the early installer screens is still really important.

This should be resolved through the new architecture provided by #1351352: Distribution installation profiles are no longer able to override the early installer screens

Thus, postponing on that.

Postponing issues were fixed.

I was looking at https://www.drupal.org/core/d8-allowed-changes#evaluate
while working on major triage

but I dont know what a new solution would look like here. so I dont know how to evaluate it for rc or being postponed.

I'm trying to look at this at the BADCamp sprint.

After battling updating php on my old computer, I have made some progress on this. Here are my notes:

1. You can pass in the langcode right now into the URL /core/install.php?langcode=fr (I confirmed this)
2. You can also pass the langcode right now through the command-line though I didn't try to do that
3. I think it makes sense to pass the langcode in just like the name is passed into the yml file like:
   

   distribution:
     name: Multilingual demo
     langcode: fr
   

4. If it's passed into the yml file like this, then we can add something like this to install_begin_request after the profile is read in:
   

   if (!empty($install_state['profile_info']['distribution']['langcode'])) {
     $install_state['parameters']['langcode'] = $install_state['profile_info']['distribution']['langcode'];
   }
   

This feels like a good approach to me but, before I put together a patch, I would like thoughts on this.

I haven't yet changed the list of langcodes in the installer form but I imagine this is doable in hook_form_install_configure_form_alter. I'll play with that next.

I'm looking at note 2 in my comment #10, perhaps I didn't understand the comments and it can't be passed in via command-line (unless it's "in the URL"). The comments that made me think that was an option were:

Non-interactive Drupal installations can override some of these default settings by passing in an array to the installation script, most notably 'parameters' (which contains one-time parameters such as 'profile' and 'langcode' that are normally passed in via the URL) 

in install_state_defaults.

@YesCT looked at #10 and #11 and thought that all is ok to go forward with a patch based on the 3 and 4 notes from #10. Would love some other feedback as well.

As for modifying the langcode form field in the installer, this should be easy to do in hook_form_install_configure_form_alter but it's not working. I tried in the multilingual_demo installer as well as the openchurch. For both of these, the form alter hook doesn't appear to be firing. But, I don't have PhpStorm running properly on my old computer and just doing print_r... I did try putting in die(); and this didn't kill the install so something is whacky.

Ok, I was being dense before and was trying to update the hook_form_install_configure_form_alter function which is the last form on the screen.

This works fine:

/**
 * Implements hook_form_FORM_ID_alter() for install_select_language_form.
 */
function multilingual_demo_form_install_select_language_form_alter(&$form, FormStateInterface $form_state) {
  // Updates the language options.
  $form['langcode']['#options'] = array(
      'fr' => 'My French',
      'de' => 'My German',
      'hu' => 'My Hungarian',
      );
}

So, updating the existing list of languages is easy peasy.

I have a patch for supporting adding langcode to the distribution's .info.ymlfile like:

distribution:
  name: This is the profile name
  langcode: fr

To test it, download a distribution like Open Church and update the .info.yml to add this text. Then run through the installer and it should choose the language you specified by default and then bypass that part of the install (skips to the second step).

Actually, #14 has an error... adding a new patch in a bit.

Ok, here's a new patch.

The things that need to be tested:

1. Test 1 - No langcode passed into installer via URL and no langcode in distribution .info.yml file
   • Language select form should show up
2. Test 2 - No langcode passed into installer via URL and langcode in distribution .info.yml file
   • Language select form should show not up and langcode should default to langcode in the .info.yml file
3. Test 3 - langcode passed into installer via URL and no langcode in distribution .info.yml file
   • Language select form should show not up and langcode should default to langcode in the URL
4. Test 4 - langcode passed into installer via URL and langcode in distribution .info.yml file
   • Language select form should show not up and langcode should default to langcode in the .info.yml file

Is it possible to do automated tests for this? It seems to me like it is.

Yay, this makes a whole lot of sense. I like this a lot.

I will see if I can allocate some time to write some tests.

One (minor) note on the patch:

+++ b/core/includes/install.core.inc
@@ -440,6 +440,20 @@ function install_begin_request($class_loader, &$install_state) {
+    if (!isset($install_state['parameters']['langcode']) || $install_state['parameters']['langcode'] != $default_langcode) {
+      // If the default language changed, update it.
+      if (isset($install_state['parameters']['langcode'])) {
+        $default_language = new Language(array('id' => $default_langcode));
+        $container->get('language.default')->set($default_language);
+        \Drupal::translation()->setDefaultLangcode($default_langcode);
+      }
+      $install_state['parameters']['langcode'] = $default_langcode;
+    }

I find the logic here to be a little bit unintuitive. Especially because the comment If the default language changed, update it. comes before a condition which does not check whether anything changed. I understand that it effectively does because of the previous condition, but I still think it's confusing (at least to me). Also I think not always setting $install_state['parameters']['langcode'] is an unnecessary optimization. I would propose something like the following for this part:

$install_state['parameters']['langcode'] = $default_langcode;
// If the default language changed, update it.
if (isset($install_state['parameters']['langcode']) && ($install_state['parameters']['langcode'] !== $default_langcode)) {
  ...
}

EDIT: Fix code formatting

Thanks for the feedback @tstoeckler! I also felt it was not as elegant as I'd like but your suggestion:

<?php
$install_state['parameters']['langcode'] = $default_langcode;
// If the default language changed, update it.
if (isset($install_state['parameters']['langcode']) && ($install_state['parameters']['langcode'] !== $default_langcode)) {
  ...
}
?>

would mean that the if statement won't fire, right? Because before it the $install_state['parameters']['langcode'] is set to $default_langcode and then it's checking if it's not equal.

What I would prefer would be to move the entire code block:

  // Set the default language to the selected language, if any.
  if (isset($install_state['parameters']['langcode'])) {
    $default_language = new Language(array('id' => $install_state['parameters']['langcode']));
    $container->get('language.default')->set($default_language);
    \Drupal::translation()->setDefaultLangcode($install_state['parameters']['langcode']);
  }

to below this new code and then we can yank out the part that is doing the setDefaultLangcode and it will simplify things. But, I'm not clear if that will mess anything up. Perhaps I should just try it. :)

1. yes, you are right. I am stupid

2. that sounds like a *great* idea!

It seems to work great for me. I removed the first setDefaultLangcode logic and moved it after the profiles are pulled in this is the code:

  // Default the language based on profile configuration, if applicable.
  if (isset($install_state['profile_info']['distribution']['langcode'])) {
    $install_state['parameters']['langcode'] = $install_state['profile_info']['distribution']['langcode'];
  }

  // Set the default language to the selected language, if any.
  if (isset($install_state['parameters']['langcode'])) {
    $default_language = new Language(array('id' => $install_state['parameters']['langcode']));
    $container->get('language.default')->set($default_language);
    \Drupal::translation()->setDefaultLangcode($install_state['parameters']['langcode']);
  }

Thoughts? Should I make a patch with this instead? Anyone think of a reason why that setDefaultLangcode logic needs to before the profiles are read in?

#23 You are not stupid. :) See #24. Would love your input on the logic move.

@tstoeckler gave the thumbs up so here's a new patch. He's hoping to write tests tomorrow.

Yeah, I looked at the surrounding code and nothing even remotely language-related is happening between those two spots in the code, so it's totally harmless to move the hunk a little bit down. Will write some tests later, hopefully.

Here we go.

Not quite sure whether this passes, because I had some filesystem problems locally, but at least in terms of coverage this should be sufficient. I've added 3 tests extending InstallerTestBase for cases 2.-4. described in #19. (Thanks for that, by the way, that was incredibly useful!) 1. is already covered by InstallerTranslationTest.

Hurray! Thanks for writing these. :)

Minor things:

1. +++ b/core/modules/system/src/Tests/Installer/DistributionProfileTranslationQueryTest.php
   @@ -0,0 +1,136 @@
   +  protected function setUp() {
   

   Needs doc block?

2. +++ b/core/modules/system/src/Tests/Installer/DistributionProfileTranslationQueryTest.php
   @@ -0,0 +1,136 @@
   +    // profile. This distribution language should still be used.
   +    // The unrouted URL assembler does not exist at this point, so we build the
   

   Minor nitpick: Move some of "The unrouted..." to the previous line?

3. +++ b/core/modules/system/src/Tests/Installer/DistributionProfileTranslationTest.php
   @@ -0,0 +1,132 @@
   +  protected function setUp() {
   

   Needs doc block?

For the last test, shouldn't the langcode passed in the URL be something other than 'de'? That way we know it was overridden with the yml file value. Or, maybe I'm not reading it right?

1.+3. will fix

2. There are two distinct points being made here, so I went with the explicit line break. No biggie though, if you think it should wrap nonetheless.

For the last test, shouldn't the langcode passed in the URL be something other than 'de'? That way we know it was overridden with the yml file value. Or, maybe I'm not reading it right?

All the different tests are a bit confusing, so please do make sure that I'm not mistaken, but I think what you're referring to is #19.4 which is covered by DistributionProfileTranslationQueryTest.

Just noticed the following, as well:

+++ b/core/modules/system/src/Tests/Installer/DistributionProfileTranslationQueryTest.php
@@ -0,0 +1,136 @@
+ * Contains \Drupal\system\Tests\Installer\DistributionProfileTest.
...
+class DistributionProfileTranslationQueryTest extends InstallerTestBase {

+++ b/core/modules/system/src/Tests/Installer/DistributionProfileTranslationTest.php
@@ -0,0 +1,132 @@
+ * Contains \Drupal\system\Tests\Installer\DistributionProfileTest.
...
+class DistributionProfileTranslationTest extends InstallerTestBase {

+++ b/core/modules/system/src/Tests/Installer/InstallerTranslationQueryTest.php
@@ -0,0 +1,90 @@
+ * Contains \Drupal\system\Tests\Installer\InstallerTranslationTest.
...
+class InstallerTranslationQueryTest extends InstallerTestBase {

Boo! (Copy-paste much?)

Here we go.

I'm going to review this now. [edit: and then had to stop. will come back to this later, if no-one beats me too it. but just wanted to make sure it was clear for other reviewers also.]

Looks good for me. Thanks for the reviews folks :) Moving to 8.1.x for inclusion.

Committed/pushed to 8.1.x, thanks!

Yay, thanks!

This issue is missing a change record. (We should add change records for noteworthy API additions.)

Published https://www.drupal.org/node/2709451.