Problem/Motivation
There are some problems / inconsistencies related to the path prefix handling.
E.g. in some situations when accessing /, UI is being shown on English instead of the default language. But if /node is accessed, the UI is shown in the default language as expect.
Proposed resolution
Discussions during the D8MI sprint lead to following results:
- If a non-english language is chosen on installation time, English is removed.
The reason for doing so is that we don't want to assume users want a multilingual site - Only the default language can have an empty path prefix.
- If the default language is changed, the path prefix of the new default language is kept, but the path prefix of the former default language is changed to the langcode.
The reason for doing so is to reduce the amount of broken links by changing the default language and we need to comply with 2.
Remaining tasks
- Rewrite Patch
- Change behaviour of installation
- Change behaviour on changing default language
- Test Patch
User interface changes
"After selecting a non-english language on installation English won't be enabled."
API changes
None
Original report by penyaskito
When accessing /, UI is being shown on English instead of the default language. If we access /node, the UI is shown on the default language as we would expect
Steps to reproduce:
- Install a D8 site in a language other than English, with a valid translation in the translations folder which could be imported.
- When going to /, interface is on English. When going to /node, interface is on the installation language.
Comment | File | Size | Author |
---|---|---|---|
#79 | interdiff-68-79.txt | 3.15 KB | das-peter |
#79 | url_prefixes_recreate-1738330-79.patch | 9.77 KB | das-peter |
#78 | url_prefixes_recreate-1738330-78.patch | 9.7 KB | das-peter |
#78 | interdiff-68-78.txt | 2.17 KB | das-peter |
#71 | 59-68.txt | 5.41 KB | Schnitzel |
Comments
Comment #1
Gábor HojtsyHm, that sounds like a bug with path prefix handling. The router rework or some D8MI change might have screwed this up. How come we don't have tests for this?! Mumble, mumble.
Comment #2
jsbalseraComment #3
penyaskitoIt happens in both cases: when installing in other language, or when adding a language after installation and setting it as the default.
Comment #4
jsbalseraIt seems that there was an error produced by a non strict comparision, so at / page the first language will have a positive match comparing no prefix (NULL) with empties prefixes.
Meanwhile someone could review the patch, I'm writing the required test.
Comment #5
Gábor HojtsyAdd space after //, dot at end of line. Maybe explain a bit more like "we are accessing the root URL" to be easier to read/understand.
Also maybe provide an example. So why is if the prefix is empty then it should not match this?
Comment #6
jsbalseraUhm, I'm a little confused right now. I've installed Drupal 8 importing a Language file (spanish in my case) and then the root page is showed in english but other than that in spanish. If a check Detection and Selections settings the URL detection method is enabled, and the prefixes are "" for english and "es" for spanish. I've change it to make some work but, when I try to change it to the original state I have an error: "The prefix may only be left blank for the default language." So it seems that, perhaps, we must check the language prefixes when we mark a language as default.
Furthermore, when we access to a path, it checked the possible presence of a prefix to set an language. If we access to the path "node/1", for example, the code explodes the path and then compare the first part ("node") with the language prefixes. It won't match so the default language method will set the language displayed. But when we access to the root page ("/") the prefix to be compared will be NULL (the code makes an array_shift to a empty array). If I make the comparison strict it will not match and the default language will be setted.
So I think that the true error is to not check that the default language is the only one that has the empty prefix.
Comment #7
Gábor HojtsyYeah, looks like when we change the default language on the site, we do not update the prefixes. I think we did or we used to have code to change up the prefixes so that if you switch the default language, if a language that is not the default had an empty prefix, it was set to the language code of the language to ensure it still works. Sounds like we should do that (and add tests).
Comment #8
k4v CreditAttribution: k4v commentedFor me it actually works without the patch... I set German as default language and imported .po file. / is shown in German. Will try again with a fresh installation.
Comment #9
k4v CreditAttribution: k4v commentedconfirmed it with a fresh setup, works for me
Comment #10
k4v CreditAttribution: k4v commentedah, i installed with english as default and switched the default after that, will try again.
Comment #11
k4v CreditAttribution: k4v commentednever mind: i can reproduce the bug when adding a .po-file to files/translations and installing with that. All the language prefixes are empty.
Comment #12
k4v CreditAttribution: k4v commentedso here is a first patch. i moved the creation of the prefixes to a seperate function, so they get completely updated from the language_list() function.
this works for me, but maybe has some side effects with other use cases...
Comment #13
Gábor HojtsyComment #14
k4v CreditAttribution: k4v commentedComment #15
Gábor HojtsyHm, we really have this configurable prefix feature, so you can actually configure these things freely. If we drop all your previous configuration and just rebuild it from scratch all the time, that is not really helpful for the existing data. We should only touch things that changed, that was the reason for the existing code. The mistake seems to be the existing empty prefix is not filled in for the previous default language, that is kind of *required* change we need to make on the data, because it is not possible that a language that is not default anymore has an empty prefix.
Comment #16
Gábor HojtsyComment #17
k4v CreditAttribution: k4v commentedright, the problem ist now, when i add a new language, all the user edited prefixes are reset to the defaults. maybe we should restrict it to the installation time...
Comment #18
Gábor HojtsyThe problem also applies to the admin page, no? When you switch defaults, does it fill in the previous language with a prefix at least? It does not seem to be an installer issue only.
Comment #19
jsbalseraI have been checking it and it does fill previous default language, so i think it's only a installer issue. Calling module_invoke_al l('language_update', $language) after we save the new language at the installer wouln't be enought?
Comment #20
jsbalseraThe change that I was talking about. It worked for me.
Comment #21
Gábor HojtsyComment #22
k4v CreditAttribution: k4v commentedi improved my first patch a bit. now i can add a language in the admin and its updated okay. only problem is when changing the default language... :/
Comment #23
k4v CreditAttribution: k4v commentedthis works for me, more edge cases?
Comment #24
k4v CreditAttribution: k4v commentedjsbalsera: I tried the patch at #20, but it doesnt fix anything for me... hmm. / shows up in english and both prefixes are empty.
Comment #25
jsbalserak4v uhm, i will try again tomorrow after work, if this it at same point. Please assign this issue to you :)
Comment #26
k4v CreditAttribution: k4v commentedComment #28
k4v CreditAttribution: k4v commentedi'm not sure why this is failing. i can change the default language in admin and the new default has correclty an empty prefix.
Comment #29
k4v CreditAttribution: k4v commentedThe test asserts that, after setting it the default language, french *has* a prefix.
I think the default language should not have a prefix? So the test is wrong?
Comment #30
LoMo CreditAttribution: LoMo commentedSounds to me like the test makes a faulty assertion, yes. I'll try the patch and manually test things, though (and maybe see if I can work my way through the simpletest logic and fix things so they correctly pass). :-)
Or, looking at the message, I'm thinking that the failure IS that French still had a language prefix when it was the default language. I'll see if I can replicate that.
Comment #31
LoMo CreditAttribution: LoMo commentedI tried using the patch in #23, even though it didn't pass tests. I thought maybe the tests were faulty, or I'd at least check how things work.
I noticed a few things, one which should maybe be put into a separate issue:
To install Drupal in a non-English language, you need to create a new translations directory within sites/default/files. The translation file can not be renamed to, e.g. de.po, but must be kept with its original name, e.g. drupal-7.15.de.po . This is a bit odd. There is also a core/profiles/standard/translations directory, where the readme file indicates *THAT* is where to put translation files. But putting translation files there does not work at all. If you do put a translation file in the apparently-correct location (sites/default/files/translations), the language selector changes, at least in Safari (I'll include this in a separate issue).
[Removed other mention of unrelated issues covered elsewhere now]
Comment #32
penyaskitoYou got more info at #1392208: Impossible to install Drupal in a non-English language when following the provided instructions. The README.txt should be changed, could you add a follow-up?
[Edited, some part of my comment was not useful and could confuse people]
Comment #33
LoMo CreditAttribution: LoMo commentedHi. Yes, will follow-up. Now trying with unpatched core to see if the latter anomaly persists.
Comment #34
k4v CreditAttribution: k4v commentedwhen you do a fresh install with a fresh database and unsing the german .po file in the translations folder, english should automatically have the prefix 'en' after installation. this works for me with the patch.
anyway i can reproduce that http://d8.local/en/node/add shows german texts... hmm. but this seems to be another issue? other admin pages show okay in english. this is strange.
Comment #35
k4v CreditAttribution: k4v commentedah, the german texts are just the names of the content types... Article and Basic Page. The rest is correctly in english. Also the shortcuts are in german, probably because thats bound to the user?
i think that this is a seperate issue.
Comment #36
LoMo CreditAttribution: LoMo commented[ Removed observations which are actually not related to this patch and have been taken care of elsewhere. ]
Comment #37
LoMo CreditAttribution: LoMo commentedI should say, I think your patch works, despite these other issues, but it's hard to say that it's perfect since there seem to be a number of issues that might also interfere with this patch working nicely.
Comment #38
LoMo CreditAttribution: LoMo commentedI think the #22 patch works well. [edited out mention of unrelated issue I thought might be involved -- now reported in separate issue]
Comment #39
k4v CreditAttribution: k4v commentedYou should test #23, there I fixed the issue when you change the default language. I still think the test is wrong.
Comment #40
k4v CreditAttribution: k4v commentedAnother idea: In #23 I fixed the updating of the prefixes. Wouldnt it be better to ignore the saved prefix for the default language?
Probably in http://api.drupal.org/api/drupal/core%21includes%21language.inc/function...
I could change patch #23 then to set the defaultprefix and keep it.
Comment #41
bforchhammer CreditAttribution: bforchhammer commentedSetting the prefix for all languages and ignoring it for the default language sounds like a good approach to me.
I'll have a look at writing a simpletest for the original issue (/ not showing up in install language).
Comment #42
bforchhammer CreditAttribution: bforchhammer commentedI'm not getting anywhere with the test, because simpletest does not seem to go through the normal set of install tasks -- one of which is "language selection". As far as I can see WebTestBase always uses English as the install language... Not sure how to proceed.
Comment #43
bforchhammer CreditAttribution: bforchhammer commentedPatch #23 works, but comments need to be cleaned up (wrap at column 80, dot at end of comment).
Comment #44
bforchhammer CreditAttribution: bforchhammer commentedAlso, with the patch applied, when you switch the default language then the prefix for the default language is deleted... I think it should only update langcodes for non-default-languages if the respective langcode is missing.
Comment #45
k4v CreditAttribution: k4v commentedHeres a new path, i removed the deletion of the prefix for the default language. Seems to work... maybe in the original code it was wrong to delete the prefix anyway.
Comment #46
bforchhammer CreditAttribution: bforchhammer commentedGo, testbot, go.
Comment #47
k4v CreditAttribution: k4v commentedI'm quicker
Comment #48
bforchhammer CreditAttribution: bforchhammer commentedThis should work as well... it leaves the current behavior intact and only sets the language prefix for english during the installer.
Comment #49
k4v CreditAttribution: k4v commentedi think #48 wont work. the original code will clear both default languages.
Comment #50
steinmb CreditAttribution: steinmb commented#47 looks good though let's see what bot says first.
Comment #51
k4v CreditAttribution: k4v commentedThe question is: can we live without empty prefixes? It would be much cleaner in my opinion and it seems to work, if a prefix is not found, drupal falls back to default.
Comment #52
bforchhammer CreditAttribution: bforchhammer commentedNew patch (based on #23) which should hopefully pass the tests....
Comment #53
LoMo CreditAttribution: LoMo commentedManually testing, will follow up. :-)
Comment #54
LoMo CreditAttribution: LoMo commentedPatch in #52 works well for me. :-)
Comment #55
k4v CreditAttribution: k4v commentedyay! this should be it.
Comment #56
das-peter CreditAttribution: das-peter commentedThe approach looks really good to me, I like the fact that there's one location that takes care of this.
However, I'm not sure if
language_save()
is the correct place to calllanguage_negotiation_url_prefixes_update()
.All other negotiation related function calls take place in the related hooks, and I think we should keep them together.
Only concern currently is that my patch now respects
$language->locked
which could affect the behaviour.Comment #58
das-peter CreditAttribution: das-peter commentedLowell and I just figured out the reason why the call to
language_negotiation_url_prefixes_update()
ended up inlanguage_save()
. It relies on the flushed static cachelanguage_list
.A new patch will follow-up.
Further we just discussed with Gàbor about dealing with languages on installation. There the decision was made that if Drupal is installed in a non-english language, English is disabled. The reason for doing so is that we don't want to assume users want a multilingual site.
Obviously no path prefix will be used for the default language selected at installation time.
Comment #59
das-peter CreditAttribution: das-peter commentedHere we go:
Attached patch removes English on installation time if a non-english language is used.
Further it has a small UX improvement in the language path prefix settings form, thanks @Bojhan.
Regarding my concern in #56; I've now simply added a comment which points out why the call of
language_negotiation_url_prefixes_update()
is located inlanguage_save()
.Manual tests were successful, let's hope the test-bot agrees. :)
Comment #60
LoMo CreditAttribution: LoMo commentedAs long as automated tests pass, I think this is good to go. The installation process for non-English works as expected: English is not enabled by default. If German is selected for installation, after installation, German is the ONLY language and has no path prefix. If we then add English and French as enabled languages, then switch the default to French, all languages use a path prefix. If we change the configuration for French to have no path prefix, then going to /, all expected elements on the front page are in French...
I don't see anything wrong with the way things are functioning now in this area. :-)
Comment #61
aspilicious CreditAttribution: aspilicious commentedThis looks great but I don't have the time to read everything.
I don't agree with:
We need some extra tests for this.
Comment #62
bforchhammer CreditAttribution: bforchhammer commentedNot sure how -- simpletest setup process is different to drupal's installation process; normal setup tasks are not worked through and there's no way to modify the "simpletest setup language" as far as I know...
Comment #63
Gábor HojtsyRight, looks like we tested the hell out of this with various people. :) The question is if we have any missing test coverage on the side of changes happening later when the site already runs (and/or if there are any bugs there solved here). We could add more tests for that, if that is applicable. If this is only relevant for the installer, then we cannot really do automated tests for it.
Comment #64
aspilicious CreditAttribution: aspilicious commentedThat sounds testable...
That happens after installation...
Comment #65
Gábor HojtsyIt happens when installing in a foreign language though, which is not possible to make work in simpletest as it is.
Comment #66
Gábor Hojtsy#630446: Allow SimpleTest to test the non-interactive installer
Comment #67
LoMo CreditAttribution: LoMo commented1) Installed in German (German is default without a path prefix)
2) Added English, French, Danish...
3) Made French the site default language, but removed the language prefix for English -- attempted to save
4) Saw expected error message that removing the path prefix from the non-default language is not allowed.
5) Added new hosts/vhosts for .de / .fr /.com versions of my "drupal8" testing site and configured domain-based language negotiation.
6) Site working well with domain-based language negotiation.
I think this can be committed… after we test all of the above *and* test adding a '/' in the path prefix (the bug Schnitzel found), once the slash is disallowed as character in the prefix… i.e., hopefully after the next patch.
Comment #68
Gábor HojtsyLooks like a bug indeed. You should only be able to remove the prefix for the default language.
Comment #69
das-peter CreditAttribution: das-peter commentedCan't believe that happens :-O Bojhan and I just added the default language indicator to that form because the validator hit me while testing with something like "Only the default language can have an empty path prefix" ;)
Comment #70
LoMo CreditAttribution: LoMo commentedRe 68/69: I actually changed my description since I think I must have been mistaken. At least I was not able to replicate that.
Schnitzel did find issues with having a '/' in the path prefix field (e.g. '12/55'). So he's adding a form validation rule to disallow a slash in any path prefix. After that has been added and I've retested, I really think we could consider this RTBC since there is no way to automatically test the scenario of installing in a non-English language.
Comment #71
Schnitzel CreditAttribution: Schnitzel commentedmade a review and looked good!
but found a bug where the language prefix does not work if it contains a slash, so added a new validator.
also wrote tests for:
- changing prefix
- changing non default language to empty string
- string containing slash
also removing t() in test messages
Comment #73
Schnitzel CreditAttribution: Schnitzel commented#71: url_prefixes_recreate-1738330-68.patch queued for re-testing.
Comment #74
LoMo CreditAttribution: LoMo commentedGreen! And I think this patch works great, too. Personally, I'd say this should be RTBC.
Comment #75
das-peter CreditAttribution: das-peter commentedAwesome, thanks for the additions!
Regarding to the mentioned possible test in #64 I'd say we should wait on a testable installation as the steps to reproduce the error (see the original post in the summary) rely on the installation.
Thus I'd vote for RTBC, too.
Comment #76
catchMissing a space between prefix%default_indicator?
Should be 'string contains', not 'strings contains'.
language_list() should have the parenthesis so it links to the function.
Just empty() covers the !isset case as well.
"the prefix a user has configured' looks abit awkwared. 'Otherwise we keep the configured prefix'?
Comment #77
Gábor HojtsyNo, if it is not the default, there is currently no whitespace at the end. If there would be whitespace before %, there would be whitespace at the end of the string for non-defaults. Maybe it would be cleaner if we'd have logic outside t() that would pick between two strings, like so:
Comment #78
das-peter CreditAttribution: das-peter commentedRe-rolled patch. I think all the points catch mentioned in #76 should be covered, only exception is the
prefix%default_indicator
this is correct as it is.Comment #79
das-peter CreditAttribution: das-peter commentedIRC:
Okay, here we go :)
Comment #81
das-peter CreditAttribution: das-peter commentedThe failed test is
Drupal\system\Tests\Mail\MailTest
, since this is fairly unrelated I give it another shot.Comment #82
das-peter CreditAttribution: das-peter commented#79: url_prefixes_recreate-1738330-79.patch queued for re-testing.
Comment #84
penyaskito#79: url_prefixes_recreate-1738330-79.patch queued for re-testing.
Comment #85
Gábor Hojtsy@das-peter: thanks for the updates! Should be back to RTBC.
Comment #86
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #87
Gábor HojtsyComment #88
Gábor HojtsyApart of the bugs fixed and the minor UX fix to designate the default language in the prefix/domain setup, the only change here is that English is now not setup if you select a foreign language on install. I've added a change notice for that at http://drupal.org/node/1776734, but I don't think that merits a changelog.txt entry. So we should be done here. Thanks all!
Comment #89.0
(not verified) CreditAttribution: commentedUpdated issue summary.