It would appear there is a use-case where the function locale_language_url_rewrite_url() produces an improper URL.

When working with domain specific (LOCALE_LANGUAGE_NEGOTIATION_URL_DOMAIN) URLs for language negotiation which are nested in subdirectories, so for instance, if we have drupal rendering behind something like this:

we run into a problem if the locale modules setting for USER INTERFACE TEXT LANGUAGE DETECTION method is enabled for URL.

Due to a hardcoding of the base url within the function locale_language_url_rewrite_url(), these lines...

$host = 'http://' . str_replace(array('http://', 'https://'), '', $options['language']->domain);
$host = parse_url($host, PHP_URL_HOST);

// Apply the appropriate protocol to the URL.
$options['base_url'] = $url_scheme . $host;

...which dictate what the new base_url shall be, are overwriting the URL path. Should the site be using the global setting $base_url, the value is ignored, the paths are forced to render incorrectly and the rest of the site will generate HREF from the base HOST url and send the user in the wrong direction when navigating links.

The following edit to locale.inc corrects this and ensures the global value is used:

// Let's check the $base_url and add it to the path if it exists.
global $base_url;
if (isset($base_url)) {
  $base_path = parse_url($base_url, PHP_URL_PATH);
  if ($base_path) {
    $options['base_url'] .= $base_path;
  }
}

I've included a patch file in case this can make it upstream.

Hopefully you find this useful and compelling to help solve a fringe use-case.

Files: 
CommentFileSizeAuthor
#44 domain-language-negotiation-fix-1826252-7-3.patch1.27 KBprashantgoel
FAILED: [[SimpleTest]]: [MySQL] 40,417 pass(es), 13 fail(s), and 0 exception(s).
[ View ]
#42 domain-language-negotiation-fix-1826252-7-2.patch2.16 KBprashantgoel
FAILED: [[SimpleTest]]: [MySQL] 40,380 pass(es), 13 fail(s), and 0 exception(s).
[ View ]
#40 domain-language-negotiation-fix-1826252-7-1.patch510 bytesprashantgoel
FAILED: [[SimpleTest]]: [MySQL] 40,272 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#39 domain-language-negotiation-fix-1826252-7-1.patch899 bytesprashantgoel
#31 domain-language-negotiation-fix-1826252-31.patch2.74 KBbenjf
PASSED: [[SimpleTest]]: [MySQL] 57,442 pass(es).
[ View ]
#27 domain-language-negotiation-fix-1826252-27.patch2 KBbenjf
FAILED: [[SimpleTest]]: [MySQL] 57,222 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#25 domain-language-negotiation-fix-1826252-25.patch5.64 KBbenjf
FAILED: [[SimpleTest]]: [MySQL] 57,994 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#13 domain-language-negotiation-fix-1826252-8-1.patch2.58 KBJeroen
PASSED: [[SimpleTest]]: [MySQL] 56,025 pass(es).
[ View ]
#12 domain-language-negotiation-fix-1826252-8.patch2.58 KBJeroen
PASSED: [[SimpleTest]]: [MySQL] 55,933 pass(es).
[ View ]
#9 domain-language-negotiation-fix-1826252-8.patch922 bytesJeroen
FAILED: [[SimpleTest]]: [MySQL] 56,995 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#8 domain-language-negotiation-fix-1826252-7.patch871 bytesJeroen
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch domain-language-negotiation-fix-1826252-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 locale.inc_.patch727 byteswilco
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale.inc__0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 locale_url_detection.patch748 byteswilco
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale_url_detection_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
locale_url_detection.patch879 byteswilco
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale_url_detection.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

wilco’s picture

StatusFileSize
new748 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale_url_detection_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Upon some reflection, I think I found an even better solution to the patch I wrote above.

Instead of going to the $base_url value, which may not be set, rather, check the $options['language']->domain value and see if the path is set there.

$language_path = parse_url($options['language']->domain, PHP_URL_PATH);
if ($language_path) {
  $options['base_url'] .= $language_path;
}

This is a tighter implementation and stays within the definition of the language configuration.

wilco’s picture

Version:7.14» 7.21
StatusFileSize
new727 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale.inc__0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This is an update of this patch for version 7.21 of Drupal Core

wilco’s picture

Priority:Normal» Critical

I'm setting this critical as it kills our government website with every update of core. Please, can I get someone to review or talk to me so I can explain the use case?

plach’s picture

Version:7.21» 8.x-dev
Priority:Critical» Major
Status:Active» Needs work
Issue tags:+needs backport to D7

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.

plach’s picture

Issue tags:+Needs tests

Also, this will need test coverage.

plach’s picture

Title:Discovered issue with locale_language_url_rewrite_url() and "Determine the language from the URL (Path prefix or domain)" option» Domain language negotiation broken when a base url is configured

Clarifying the issue title.

Jeroen’s picture

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(), '/');

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

I'll add patches for D8 and D7 tonight.

Jeroen’s picture

StatusFileSize
new871 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch domain-language-negotiation-fix-1826252-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's the patch for Drupal 7.

Jeroen’s picture

StatusFileSize
new922 bytes
FAILED: [[SimpleTest]]: [MySQL] 56,995 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Here's the patch for Drupal 8.

plach’s picture

Status:Needs work» Needs review

Thanks, we need some test coverage before committing this.

Status:Needs review» Needs work

The last submitted patch, domain-language-negotiation-fix-1826252-8.patch, failed testing.

Jeroen’s picture

StatusFileSize
new2.58 KB
PASSED: [[SimpleTest]]: [MySQL] 55,933 pass(es).
[ View ]

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.

Jeroen’s picture

Issue tags:-Needs tests
StatusFileSize
new2.58 KB
PASSED: [[SimpleTest]]: [MySQL] 56,025 pass(es).
[ View ]

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

plach’s picture

Status:Needs work» Needs review

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

plach’s picture

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?

Jeroen’s picture

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.

plach’s picture

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.

dcam’s picture

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

YesCT’s picture

Issue tags:+Needs manual testing

tagging

vuzzbox’s picture

Assigned:Unassigned» vuzzbox

Manually testing...

Jeroen’s picture

@vuzzbox, what's your conclusion?

vuzzbox’s picture

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.

Gábor Hojtsy’s picture

Priority:Major» Normal
Status:Needs review» Reviewed & tested by the community
Issue tags:+D8MI, +language-base

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.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

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
benjf’s picture

Status:Needs work» Needs review
StatusFileSize
new5.64 KB
FAILED: [[SimpleTest]]: [MySQL] 57,994 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

re-rolled, with minor conflicts resolved

Status:Needs review» Needs work

The last submitted patch, domain-language-negotiation-fix-1826252-25.patch, failed testing.

benjf’s picture

Status:Needs work» Needs review
StatusFileSize
new2 KB
FAILED: [[SimpleTest]]: [MySQL] 57,222 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

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

Gábor Hojtsy’s picture

Status:Needs review» Needs work

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

benjf’s picture

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!

Gábor Hojtsy’s picture

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

benjf’s picture

StatusFileSize
new2.74 KB
PASSED: [[SimpleTest]]: [MySQL] 57,442 pass(es).
[ View ]

thanks for the advice - hopefully this one works.

alexpott’s picture

Status:Needs work» Needs review
Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

Greato!

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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

Gábor Hojtsy’s picture

Version:8.x-dev» 7.x-dev
Status:Fixed» Patch (to be ported)

Has needs backport to D7 tag :)

Jeroen’s picture

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?

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Issue summary:View changes

typos and edits.

prashantgoel’s picture

Issue summary:View changes
StatusFileSize
new899 bytes

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.

prashantgoel’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new510 bytes
FAILED: [[SimpleTest]]: [MySQL] 40,272 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 40: domain-language-negotiation-fix-1826252-7-1.patch, failed testing.

prashantgoel’s picture

Status:Needs work» Needs review
StatusFileSize
new2.16 KB
FAILED: [[SimpleTest]]: [MySQL] 40,380 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Updated the code with the test cases.

Status:Needs review» Needs work

The last submitted patch, 42: domain-language-negotiation-fix-1826252-7-2.patch, failed testing.

prashantgoel’s picture

Status:Needs work» Needs review
StatusFileSize
new1.27 KB
FAILED: [[SimpleTest]]: [MySQL] 40,417 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Updated test case.

Status:Needs review» Needs work

The last submitted patch, 44: domain-language-negotiation-fix-1826252-7-3.patch, failed testing.

YesCT’s picture

+++ 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';

YesCT’s picture

+++ 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(), '/') . '/';

?