Problem/Motivation

When you invoke the installer, it shows a list of ~100 languages to pick from. When you pick, the translation file is downloaded, and you go over to profile selection page. Now you realize you clicked on the wrong item and understand no word of the installer. You decide to go back in your browser and select a different language, but it is not anymore possible. When local files are found, only those are listed in the dropdown.

Although this sounds like an obscure use-case I ran into it multiple times in testing the installer, and I think it is confusing.

Steps to reproduce

  1. Go to the drupal installer
  2. Select Spanish
  3. Go back to the first step of the installer
  4. Note how Spanish and English are the only two options and you can't install Drupal in a different language anymore.

Proposed resolution

Display all the languages at all times. If a local language file for a language that is not in the predefined language list is found, add it to the available language list. Add tests.

Remaining tasks

Review. Commit.

User interface changes

All languages are shown in the installer selector when going back from a state that downloaded files.

API changes

None.

#1351352: Distribution installation profiles are no longer able to override the early installer screens
#1386394: Add back the ability for install profiles (or at least distros) to pre-select a language or modify the language-selection screen

CommentFileSizeAuthor
#67 1974048-language-list-67.patch4.96 KBGábor Hojtsy
#67 interdiff.txt1.63 KBGábor Hojtsy
#59 interdiff.txt5.89 KBGábor Hojtsy
#59 1974048-language-list-59.patch4.85 KBGábor Hojtsy
#59 1974048-language-list-59-test-only-WILL-FAIL.patch1.61 KBGábor Hojtsy
#57 1974048-language-list-57.patch8.57 KBvijaycs85
#57 1974048-language-list-54.patch8.57 KBvijaycs85
#54 1974048-language-list-54.patch8.62 KBvijaycs85
#54 1974048-language-list-54-test-only.patch5.34 KBvijaycs85
#52 1974048-diff-47-52.txt2.63 KBvijaycs85
#52 1974048-language-list-52.patch3.61 KBvijaycs85
#47 drupal8-limited-languages-1974048-47.patch1.72 KBer.pushpinderrana
#47 interdiff-1974048-45-47.txt726 byteser.pushpinderrana
#45 1974048-limited-languages.interdiff.42-45.txt1.06 KBpenyaskito
#45 1974048-limited-languages-45.patch1.67 KBpenyaskito
#42 1974048-limited-languages.interdiff.40-42.txt554 bytespenyaskito
#42 1974048-limited-languages-42.patch1.97 KBpenyaskito
#40 1974048-limited-languages-40.patch1.72 KBpenyaskito
#37 1974048-37.png118.79 KBSutharsan
#25 language-selector-1974048-25.patch1.7 KBSutharsan
#25 interdiff-1974048-22-25.patch2.17 KBSutharsan
#22 language-selector-1974048-22.patch3.05 KBbserem
#21 language-selector-1974048.15-21.interdiff.txt1.22 KBpenyaskito
#21 language-selector-1974048-21.patch2.67 KBpenyaskito
#15 drupal-language-selector-1974048-7886473.patch2 KBbserem
#11 drupal-language-selector-1974048-7885605.patch3.21 KBbserem
#9 drupal-language-selector-1974048.patch3.26 KBbserem
#9 1974048 - default.jpg96.62 KBbserem
#9 1974048 - local files.jpg105.34 KBbserem
#9 1974048 - list open.jpg107.28 KBbserem
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

YesCT’s picture

Issue summary: View changes

Updated issue summary.

lucascaro’s picture

Added steps to reproduce.

lucascaro’s picture

bserem’s picture

Like Gabors proposal, I also believe that all languages should be displayed when the installation is run. No matter if a local file is available.

I believe that, in the Drupal community, it is possible for some people to share a distribution (officialy through d.o or even unofficially) with language files in it. This however shouldn't stop people from testing the distro in their native language.

If that was the case, the installer should also hide English from the list, an approach that I do not find appealing.

Sutharsan’s picture

I agree with Gabor's proposal to have the installer show all selectable languages regardless of available local files.

Additionally distributions should have the possibility to limit the choice of languages, for example if the distribution is targeted at specific countries and/or languages. Limiting the choice to one language should result in skipping the language selection step.

I am not sure if there is still a requirement to provide translations with the distribution. If so, the distro should have option to disable the download of translations. This would give the distro the option to fully control its (installation) language experience.

Side note: currently the 'download_translation' setting in the installer is broken.

Gábor Hojtsy’s picture

Should we get someone on this issue or do you want to take it? It probably needs more code removed than added :D

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

Sutharsan’s picture

bserem’s picture

In reply to #6, I could give it a try, if one could point me to the right folder inside drupal...

bserem’s picture

Assigned: Unassigned » bserem
Status: Active » Needs review
FileSize
107.28 KB
105.34 KB
96.62 KB
3.26 KB

So, with some help here in the Drupalcon Sprint, I present a patch and explain the logic behind it.

The attached screenshots show the behavior after the patch is applied.
First: Default state, with no local language files (with a bit of changes in the help text).
Second: Local files are available, extra text is being displayed and they have a * in their name.
Third screenshot: The list of languages open, with local languages being marked with a star.

ps: I do not know the proper procedure, do I assign this to myself on no?

Sutharsan’s picture

Status: Needs review » Needs work

@bserem, thanks for stepping up. But I don't agree with the direction.

+++ b/core/includes/install.core.inc
@@ -1592,10 +1592,18 @@ function install_select_language_form($form, &$form_state, $files = array()) {
+  $local_message = '<p>Local translations were found and are marked with a " * " (star sign) in front of the corresponding language. ' .
+      'They will be used instead of remote ones.</p>';

Do we really want to bother people with this? Remember this is the first time experience with Drupal. Users will see this if
1. they have selected a language before and managed to return to the first installation step. There is no link in the normal process that takes them back. Either they know what they are doing or they are confused. This new text will probably make it worse for them. So, please don't!
2. They have an installation profile which comes with default languages but allows users to select other languages too. They will not be familiar with the concepts 'remote' and 'local'. And why would they bother?

bserem’s picture

@Sutharsan do you disagree with the explanation text or with the whole idea of informing people about local files?
Maybe the attached patch is better when it comes to new people and their first installation.

In my opinion though, first time Drupal users might try to re-run the installation, so I believe it will be good to notify them somehow.
Same of course applies to veteran users.

I would like some more clarification on the matter, so that I can get the logic right.

Sutharsan’s picture

I disagree with bothering first time users with minor important stuff. The "*" and the explanation is noise which should be prevented. So, indeed, I disagree with informing people about this.

... first time Drupal users might try to re-run the installation, ...

I doubt if first time users will regularly re-run the installation. And if they do, they are probably in trouble. We should not give them noise, but clear information which leads them to a good result.

bserem’s picture

So your proposal for the issue is to completely skip the *, and not mention to the end user whether a local file or a remote file is going to be used. Am I right?
This is actually quite easy to be done!

I would of course like to hear other peoples ideas to, so that I can produce a patch that most of us will find acceptable :)

Gábor Hojtsy’s picture

I agree its better to skip the whole identification of local stuff and just skip the *. I think its a real possibility that someone accidentally picks the wrong language, hits the back button and then would like to pick something else.

bserem’s picture

Status: Needs work » Needs review
FileSize
2 KB

Well then, things were way simpler. The attached patch doesn't inform the user about local translation files.
It will however continue with the local file if a user selects a language that has a local file!
If you believe this should be removed please let me know.

Gábor Hojtsy’s picture

I think this looks fine with me.

bserem’s picture

If you all agree then we can close this issue (not me of course). I'll go check some other issue.

penyaskito’s picture

I'm testing and reviewing this.

penyaskito’s picture

Status: Needs review » Needs work

OK, reviewed the patch and looks great. However, there is something that still could confuses some users.

As @Sutharsan pointed, users that come back to the first step in the installer are probably really confused, and we are removing the help text for them!

I would suggest that we set $form['help'] no matter of the $files variable.

penyaskito’s picture

Assigned: bserem » penyaskito

Working myself on the patch.

penyaskito’s picture

Assigned: penyaskito » Unassigned
Status: Needs work » Needs review
FileSize
2.67 KB
1.22 KB

In the patch attached, help is shown even when returning back to the installer.

bserem’s picture

Oh, I must have missed the if(empty($files)) line on the last patch... Thanks penyaskito!

Sidenote, while we are at it: since we out all this effort for first time users. Shouldn't the help text be altered from "if you don't want this" to something like "english is included" ?
After all, we do not care about downloading the language anymore, since we do not display a list of already downloaded languages.

Status: Needs review » Needs work

The last submitted patch, language-selector-1974048-22.patch, failed testing.

Sutharsan’s picture

+++ b/core/includes/install.core.inc
@@ -1563,8 +1563,15 @@ function install_select_language_form($form, &$form_state, $files = array()) {
+  // Select lists based on all standard languages.
+  foreach ($standard_languages as $langcode => $language_names) {
+    $select_options[$langcode] = $language_names[1];
+    $browser_options[$langcode] = new Language(array(
+      'id' => $langcode,
+    ));
+  }
+  // Select lists based on available language files.
   if (count($files)) {
-    // Select lists based on available language files.
     foreach ($files as $langcode => $uri) {
       $select_options[$langcode] = isset($standard_languages[$langcode]) ? $standard_languages[$langcode][1] : $langcode;
       $browser_options[$langcode] = new Language(array(

We should sort the $select_options based on their key. Otherwise the file added languages will end up at the bottom of the list.

+++ b/core/includes/install.core.inc
@@ -1591,13 +1588,11 @@ function install_select_language_form($form, &$form_state, $files = array()) {
-
-  if (empty($files)) {
-    $form['help'] = array(
-      '#markup' => '<p>Translations will be downloaded from the <a href="http://localize.drupal.org">Drupal Translation website</a>. ' .
-      'If you do not want this, select <em>English</em> and continue the installation. For more information, see the <a href="http://drupal.org/documentation/install">online handbook</a>.</p>',
-    );
-  }
+  $form['help'] = array(
+    '#markup' => '<p>Translations will be downloaded from the <a href="http://localize.drupal.org">Drupal Translation website</a>. ' .
+    '<em>English</em> is included in Drupal.</p>' .
+    '<p>For more information, see the <a href="http://drupal.org/documentation/install">online handbook</a>.</p>',
+  );

This does not help the problem of this issue. Pls remove.

@bserem, please add interdiffs to you patch. This helps reviewers to find those parts recently changed. https://drupal.org/node/1488712

Sutharsan’s picture

Sutharsan’s picture

As per #24

Sutharsan’s picture

Status: Needs work » Needs review
Sutharsan’s picture

Needs review

Status: Needs review » Needs work

The last submitted patch, interdiff-1974048-22-25.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

Restoring tags.

bserem’s picture

@sutharsan: File added languages don't end up at the bottom with any of my patches (in opera, firefox, chromium at least). They appear where they should based on alphabetical sorting.

Do you experience a different behavior?

Sutharsan’s picture

@bserem, then you need to test this with a language which is not in the standard language list (LanguageManager::getStandardLanguageList()).

Gábor Hojtsy’s picture

That would happen in the edge case when a local language file was available in a langcode that is not listed in the standard list of languages.

Sutharsan’s picture

It will be listed in the list with its language code. See screenshot where drupal-8.0.foobar.po was added to sites/default/files/translations.

Perhaps we should change this. Now we have only langcode, it is both used as langcode and as displayble language.

Current patch:
File: drupal-8.0.fb.po
Langcode: fb
Display: fb

Alternative 1:
File: drupal-8.0.fb.po
Langcode: fb
Display: Language: fb

Alternative 2:
File: drupal-8.0.fb_FooBar.po
Langcode: fb
Display: FooBar
Can be combined with Alternative 1 as fallback.

Sutharsan’s picture

FileSize
118.79 KB

1974048-37.png

barraponto’s picture

Is that order algorithm case-sensitive? What happens with drupal-8.0.Foobar.po?

barraponto’s picture

Issue summary: View changes

Updated issue summary.

penyaskito’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs reroll.

penyaskito’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +sprint
FileSize
1.72 KB

Rerolled. Now the affected code is on Drupal/Core/Installer/Form/SelectLanguageForm.

Status: Needs review » Needs work

The last submitted patch, 40: 1974048-limited-languages-40.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
1.97 KB
554 bytes

It's interesting that a form named SelectLanguageForm didn't need Core\Language before :-P

Status: Needs review » Needs work

The last submitted patch, 42: 1974048-limited-languages-42.patch, failed testing.

Gábor Hojtsy’s picture

+++ b/core/lib/Drupal/Core/Installer/Form/SelectLanguageForm.php
@@ -45,21 +46,21 @@ public function buildForm(array $form, FormStateInterface $form_state, $install_
+      $browser_options[$langcode] = new Language(array(
+        'id' => $langcode,
+      ));
...
-        $browser_options[] = $langcode;

Well, the reason is there is no actual language objects should be involved.... The code you removed used langcodes, the code you added uses Language objects. Not good :D

All the fails are

strtolower() expects parameter 1 to be string, object given Warning in UserAgent.php on line 115 in Drupal\Component\Utility\UserAgent::getBestMatchingLangcode()
penyaskito’s picture

Status: Needs work » Needs review
FileSize
1.67 KB
1.06 KB

Oops!

Sutharsan’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Installer/Form/SelectLanguageForm.php
@@ -45,21 +45,19 @@ public function buildForm(array $form, FormStateInterface $form_state, $install_
+    // Select lists based on all standard languages.
+    foreach ($standard_languages as $langcode => $language_names) {
+      $select_options[$langcode] = $language_names[1];
+      $browser_options[$langcode] = $langcode;
+    }
+    // Add languages based on language files in the translations directory.
     if (count($files)) {
-      // Select lists based on available language files.
       foreach ($files as $langcode => $uri) {
         $select_options[$langcode] = isset($standard_languages[$langcode]) ? $standard_languages[$langcode][1] : $langcode;
         $browser_options[] = $langcode;
       }
     }

The $browser_options[] = $langcode; may add duplicate browser options as the array keys will be unique. Use langcode as key here.

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
726 bytes
1.72 KB

Just did following change in this patch as mentioned in #46.

The $browser_options[] = $langcode; may add duplicate browser options as the array keys will be unique. Use langcode as key here.

[DONE]

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Tried this one. Works well, except, the download help will not appear if you go back to this screen. That is wrapped in a if (empty($files)) { condition. Ideally the #states specification on it would include all the language codes which are downloaded already, so those would not show the help but the rest would. Not sure if #states supports multiple alternate options like that? If not, at least exluding English like before but still showing help for the rest is better than no help.

Gábor Hojtsy’s picture

Well, @penyaskito points out that Sutharsan said in #24 that the help text changes are unrelated, although @penyaskito suggested in #19 the same as I did in #48, to show the help text. I think the help text not showing is very much in scope here and should be resolved. If we are showing the help text the same way as if there was no local file, I think that is better than not showing it. Ideally the help text would show for language codes which are not already present locally, but that looks like may require some custom JS. Not sure that should be done here, that may result in the whole bug not being resolved, so I'd rather resolve the core of the bug.

Gábor Hojtsy’s picture

We have had some more discussion about this in IRC and the intention of the message is not agreed on among us. Better for now to just remove the files condition and display it for all languages. We can still argue about it later on once this is fixed. :D

Gábor Hojtsy’s picture

Issue tags: +Needs tests

As for how I would test this:

  1. Create a new test subclassed from InstallerTestBase
  2. Override the setUpLanguage() method, create dynamic .po files (they can be empty) in the conf_path() . '/files/translations' dir and test with drupalGet() that they showed up on the page

I would suggest adding a .po file in a totally uknown language, eg. xx-drupal-test to see that it was added to the list. Not a known language that is there anyway.

Does that make sense?

Sutharsan’s picture

#50 is what we agreed on in IRC. I do not agree the extra state conditions added in this patch.

vijaycs85’s picture

Issue tags: -Needs tests
FileSize
5.34 KB
8.62 KB

#50 is what we agreed on in IRC. I do not agree the extra state conditions added in this patch.

ok, updated.

Reg: #51, not sure how to add new language into this list. however managed to test the list with code InstallerTestBase::setUp()

Status: Needs review » Needs work

The last submitted patch, 54: 1974048-language-list-54.patch, failed testing.

The last submitted patch, 54: 1974048-language-list-54-test-only.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
8.57 KB
8.57 KB

Updating main patch...

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Why do we even go back in the installer? All we want to know is if there is a custom .po file, all the predefined languages are still shown and the extra .po file also shows up as an option to choose, no? That sounds like much less to test?

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.61 KB
4.85 KB
5.89 KB

Here is a much simpler patch. It uses the installer test added above but only overrides the language setup step. Saves a custom .po file from that step and ensures it shows up on the page. It also asserts some predefined languages to make sure both showed up. Attached test only version as well to show the pre-patch behaviour is different.

Gábor Hojtsy’s picture

BTW as can be seen from the test, we need to create old non-semantic version translation file so it gets detected. I opened #2323259: Local translation file detection is not semantic version compatible for that bug. If there are no other tests for that (ie. that does not fail in any way), we need to fold that here, so we have a test accompanying the file detection.

The last submitted patch, 59: 1974048-language-list-59-test-only-WILL-FAIL.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes

Looking at #1351352: Distribution installation profiles are no longer able to override the early installer screens it does not seem feasible to make this feature configurable from the profile at this time, it is not looked up / loaded at the time of language selection. Such a feature may be added at a later time, even in 8.1.0, so not concerned about that problem. IMHO ready to review to get in as-is. From all the people who worked on this issue, looks like Sutharsan or vijaycs85 would be eligible to RTBC. There was almost none left of vijaycs85's test and last time Sutharsan posted a patch on this issue was a year ago.

Gábor Hojtsy’s picture

Status: Needs review » Postponed

#2323259: Local translation file detection is not semantic version compatible is now fully covered, so would be good to get that in first, so we don't need to add incorrect version numbers with this test. Please review ASAP :)

vijaycs85’s picture

+++ b/core/modules/system/src/Tests/Installer/InstallerLanguagePageTest.php
@@ -0,0 +1,46 @@
+    $this->assertRaw('>Afrikaans<');
+    $this->assertRaw('>ქართული ენა<');
+    $this->assertRaw('>हिन्दी<');
+    $this->assertRaw('>xoxo<');

Small improvement on these asserts would be:

    foreach (LanguageManager::getStandardLanguageList() as $langcode => $name) {
      $this->assertOption('edit-langcode', $langcode);
      $this->assertRaw($name);
    }

Basically this will improve 1) coverage for all languages 2) we don't need to change anything when the format of getStandardLanguageList change.

Gábor Hojtsy’s picture

@vijaycs85: I think that is a great idea, although I loved to include Hindi verbatim in a test finally :) Oh well! Will update once #2323259: Local translation file detection is not semantic version compatible lands.

vijaycs85’s picture

Gábor Hojtsy’s picture

Updated test with new version number and suggestions in #64 adapted to actual data structures.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

#67 looks good and issue summary is up to date. Let's get this in.

  • webchick committed 7d58841 on 8.0.x
    Issue #1974048 by Gábor Hojtsy, vijaycs85, er.pushpinderrana, Sutharsan...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

I've also run into this a few times myself, and been confused each time. I agree with the fix; to a user, they don't really need to care whether a translation already exists or whether it needs to be downloaded; the download only represents a few seconds delay.

The only use case this potentially screws is, let's say the entire German government decided to adopt Drupal (one can dream). They might want to create a
"Gov.de" distro to distribute among their various web properties. There's only one language to ever pick: German. By shipping with a German translation, they'd avoid the sprawling morass of possible options on this form and get straight to business.

However, they could also just as easily pick "German" from the big list. :P Like they do in every other localized program (e.g. OSX). So...

Committed and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

@webchick: yeah the point here is that the pre-patch behavior was logical for a distro shipping with languages but not for Drupal downloading files and going back. We have #1386394: Add back the ability for install profiles (or at least distros) to pre-select a language or modify the language-selection screen to let distros specify any limitations to the language list.

Gábor Hojtsy’s picture

Issue tags: -sprint

Usability++ :)

Status: Fixed » Closed (fixed)

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