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
- Go to the drupal installer
- Select Spanish
- Go back to the first step of the installer
- 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.
Related Issues
#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
Comment | File | Size | Author |
---|---|---|---|
#67 | 1974048-language-list-67.patch | 4.96 KB | Gábor Hojtsy |
#67 | interdiff.txt | 1.63 KB | Gábor Hojtsy |
#59 | interdiff.txt | 5.89 KB | Gábor Hojtsy |
#59 | 1974048-language-list-59.patch | 4.85 KB | Gábor Hojtsy |
#59 | 1974048-language-list-59-test-only-WILL-FAIL.patch | 1.61 KB | Gábor Hojtsy |
Comments
Comment #1
YesCT CreditAttribution: YesCT commented#1848490-26: Import translations automatically during installation (and some other later comments) might be good background
and
#1804688-125: Download and import interface translations
Comment #1.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #2
lucascaro CreditAttribution: lucascaro commentedAdded steps to reproduce.
Comment #3
lucascaro CreditAttribution: lucascaro commentedwaiting on #1351352: Distribution installation profiles are no longer able to override the early installer screens
Comment #4
bserem CreditAttribution: bserem commentedLike 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.
Comment #5
Sutharsan CreditAttribution: Sutharsan commentedI 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.
Comment #6
Gábor HojtsyShould we get someone on this issue or do you want to take it? It probably needs more code removed than added :D
Comment #6.0
Gábor HojtsyUpdated issue summary.
Comment #7
Sutharsan CreditAttribution: Sutharsan commentedAdded #1386394: Add back the ability for install profiles (or at least distros) to pre-select a language or modify the language-selection screen as a related issue to the summary.
Comment #8
bserem CreditAttribution: bserem commentedIn reply to #6, I could give it a try, if one could point me to the right folder inside drupal...
Comment #9
bserem CreditAttribution: bserem commentedSo, 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?
Comment #10
Sutharsan CreditAttribution: Sutharsan commented@bserem, thanks for stepping up. But I don't agree with the direction.
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?
Comment #11
bserem CreditAttribution: bserem commented@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.
Comment #12
Sutharsan CreditAttribution: Sutharsan commentedI 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.
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.
Comment #13
bserem CreditAttribution: bserem commentedSo 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 :)
Comment #14
Gábor HojtsyI 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.
Comment #15
bserem CreditAttribution: bserem commentedWell 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.
Comment #16
Gábor HojtsyI think this looks fine with me.
Comment #17
bserem CreditAttribution: bserem commentedIf you all agree then we can close this issue (not me of course). I'll go check some other issue.
Comment #18
penyaskitoI'm testing and reviewing this.
Comment #19
penyaskitoOK, 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.
Comment #20
penyaskitoWorking myself on the patch.
Comment #21
penyaskitoIn the patch attached, help is shown even when returning back to the installer.
Comment #22
bserem CreditAttribution: bserem commentedOh, 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.
Comment #24
Sutharsan CreditAttribution: Sutharsan commentedWe should sort the $select_options based on their key. Otherwise the file added languages will end up at the bottom of the list.
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
Comment #25
Sutharsan CreditAttribution: Sutharsan commentedAs per #24
Comment #26
Sutharsan CreditAttribution: Sutharsan commentedAs per #24
Comment #27
Sutharsan CreditAttribution: Sutharsan commentedComment #28
Sutharsan CreditAttribution: Sutharsan commentedNeeds review
Comment #32
jibranRestoring tags.
Comment #33
bserem CreditAttribution: bserem commented@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?
Comment #34
Sutharsan CreditAttribution: Sutharsan commented@bserem, then you need to test this with a language which is not in the standard language list (LanguageManager::getStandardLanguageList()).
Comment #35
Gábor HojtsyThat 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.
Comment #36
Sutharsan CreditAttribution: Sutharsan commentedIt will be listed in the list with its language code. See screenshot where
drupal-8.0.foobar.po
was added tosites/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.
Comment #37
Sutharsan CreditAttribution: Sutharsan commentedComment #38
barraponto CreditAttribution: barraponto commentedIs that order algorithm case-sensitive? What happens with drupal-8.0.Foobar.po?
Comment #38.0
barraponto CreditAttribution: barraponto commentedUpdated issue summary.
Comment #39
penyaskitoNeeds reroll.
Comment #40
penyaskitoRerolled. Now the affected code is on
Drupal/Core/Installer/Form/SelectLanguageForm
.Comment #42
penyaskitoIt's interesting that a form named SelectLanguageForm didn't need Core\Language before :-P
Comment #44
Gábor HojtsyWell, 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
Comment #45
penyaskitoOops!
Comment #46
Sutharsan CreditAttribution: Sutharsan commentedThe
$browser_options[] = $langcode;
may add duplicate browser options as the array keys will be unique. Use langcode as key here.Comment #47
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedJust did following change in this patch as mentioned in #46.
[DONE]
Comment #48
Gábor HojtsyTried 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.Comment #49
Gábor HojtsyWell, @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.
Comment #50
Gábor HojtsyWe 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
Comment #51
Gábor HojtsyAs for how I would test this:
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?
Comment #52
vijaycs85Fixed the help text to appear on all language except those available locally. Tested manually but missing test coverage.
Comment #53
Sutharsan CreditAttribution: Sutharsan commented#50 is what we agreed on in IRC. I do not agree the extra state conditions added in this patch.
Comment #54
vijaycs85ok, updated.
Reg: #51, not sure how to add new language into this list. however managed to test the list with code InstallerTestBase::setUp()
Comment #57
vijaycs85Updating main patch...
Comment #58
Gábor HojtsyWhy 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?
Comment #59
Gábor HojtsyHere 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.
Comment #60
Gábor HojtsyBTW 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.
Comment #62
Gábor HojtsyLooking 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.
Comment #63
Gábor Hojtsy#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 :)
Comment #64
vijaycs85Small improvement on these asserts would be:
Basically this will improve 1) coverage for all languages 2) we don't need to change anything when the format of getStandardLanguageList change.
Comment #65
Gábor Hojtsy@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.
Comment #66
vijaycs85Yay! #2323259: Local translation file detection is not semantic version compatible is in...
Comment #67
Gábor HojtsyUpdated test with new version number and suggestions in #64 adapted to actual data structures.
Comment #68
vijaycs85#67 looks good and issue summary is up to date. Let's get this in.
Comment #70
webchickI'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!
Comment #71
Gábor Hojtsy@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.
Comment #72
Gábor HojtsyUsability++ :)