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:

  1. 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
  2. Only the default language can have an empty path prefix.
  3. 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:

  1. Install a D8 site in a language other than English, with a valid translation in the translations folder which could be imported.
  2. When going to /, interface is on English. When going to /node, interface is on the installation language.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Hm, 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.

jsbalsera’s picture

Assigned: Unassigned » jsbalsera
penyaskito’s picture

Issue tags: +Needs tests

It happens in both cases: when installing in other language, or when adding a language after installation and setting it as the default.

jsbalsera’s picture

It 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.

Gábor Hojtsy’s picture

Status: Active » Needs work
+++ b/core/includes/language.incundefined
@@ -502,7 +502,8 @@ function language_url_split_prefix($path, $languages) {
+    //Added strict comparision to avoid false match when we are accessing /
+    if (isset($prefixes[$language->langcode]) && $prefixes[$language->langcode] === $prefix) {

Add 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?

jsbalsera’s picture

Uhm, 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.

Gábor Hojtsy’s picture

Yeah, 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).

k4v’s picture

For 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.

k4v’s picture

confirmed it with a fresh setup, works for me

k4v’s picture

ah, i installed with english as default and switched the default after that, will try again.

k4v’s picture

never mind: i can reproduce the bug when adding a .po-file to files/translations and installing with that. All the language prefixes are empty.

k4v’s picture

so 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...

Gábor Hojtsy’s picture

Status: Needs work » Needs review
k4v’s picture

Gábor Hojtsy’s picture

Hm, 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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
k4v’s picture

right, 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...

Gábor Hojtsy’s picture

The 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.

jsbalsera’s picture

I 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?

jsbalsera’s picture

The change that I was talking about. It worked for me.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
k4v’s picture

i 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... :/

k4v’s picture

this works for me, more edge cases?

k4v’s picture

jsbalsera: I tried the patch at #20, but it doesnt fix anything for me... hmm. / shows up in english and both prefixes are empty.

jsbalsera’s picture

k4v uhm, i will try again tomorrow after work, if this it at same point. Please assign this issue to you :)

k4v’s picture

Assigned: jsbalsera » k4v

Status: Needs review » Needs work

The last submitted patch, url_prefixes_recreate-1738330-23.patch, failed testing.

k4v’s picture

Status: Needs work » Needs review

i'm not sure why this is failing. i can change the default language in admin and the new default has correclty an empty prefix.

k4v’s picture

The 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?

LoMo’s picture

Sounds 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.

LoMo’s picture

I 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]

penyaskito’s picture

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).

You 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]

LoMo’s picture

Hi. Yes, will follow-up. Now trying with unpatched core to see if the latter anomaly persists.

k4v’s picture

when 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.

k4v’s picture

ah, 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.

LoMo’s picture

[ Removed observations which are actually not related to this patch and have been taken care of elsewhere. ]

LoMo’s picture

I 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.

LoMo’s picture

I think the #22 patch works well. [edited out mention of unrelated issue I thought might be involved -- now reported in separate issue]

k4v’s picture

You should test #23, there I fixed the issue when you change the default language. I still think the test is wrong.

k4v’s picture

Another 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.

bforchhammer’s picture

Setting 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).

bforchhammer’s picture

I'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.

bforchhammer’s picture

Status: Needs review » Needs work

Patch #23 works, but comments need to be cleaned up (wrap at column 80, dot at end of comment).

bforchhammer’s picture

Also, 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.

k4v’s picture

Heres 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.

bforchhammer’s picture

Status: Needs work » Needs review

Go, testbot, go.

k4v’s picture

I'm quicker

bforchhammer’s picture

This should work as well... it leaves the current behavior intact and only sets the language prefix for english during the installer.

k4v’s picture

i think #48 wont work. the original code will clear both default languages.

steinmb’s picture

#47 looks good though let's see what bot says first.

k4v’s picture

The 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.

bforchhammer’s picture

New patch (based on #23) which should hopefully pass the tests....

LoMo’s picture

Manually testing, will follow up. :-)

LoMo’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #52 works well for me. :-)

k4v’s picture

yay! this should be it.

das-peter’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.05 KB
2.66 KB

The 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 call language_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.

Status: Needs review » Needs work

The last submitted patch, url_prefixes_recreate-1738330-56.patch, failed testing.

das-peter’s picture

Lowell and I just figured out the reason why the call to language_negotiation_url_prefixes_update() ended up in language_save(). It relies on the flushed static cache language_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.

das-peter’s picture

Status: Needs work » Needs review
FileSize
2.14 KB
4.56 KB

Here 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 in language_save().
Manual tests were successful, let's hope the test-bot agrees. :)

LoMo’s picture

As 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. :-)

aspilicious’s picture

This looks great but I don't have the time to read everything.
I don't agree with:

As long as automated tests pass, I think this is good to go.

We need some extra tests for this.

bforchhammer’s picture

We need some extra tests for this.

Not 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...

Gábor Hojtsy’s picture

Right, 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.

aspilicious’s picture

E.g. in some situations when accessing /, UI is being shown on English instead of the default language.

That sounds testable...
That happens after installation...

Gábor Hojtsy’s picture

It happens when installing in a foreign language though, which is not possible to make work in simpletest as it is.

Gábor Hojtsy’s picture

LoMo’s picture

1) 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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looks like a bug indeed. You should only be able to remove the prefix for the default language.

das-peter’s picture

Can'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" ;)

LoMo’s picture

Re 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.

Schnitzel’s picture

Status: Needs work » Needs review
FileSize
9.75 KB
5.41 KB

made 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

Status: Needs review » Needs work
Issue tags: -Needs tests, -D8MI, -sprint

The last submitted patch, url_prefixes_recreate-1738330-68.patch, failed testing.

Schnitzel’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +D8MI, +sprint
LoMo’s picture

Status: Needs review » Reviewed & tested by the community

Green! And I think this patch works great, too. Personally, I'd say this should be RTBC.

das-peter’s picture

Awesome, 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.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/language/language.admin.incundefined
@@ -698,7 +698,11 @@ function language_negotiation_configure_url_form($form, &$form_state) {
+      '#title' => t('%language (%langcode) path prefix%default_indicator', array(

Missing a space between prefix%default_indicator?

+++ b/core/modules/language/language.admin.incundefined
@@ -742,6 +746,11 @@ function language_negotiation_configure_url_form_validate($form, &$form_state) {
+    elseif (strpos($value, '/') !== FALSE) {
+      // Throw a form error if the strings contains a slash,

Should be 'string contains', not 'strings contains'.

+++ b/core/modules/language/language.moduleundefined
@@ -244,6 +244,10 @@ function language_save($language) {
+  // Update URL Prefixes for all languages after the new default language is

language_list() should have the parenthesis so it links to the function.

+++ b/core/modules/language/language.negotiation.incundefined
@@ -435,6 +435,24 @@ function language_negotiation_url_prefixes() {
+    // or the prefix is set to the empty string.
+    if (!isset($prefixes[$language->langcode]) || empty($prefixes[$language->langcode])) {

Just empty() covers the !isset case as well.

+++ b/core/modules/language/language.negotiation.incundefined
@@ -435,6 +435,24 @@ function language_negotiation_url_prefixes() {
+    }
+    // Otherwise we keep the prefix a user has configured.
+  }
+  language_negotiation_url_prefixes_save($prefixes);
+}

"the prefix a user has configured' looks abit awkwared. 'Otherwise we keep the configured prefix'?

Gábor Hojtsy’s picture

+++ b/core/modules/language/language.admin.incundefined
@@ -698,7 +698,11 @@ function language_negotiation_configure_url_form($form, &$form_state) {
+      '#title' => t('%language (%langcode) path prefix%default_indicator', array(

Missing a space between prefix%default_indicator?

No, 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:

      $t_args = array('%language' => $language->name, '%langcode' => $language->langcode); 
      // ...
      '#title' => $language->default ? t('%language (%langcode) path prefix (Default language)', $t_args) : t('%language (%langcode) path prefix', $t_args),
das-peter’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
9.7 KB

Re-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.

das-peter’s picture

IRC:

[18:08] GaborHojtsy das-peter: catch indicated he would be happier with my proposed default ? t() : t() setup in #1738330: Confusing Language negotiation when accessing /

Okay, here we go :)

Status: Needs review » Needs work

The last submitted patch, url_prefixes_recreate-1738330-79.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review

The failed test is Drupal\system\Tests\Mail\MailTest, since this is fairly unrelated I give it another shot.

das-peter’s picture

Issue tags: -Needs tests, -D8MI, -sprint

Status: Needs review » Needs work

The last submitted patch, url_prefixes_recreate-1738330-79.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +D8MI, +sprint
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

@das-peter: thanks for the updates! Should be back to RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Gábor Hojtsy’s picture

Issue tags: +negotiation
Gábor Hojtsy’s picture

Issue tags: -sprint

Apart 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!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.