URL generation only works on port 80 when using domain based language negotation

Does this also apply to Drupal 8?

Will also definitely need tests.

Tagging as release blocker so this gets fixed first

Problem confirmed on Drupal 8

Setup: Server listening on port 80 and 81
All URL's are linking to port 80

Let's try this

fixed

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.phpundefined
@@ -79,4 +79,34 @@ class LanguageUrlRewritingTest extends WebTestBase {
+    // Enable domain configuration
...
+    // Fake a different port
...
+    // Create an absolute French link

dots

Looks good, except for these code-style issues.

+++ b/core/modules/language/language.negotiation.inc
@@ -383,6 +383,14 @@ function language_url_rewrite_url(&$path, &$options) {
+          $http_host= $_SERVER['HTTP_HOST'];

This is missing a space before the =.

+++ b/core/modules/language/language.negotiation.inc
@@ -383,6 +383,14 @@ function language_url_rewrite_url(&$path, &$options) {
+            $http_host_tmp = explode(':', $http_host);
+            $options['base_url'] .= ':' . $http_host_tmp[1];

This is a matter of taste, but the first line could be written as

list($host, $port) = explode(':', $http_host);

That would allow using $port directly and would avoid the arbitrary 1 index.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php
@@ -79,4 +79,34 @@ class LanguageUrlRewritingTest extends WebTestBase {
+      "language_negotiation_url_part" => 1,
+      "domain[fr]" => $language_domain

We usually prefer single-quotes, I think.

space and quotes fixed
$http_host_tmp[1] rewritten to end($http_host_tmp), a similar construct is used in the other issue.

@attiks: can you summarize in a couple sentences the reasons this did not work before?

Code-style-wise looks good. :)

Is this tagged " 7.15 release blocker" because it's a regression from behavior that worked in previous versions of Drupal? Do we know when the last time was (if ever) that this behavior did work?

Just trying to clarify why that tag is applied to this issue... thanks.

This was first committed to D7 on 27/3/2012 #1250800-130: Language domain should work regardless of ports or protocols, and release with 7.12
Followed by #1572394: Language detection by domain only works on port 80 on 15/5/2012 with a first problem, fixed and committed #1572394-43: Language detection by domain only works on port 80
And finally (i hope) this issue

It's not really a regression but in the past you couldn't use domain names if you site needed http/https, we tried fixing it in the first issue, the other 2 are 'new' problems. So it would be nice if we can fix this before releasing a new D7.

Well, it is both a regression, and not. So earlier before #1250800: Language domain should work regardless of ports or protocols, your help text said you should enter a full URL (with protocol, port, etc) which was prepended to your paths for specific languages. That functionality still works in Drupal 7, since we wanted to keep it backwards compatible, but the current help text suggests you enter a domain name only, and Drupal will put on the port and protocol itself, so you can have http and https pages for example and still use domains for languages. If you do follow the new instructions, custom ports will not work. If you follow the original (old) instructions, custom ports will work. So its a regression in that our current instructions will not make your setting work if you are not on port 80, looks like.

OK, thanks for the further information.

I think since this has been around for a little while and is only a "pseudo-regression", we probably wouldn't hold up a Drupal 7 release just for this, so I'm removing the tag. But I'd definitely commit this to D7 quickly, once it's ready!

#12: i1645156-12.patch queued for re-testing.

Generally looks good, great that it has extra test coverage. Some notes:

+++ b/core/modules/language/language.negotiation.incundefined
@@ -383,6 +383,14 @@ function language_url_rewrite_url(&$path, &$options) {
+          // Add the port if we're not running on port 80.
+          $http_host = $_SERVER['HTTP_HOST'];
+          if (strpos($http_host, ':') !== FALSE) {
+            $http_host_tmp = explode(':', $http_host);
+            $options['base_url'] .= ':' . end($http_host_tmp);
+          }

I have not found docs that would indicate this would only ever contain the number if its different from 80 (or that it would always contain the number in that case). Looked on php.net. Do we have some docs to refer to here?

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.phpundefined
@@ -79,4 +79,34 @@ class LanguageUrlRewritingTest extends WebTestBase {
+  /**
+   * Check URL rewriting for the given language when using a domain name.
+   */

"when using a domain name with a non-standard port" right? That is the goal of the patch. (Note the comment line will pass 80 chars then, needs some word-smithing to stay under that limit).

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.phpundefined
@@ -79,4 +79,34 @@ class LanguageUrlRewritingTest extends WebTestBase {
+  function testDomainNameNegotiationPort() {
+    $language_domain = 'example.cn';
...
+    $this->assertTrue(strcmp($url, 'http://example.cn:88/') == 0, 'The right port is used');
+  }

This is more minor, but it would be good to use a domain that is relevant for the language tested :) You test French with example.cn, interesting combination :)

$_SERVER['HTTP_HOST'] is the Host taken from the HTTP request header, it can even contain :80 if the request was made like that, see a similar discussion in secure pages: #334419: $_SERVER['HTTP_HOST'] has port number and regex doesn't strip it.. php.net says "Contents of the Host: header from the current request, if there is one."

I used example.cn because the tests are using this already for a different test, if you want I'll change it.

I will fix the comment later today.

Ok, http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.23 documents the port question pretty well, right. Would be good to summarize it in a sentence there. "HTTP_HOST will optionally contain the port. Preserve that for the domain of the target language as well." Or something like that. It would be good to fix the example.cn test and the comments too, thanks! I think this looks to be close.

should be ok

Looks great now. Fixes the bug, expands tests proper :) Thanks for sticking to it.

Add on sprint for tracking.

This seems like a pretty straight-forward bug fix. Nice test!

Committed and pushed to 8.x. Doesn't seem to apply cleanly to D7.

Reworked test and test works properly locally. I'm somewhat of a noob when it comes to testing, so please let me know if I did anything incorrectly.

index 246edc2..e9626d5 100644
--- a/.htaccess

--- a/.htaccess
+++ b/.htaccessundefined

+++ b/.htaccessundefined
@@ -102,7 +102,7 @@ DirectoryIndex index.php index.html index.htm

@@ -102,7 +102,7 @@ DirectoryIndex index.php index.html index.htm
   RewriteCond %{REQUEST_FILENAME} !-f
   RewriteCond %{REQUEST_FILENAME} !-d
   RewriteCond %{REQUEST_URI} !=/favicon.ico
-  RewriteRule ^ index.php [L]
+  RewriteRule ^ /index.php [L]

remove this please

Ah, that's what I get for trying to code in a car while low on sleep, lol. Thanks @attiks!

+    // Fake a different port.
+    $_SERVER['HTTP_HOST'] .= ':88';

Shouldn't we be undoing this at the end of the test? Otherwise, that value will persist for all other tests that run afterwards in the same page request, which could potentially affect some of them.

I can't think of a specific example right now, but I know that elsewhere in the codebase where we have a test modify $_SERVER, we try to make sure the modifications are undone at the end.

I had a quick look:

• modules/simpletest/tests/common.test is setting the value back
• modules/simpletest/tests/bootstrap.test is setting the value back
• modules/locale/locale.test isn't setting the value back

I don't think it's needed, but it wouldn't hurt to reset the value,

$_SERVER['HTTP_HOST'] = str_replace(':88', '', $_SERVER['HTTP_HOST']);

should do it

But changing this will mean fixing Drupal 8 first

Agreed, let's fix it for Drupal 8 first. I think we can just remember the value at the start and set it back at the end? Instead of the dynamic manipulation suggested.

Trivial.

Yup.

This is one of the quickest RTBCs ever, I think :) (Assuming the tests pass.)

Oh, ha, mine wasn't the quickest RTBC ever because Gábor cross-posted and changed the issue status one minute before me :)

*whoosh*

Committed and pushed to 8.x. Thanks! :)

Let's continue with D7 backport then.

as requested

+++ b/includes/locale.incundefined
@@ -457,6 +457,14 @@ function locale_language_url_rewrite_url(&$path, &$options) {
+
+          // HTTP_HOST will optionally contain the port. Preserve that for
+          // the domain of the target language as well.
+          $http_host = $_SERVER['HTTP_HOST'];
+          if (strpos($http_host, ':') !== FALSE) {
+            $http_host_tmp = explode(':', $http_host);
+            $options['base_url'] .= ':' . end($http_host_tmp);
+          }

Just looking at this one, is it not possible at all for $options['base_url'] to already contain a port? (In which case after this patch it will be two ports?).

According to default.settings.php it can be

* Examples:
* $base_url = 'http://www.example.com';
* $base_url = 'http://www.example.com:8888';
* $base_url = 'http://www.example.com/drupal';
* $base_url = 'https://www.example.com:8888/drupal';

So better change code to something like (untested):

if (strpos($http_host, ':') !== FALSE && strpos($options['base_url'], ':') === FALSE) {
  $http_host_tmp = explode(':', $http_host);
  $options['base_url'] .= ':' . end($http_host_tmp);
}

This also means that we need a new issue to change the examples in default.settings.php for Drupal 8!

Unassigning since I cannot fix this right now, feel free to jump in ;-)

Should this be set back again on Drupal 8 because it has the same problem? :/

Gabor,

Difficult question, I think we should decide what $base_url is/will be and what values are acceptable. But my experience is limited to the first option ($base_url = 'http://www.example.com';), I never used any of the other options.

I guess we need people using reverse proxies (Varnish), load balancers, ... to give feedback, so we can determine the impact (and see if it's even possible).

Back to 8.x, we either

1. Fix this as in #42 and create a follow-up issue for $base_url discussion
2. We discuss $base_url first, postponing this fix

I think 1 is the sanest to do?

Yeah, let's fix like #42 in Drupal 8 for now I think.

According to KrisBuytaert: "mostly using it for rev proxy setups" (twitter) so in that case we cannot alter the $base_url, AFAIK #42 respects that.

I'm claiming this for the current sprint at DrupalCon.

added #42 to drupal 8 core/modules/language/language.negotiation.inc

diff --git a/core/modules/language/language.negotiation.inc b/core/modules/language/language.negotiation.inc
index 48a8083..bfe3a27 100644
--- a/core/modules/language/language.negotiation.inc
+++ b/core/modules/language/language.negotiation.inc
@@ -401,7 +401,7 @@ function language_url_rewrite_url(&$path, &$options) {
           // HTTP_HOST will optionally contain the port. Preserve that for
           // the domain of the target language as well.
           $http_host = $_SERVER['HTTP_HOST'];
-          if (strpos($http_host, ':') !== FALSE) {
+          if (strpos($http_host, ':') !== FALSE && strpos($options['base_url'], ':') === FALSE) {
             $http_host_tmp = explode(':', $http_host);
             $options['base_url'] .= ':' . end($http_host_tmp);
           }

So this took a bit longer than expected.

1. The existing test was broken when using dirty URLs
2. We override $options['base_url'] before we do the port checking, but we want to retain a possible port from there.

+++ b/core/modules/language/language.negotiation.inc
@@ -390,20 +390,37 @@ function language_url_rewrite_url(&$path, &$options) {
+        global $is_https;
+        $url_scheme = ($is_https) ? 'https://' : 'http://';
...
         if (is_object($options['language']) && !empty($domains[$options['language']->langcode])) {
...
-          global $is_https;
-          $url_scheme = ($is_https) ? 'https://' : 'http://';

Oops, that change is accidental.

+++ b/core/modules/language/language.negotiation.inc
@@ -390,20 +390,37 @@ function language_url_rewrite_url(&$path, &$options) {
-            $http_host_tmp = explode(':', $http_host);
-            $options['base_url'] .= ':' . end($http_host_tmp);
...
+            list($host, $port) = explode(':', $http_host);
+            $options['base_url'] .= ':' . $port;

This change is not strictly necessary, but I find the list() syntax much more readable, because the variables, actually have a meaning.

+++ b/core/modules/language/language.negotiation.inc
@@ -390,20 +390,37 @@ function language_url_rewrite_url(&$path, &$options) {
+debug($original_base_url);
+debug($options['base_url']);
+debug($host);
+debug($port);

Oops

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php
@@ -106,6 +106,11 @@ class LanguageUrlRewritingTest extends WebTestBase {
+debug(url('', array('absolute' => TRUE)));

...

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php
@@ -114,9 +119,27 @@ class LanguageUrlRewritingTest extends WebTestBase {
-    $url = url('', array('absolute' => TRUE, 'language' => $language));
+    $url = url('', array(
+      'absolute' => TRUE,
+      'language' => $language,
+    ));

This is not strictly necessary, but I wanted to make it consistent with the call below.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php
@@ -114,9 +119,27 @@ class LanguageUrlRewritingTest extends WebTestBase {
-    $this->assertTrue(strcmp($url, 'http://example.fr:88/') == 0, 'The right port is used.');
+    $this->assertEqual($url, $expected, 'A given port is not overriden.');

I don't why strcmp() was used here, I don't see a reason not to use assertEqual()

Will roll a followup patch in a moment.

This one is better, I hope. Bring on those reviews! :-)

Oops I only now saw #48. That was a crosspost. Anyway, as point 2. in #50 elaborates, that approach does not work, at least not directly.

Ahhh, I uploaded the same files again. I'm sorry. Here we go.

Nice work, some minor remarks

+++ b/core/modules/language/language.negotiation.incundefined
@@ -392,18 +392,31 @@ function language_url_rewrite_url(&$path, &$options) {
+            $original_base_url = str_replace($url_scheme, '', $options['base_url']);

isn't it safer to do str_replace(array('http://', 'https://'), '' .... so if gets removed no matter what is in there.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.phpundefined
@@ -114,9 +118,27 @@ class LanguageUrlRewritingTest extends WebTestBase {
+    $expected = 'http://example.fr:88/';
+    $expected .= $index_php ? 'index.php/' : '';

i think it looks cleaner on 1 line

1. I personally don't really know how all the different SSL settings work, I just noticed that this seems to work. Your proposal definitely can't hurt, I'll reroll with that.

2. I did it like that, so we don't need to duplicate the actual URL, but I don't really care either. I'll reroll with that.

Here we go.

Thinking about the $url_scheme / $base_url stuff, I actually think in case the base_url is given, we should take over the scheme, instead of relying on $is_https, just like we take over the port (with this patch).

I'm not sure that that should be part of this patch, though.

$options['base_url'] contains the Protocol (http://, 'https://', ...), so just checking for : is not the best idea ;-)

I shouldn't do that, and certainly not is this patch. The underlying problem is that you can only specify 1 $base_url containing a full URL including scheme and port, which should in a follow-up issue be changed so you only specify the FQDN so it is consistent and works regardless of the scheme and/or port.

FYI: I talked some more to other devops and most of them never use $base_url for Drupal 7 when using a reverse proxy.

I'm new to this issue and the specific topic itself. So instead of using the base url $_SERVER['SERVER_PORT'] may be better.
But i'm not sure if this port problem should be handled in this issue itself or if it is a more generic problem.

If the port is always given in global $base_url, that could be a workaround to parse the url and get the port.
I can change the patch to parse global $base_url, but my knowledge of the core is not good enough to handle this issue if it should be fixed in a more generic way within Drupal.

I think we should stay with the patch from #57 and handle all the rest in a separate issue.

Re uploading patch from #57

I agree. I definitely there should be a follow-up where we can discuss this in detail. Also because, the whole HTTP/HTTPS thing needs the same discussion.

But this patch fixes an edge-case, but legitimate bug, so let's ship it!

RTBC anyone?

It also looks good to me. The only thing I've seen is that there is not use of t() function in the tests assertions. In the other parts in core, t() is used, but I am not sure if this is a good thing or not. Anyhow, I think we should follow a common path, either we use t() all the time, or we never use it.

t() calls are in progress of being removed from assertion messages. We should not use it for assertion messages.

In that case, I think this can be RTBC.

+++ b/core/modules/language/language.negotiation.incundefined
@@ -392,18 +392,31 @@ function language_url_rewrite_url(&$path, &$options) {
+
+          // Save the original base URL. If it contains a port, we need to
+          // retain it below.
+          if (!empty($options['base_url'])) {
+            // The colon in the URL scheme messes up the port checking below.
+            $original_base_url = str_replace(array('https://', 'http://'), '', $options['base_url']);
+

If we're saving the original base URL, then we'd be saving the whole thing, not the base URL minus the protocol. We could call it something like 'normalized base url'?

Otherwise I think this is probably fine.

new patch

#67: i1645156.patch queued for re-testing.

#67: i1645156.patch queued for re-testing.

I'm not sure I am allowed to RTBC this. It looks good, though.

Resolves the concern from @catch. Back to RTBC.

Awesome, thanks so much for this fix!

Committed and pushed to 8.x. Moving to 7.x for backport.

First attempt for 7.x
I still have to figure out backporting the test changes.

Disregard previous patch at all, still making my way through language system (= warning, novice at the wheel).
At least this should be modifying the appropiate code. Nothing broke... I'm starting to suspect that I must have something misconfigured :)

Nice work, patch is looking good, but we still need the test. If you write the test, can you upload a patch with only the test first so we can see that it will break as you did in #54

Removing from D8MI sprint, where we focus on D8 :)

Unassigning, as I'm not involved with the D7 port.

The patch still applies cleanly against 7.20 and works like a charm.

Updated issue summary.

What happened to this issue? It used to be a D7 release blocker ...

78: drupal--i1645156-78.D7.patch queued for re-testing.

With more sites moving to using https only, this limitation is becoming more annoying.

The patch in #78 still applies. What do we need to do to get this into D7?

Looks like the tests still need to be backported to D7. See #79.

Ok, setting it back to Needs work then until we get a patch with those. Thanks @dcam for your work on these issues.

For me the patch does not work in Drupal 7.34. $options['base_url'] is not set, but i have a $base_url in settings.php.

The patch from #78 applies cleanly to 7.41. As requested, here is the tests-only patch backported + re-rolled from #54 and #40. Hoping for a quick RTBC for getting the issue fixed in D7 as it has been in D8.

Because the added tests were supposed to fail until this issue gets fixed...

Thank you for your work, @talhaparacha!

A patch which combines the test and the fix must be created now. The issue can't be RTBCed without demonstrating that the fix corrects the test failures.

Edit: nevermind about the missing test failure. It was only reporting a single failure when I wrote that, but now it reports two.

@dcam Understood. Here we go; combined patch (fix from #78 and tests from #89) along with the tests-only patch.

Patch applies cleanly to 7.41, and manually tested that urls correctly retain the port (if set). Also URLs continue to work correctly if the default port is set and not specified in the URL. #94 demonstrates that the patch (with test) fixes the failing test, so marking this as RTBC.

Nice work! Those i18n patches always have such a nice structure and are well cared for! Thank you!

Marking for D7 commit.

Code is essentially the same as D8, backport looks OK to me

Committed and pushed to 7.x! Thanks!