Domain language negotiation broken when a base url is configured

The critical priority is only for issues breaking a whole site in very common configurations, this is admittedly an edge case. From a cursory look the proposed solution looks a bit weird to me: am I missing something or does it assume that language domains are configured as full URLs with a non empty base path? If this is the case the configuration is not correct, as the language domain is assumed to hold only the domain URL part.

That said, I've verified that the bug exists and is easily reproducible by just configuring a base_url and setting domain language negotiation up. I think something like the following would be a more correct solution:

<?php
$options['base_url'] = $url_scheme . $host . rtrim('/', base_path());
?>

Anyway, this will need to be fixed in D8 and then backported.

Also, this will need test coverage.

Clarifying the issue title.

This also breaks our customer's website.

I would opt to rewrite the Issue title again. Because the problem can be described as follows:

Domain language negotiation broken with Drupal installed in a subfolder.

It breaks even without explicitly setting a $base_url in settings.php, the check in the patch to see if (isset($base_url)) is unnecessary because there's always a base_url, whether you set it in settings.php or not.

So, indeed, currently, when you have:

http://dutch-example.com/subfolder
http://french-example.com/subfolder

with language domains configured as french-example.com and dutch-example.com it outputs URLs as http://dutch-example.com/ and forgets about "subfolder"

plach's fix is minimal and almost works, it's just that the arguments have to be like this: rtrim(base_path(), '/');

$options['base_url'] = $url_scheme . $host . rtrim(base_path(), '/');

I'll add patches for D8 and D7 tonight.

Here's the patch for Drupal 7.

Here's the patch for Drupal 8.

Thanks, we need some test coverage before committing this.

Here it is again for Drupal 8 with the fix...

I had introduced a bug that adds the subfolder between the domain and the port... http://my.example.com/subfolder:88 doesn't work.

Also adjusted tests so they don't fail when run from a Drupal in a subfolder.

Hmm, maybe the build-system didn't pick up my 2nd patch because it has the same name as the first failing one...

Just a rename then...

Set the status to needs review if you want the bot to pick the patch up.

Ok, bot green. Can you provide a test-only patch (as in #13 but without the fix) to show that the bug is actual test-covered?

Well,

I don't quite know how to do that... My understanding is that when you run the Drupal tests, Drupal creates a new database to run the tests against, right?

So it basically runs its tests from the location where it's installed, just on a fresh database. My test-case should set up Drupal in a subfolder of Apache's DocumentRoot and as far as I know I don't think it'll be copying itself into a new directory.

The only way I found I could verify whether my patch works is by actually installing Drupal in a subfolder and running the tests. If you know how to create a test-setup so that Drupal acts as if it's installed in a subfolder, please enlighten me... I don't know.

Well, testbots work in a slightly different way: they perform a checkout in sites/default/files/checkout but I guess there is no way to configure a base path. You could try and see if there is test coverage for the plain base path functionality: if there is take inspiration from it, otherwise I think it's ok to go on with the current patch.

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

tagging

Manually testing...

@vuzzbox, what's your conclusion?

Conclusion: Applying the D8 patch resolves the problem. D7 not tested.

Steps I took to reproduce the error:

• Clean, functional install of D8 in subfolder http://drupal8/drupal
• Configured two new vhost aliases: english.drupal8 and francais.drupal8 pointing to same install
• Enabled Language, Content Translation and Interface Translation modules
• Configured domains in "Language detection and selection" settings:
  • English: english.drupal8
  • French: francais.drupal8
• Navigation fails from that point forward, per original post. Fails to append the subfolder name

Applied this patch: domain-language-negotiation-fix-1826252-8-1.patch

URLs now properly formatted with sub-folder paths and I can navigate successfully across both english.drupal8 and francais.drupal8 sites.

The patch looks good on a code review too. I don't believe this would be a major issue though, its kind of a twisted configuration that you'd use subfolders for sites but then still have control over domains to switch langage. You could just as well configure your site more cleanly to use domains in the first place instead of folders. The idea is that you'd likely use a subfolder if your host does not let you fiddle with domains. Anyway, the patch looks good, and should get in to cover this edge case.

Needs a reroll

curl https://drupal.org/files/domain-language-negotiation-fix-1826252-8-1.patch  | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2641  100  2641    0     0   2757      0 --:--:-- --:--:-- --:--:--  3368
error: patch failed: core/modules/language/language.negotiation.inc:486
error: core/modules/language/language.negotiation.inc: patch does not apply
error: patch failed: core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php:454
error: core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php: patch does not apply
error: patch failed: core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php:136
error: core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php: patch does not apply

re-rolled, with minor conflicts resolved

another attempt - looks like I tried to re-introduce an entire function, oops!

Only test changes are included in this patch, not the actual fix from core/modules/language/language.negotiation.inc?!

I'm unsure how to proceed, any advice would be great! It looks like the 'language_url_rewrite_url' function is no longer in 8.x HEAD, which is where this fix occurs (that entire function is in my patch on #25). Does this mean it's no longer relevant at all? Or maybe this functionality has been moved?

thanks!

It moved in all probability :) You can grep Drupal for a distinctive snippet of that function to see where it is now.

gabor$ grep -r "The colon in the URL scheme messes up" *
core/modules/language/lib/Drupal/language/HttpKernel/PathProcessorLanguage.php:          // The colon in the URL scheme messes up the port checking below.

There is your answer :)

thanks for the advice - hopefully this one works.

It looks to be the place where it went, indeed!

Greato!

Committed 0363a0d and pushed to 8.x. Thanks!

Has needs backport to D7 tag :)

Sorry, I've been away on holiday.
Glad to see the patch made it in! It might be a corner-case but it's a bit short-sighted to dictate how customers should set up their environments.
In this case the customer (a big one) has a website and a subsection of the website (a Drupal installation) lives in a subfolder.

Anyway, shall I do the same patch for Drupal 7 then? Or is someone already on it?

I guess I'll have to create a new issue for it on the D7 issue branch?

Our current backport process is we use the same issue for the backport. So as soon as you have a patch, feel free to change status to needs review :)

typos and edits.

I have backported the patch for d7 and will be working on this issue.
I will be shortly submitting the updated patch with test cases.

I have updated the patch as the previous patch contained white spaces and the debug code.
Explanation:
Code for locale_language_url_rewrite_url function is present in the file includes/locale.inc for d7.

I have changed the #8 a bit because d8 patch which is commited seemed to be a slight different. So as to maintain the consistency I added a new line appending the base url.

Thanks

Updated the code with the test cases.

Updated test case.

+++ b/modules/locale/locale.test
@@ -2514,7 +2514,7 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+      $link = (!empty($GLOBALS['conf']['clean_url'])) ? $langcode . '.example.com' . rtrim(base_path(), '/') . '/admin' : $langcode . '.example.com/?q=admin';

this might need a slash between .com and rtrim ?

oh and these ternary operators hurt my brain.
[edit: added bits from irc conversation]
Looks like rtrim would be needed in the second part wher ethere is no clean urls.

maybe making a $base_url var would clean that up? like...
$base_url = $langcode . '.example.com/' . rtrim(base_path(), '/');
$link = (!empty($GLOBALS['conf']['clean_url'])) ? $base_url . '/admin' : $base_url . '/?q=admin';

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php
@@ -144,7 +145,8 @@ function testDomainNameNegotiationPort() {
-    $expected = $index_php ? 'http://example.fr:90/index.php/' : 'http://example.fr:90/';
+    $expected = $index_php ? 'http://example.fr:90/index.php' : 'http://example.fr:90' . rtrim(base_path(), '/') . '/';

although, looking back at the d8 patch, there is not a / before the rtrim.

I'm confused.

[edit: more thoughts from irc]
maybe base_path always starts with a / (so we dont get something like http://example.fr:90whatever/

also, that "fix" in the test in the d8 patch, ... seems like it needs the rtrim(base_path()) in the first half of that if also.
should it have been:

+    $expected = $index_php ? 'http://example.fr:90' . rtrim(base_path(), '/') . /index.php' : 'http://example.fr:90' . rtrim(base_path(), '/') . '/';

?

At the moment tests pass.
Also manual testing is ok.
Can we commit it to 7.x ?

Ok, lets fix tests

Locally all test is ok for me. Lets check it one more time.

Re-roll.

Ok, last path pass tests.

Also I confirm that patch works well in my environment and my multilingual setup.

I have domain language detection and drupal placed in subdir.

Any reason why the patch #44 / #56 is missing two changes in tests from the D8 patch? Only one change in tests is included in the D7 patch. If there is not reason for this, these should be added as well (the code from D8 patch is still used in D9).

Moving this back to Needs review because of #59. Thanks!