Domain based language detection always use the default language if the HTTP server listen to non 80 port.

$_SERVER['HTTP_HOST'] contains the port number if it is not 80.
$host does not contains port.

Files: 
CommentFileSizeAuthor
#48 i1572394-48.patch1.15 KBcrotown
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#37 i1572394-37.patch1.77 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 39,142 pass(es).
[ View ]
#19 i1572394-19.patch701 bytesattiks
PASSED: [[SimpleTest]]: [MySQL] 36,682 pass(es).
[ View ]
#13 i1572394-13-test.patch712 bytesattiks
FAILED: [[SimpleTest]]: [MySQL] 39,110 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#13 i1572394-13.patch1.73 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 39,111 pass(es).
[ View ]
#8 i1572394-6.patch1.83 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 36,605 pass(es).
[ View ]
#7 i1572394-6-test.patch723 bytesattiks
FAILED: [[SimpleTest]]: [MySQL] 36,592 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#7 i1572394-6.patch723 bytesattiks
FAILED: [[SimpleTest]]: [MySQL] 36,594 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#1 drupal-1572394.patch670 bytesSweetchuck
PASSED: [[SimpleTest]]: [MySQL] 39,053 pass(es).
[ View ]

Comments

Sweetchuck’s picture

Version:8.x-dev» 7.x-dev
Issue tags:-D8MI, -sprint, -language-base
StatusFileSize
new670 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,053 pass(es).
[ View ]

quick fix
Problem with this patch: language detection cannot be port number based.

Gábor Hojtsy’s picture

Version:7.x-dev» 8.x-dev
Status:Active» Needs review
Issue tags:+D8MI, +sprint, +language-base

To me it sounds like it would make more sense to remove the port from $_SERVER['HTTP_HOST'] and then use the resulting value to compare. I assume this applies to Crupal 8 as well, and needs to be fixed there first in that case.

Gábor Hojtsy’s picture

Version:7.x-dev» 8.x-dev
Status:Needs review» Needs work
Issue tags:+D8MI, +sprint, +language-base

BTW the same code in Drupal 8 is in language_from_url() and is in core/modules/language/language.negotiation.inc.

Gábor Hojtsy’s picture

Title:language detection on port non 80» Language detection by domain only works on port 80
Gábor Hojtsy’s picture

I've verified that indeed HTTP_HOST woud contain the port number (despite its name). This bug was relatively recently introduced to Drupal 7 I think with #1250800: Language domain should work regardless of ports or protocols, so it was only part of Drupal 7 for some time. Looks like people are not running (multilingual) Drupal sites on non-standard ports too often. Posted there asking for more people interested, but we really need you taking this further. Thanks!

attiks’s picture

Assigned:Unassigned» attiks

I checked RFC2616 and it states: "The Host request-header field specifies the Internet host and port number of the resource being requested", Host = "Host" ":" host [ ":" port ] and no port means use the default of the requested service.

I'll make a patch removing the port before doing the check.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new723 bytes
FAILED: [[SimpleTest]]: [MySQL] 36,594 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new723 bytes
FAILED: [[SimpleTest]]: [MySQL] 36,592 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

I don't know if this is the fastest code: $http_host = current(explode(':', $http_host));

attiks’s picture

StatusFileSize
new1.83 KB
PASSED: [[SimpleTest]]: [MySQL] 36,605 pass(es).
[ View ]

Correct version of i1572394-6.patch

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

The last submitted patch, i1572394-6.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review
Issue tags:+D8MI, +sprint, +language-base

#8: i1572394-6.patch queued for re-testing.

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

Looks good, should then be backported to D7 too. Let's get this in D8 and 7, so it can be part of the next D7 release too.

catch’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)
Issue tags:+needs backport to D7

Thanks. Committed/pushed to 8.x, moving to 7.x for backport.

attiks’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.73 KB
PASSED: [[SimpleTest]]: [MySQL] 39,111 pass(es).
[ View ]
new712 bytes
FAILED: [[SimpleTest]]: [MySQL] 39,110 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
Gábor Hojtsy’s picture

Looks good to go to me if it passes tests :)

attiks’s picture

Let's hope the first one fails and only the second passes ;p

David_Rothstein’s picture

+        $http_host = current(explode(':', $http_host));

According to #1517654: Strict warning by field_ui_table_pre_render(), this kind of thing can cause E_STRICT warnings.

Note that I've never been able to reproduce that myself, and don't understand why it would; I can reproduce it with reset(), end(), prev(), etc. because those change the passed-in array, but current() doesn't so I have no idea why PHP would take the array by reference.

However, there are a whole lot of people over on that other issue claiming that it does and that for them, gobs of warnings are being triggered. Maybe it depends on PHP version?

Gábor Hojtsy’s picture

Version:7.x-dev» 8.x-dev
Status:Needs review» Needs work

In that case, the D8 one would need to be fixed first (again). Doh.

attiks’s picture

I'll reroll both, but I'm also not able to reproduce this. The array is taken by reference for performance reasons.

FYI PHP 5.3.5 & PHP 5.3.10

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new701 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,682 pass(es).
[ View ]
Robin Millette’s picture

Instead of this

<?php
$http_host
= current(explode(':', $http_host));
?>

I generally use

<?php
list($http_host) = explode(':', $http_host);
?>
attiks’s picture

Robin, i don't mind using your code, but will it make a difference?

Robin Millette’s picture

I never used current(), so I'm not aware of its implications and changes over the years. But list() is quite safe in my experience, no secondary effects.

Alan D.’s picture

From the other thread, these are the settings that triggered the warning:

Linux www.example.com 2.6.18-028stab085.3-ent #1 SMP Mon Mar 21 19:57:43 MSK 2011 i686
PHP Version 5.2.17
error_reporting 6143

@regarding #20
This is fine too as you will always get at least one value.

<?php
list($http_host) = explode(':', $http_host);
?>

This can throw warnings if there were no ":" in the $http_host string.

<?php
list($http_host,$port) = explode(':', $http_host);
?>
attiks’s picture

@catch which one do you prefer?

Gábor Hojtsy’s picture

Why would list() = explode() throw warnings if there was no :, and how was that avoided then with current(explode()) earlier?

attiks’s picture

@Gábor: I think the last example from #23 can throw warnings if there's no ':'. Do you prefer #19 or #20?

Gábor Hojtsy’s picture

@attiks: yeah, I asked why? Why would that throw a warning and current(explode()) would not?

attiks’s picture

current(explode()) can throw a warning because the array passed to current() is passed by reference, but apparently it depends on the PHP version if you get a warning or not. AFAIK list($http_host) = explode(':', $http_host); will never throw a warning. And list($http_host,$port) = explode(':', $http_host); will complain ("Undefined offset: 1") if there's no ':' inside $http_host.

I hope it answers your question.

Gábor Hojtsy’s picture

Oh, shoot. There are two examples in #23, and "This can throw warnings" was inbetween the two. It was supposed to be meant for the second example, not the first I guess. I don't get why are we concerned about the second example though as we don't need that code (to extract the port separately). Why are we wasting our time discussing it? :)

attiks’s picture

My idea exactly :) , but since I have your attention: Do you prefer #19 or #20?

Gábor Hojtsy’s picture

I don't really care, either works.

attiks’s picture

RTBC for #19?

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community
Alan D.’s picture

Sorry guys, I should have made that clearer! It was in response to "list() is quite safe in my experience, no secondary effects".

sun’s picture

#19: i1572394-19.patch queued for re-testing.

Dries’s picture

Version:8.x-dev» 7.x-dev

Committed to 8.x. Thanks! Moving to 7.x.

attiks’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.77 KB
PASSED: [[SimpleTest]]: [MySQL] 39,142 pass(es).
[ View ]
Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

Straight backport.

David_Rothstein’s picture

Version:7.x-dev» 8.x-dev

This and several other issues look like they weren't actually committed/pushed. So, #19 is still RTBC for Drupal 8.

Dries’s picture

Pushed it now, I hope. :)

Gábor Hojtsy’s picture

Sweetchuck’s picture

Very big thank you for every body :-)

David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed
Issue tags:+7.15 release notes
Gábor Hojtsy’s picture

Issue tags:-sprint

Not on the sprint anymore than. Thanks.

attiks’s picture

Assigned:attiks» Unassigned
Sweetchuck’s picture

Status:Fixed» Closed (fixed)

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

crotown’s picture

Version:7.x-dev» 6.x-dev
Status:Closed (fixed)» Needs review
Issue tags:-needs backport to D7, -D8MI, -7.15 release notes+6.27 release notes
StatusFileSize
new1.15 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Here is a patch for Drupal 6.x rolled against the current 6.x branch. I did this for for a Acquia client. I want to contribute it back to the community. It is essentially the same as i1572394-19.path but backported to D6.

crotown’s picture

Issue tags:+D8MI, +7.15 release notes

Putting back tags I removed.

Gábor Hojtsy’s picture

The patch itself looks good. This was a set of issues for Drupal 7. Are you sure this is the only part of the set of issues that applies to Drupal 6? Did you or the customer successfully test/use the patch? Did anybody else test the patch?

crotown’s picture

I am not sure that this is the only part of larger set of Drupal 7 set of issues that applies to Drupal 6. I thought that my patch covered everything that the previous patch did except for testing code -- but it looks like I did not fully understand the code that I did not backport. Here is what I did...

I tested the patch by making a multilingual local clone of the client's site and verifying that a URL like fr.example.local:8080 is now seen as equivalent to fr.example.local and selects the French version of example.local.

The client issue was that they wanted to set up a local site that worked the same way as their live site example.com, where picking "French" out of a dropdown menu redirects to "fr.example.com" and results in a French version of the site be served using the same database that serves all the different languages. This worked for the local site after applying my patch, and not before since the ":8080"s caused ALL of the comparisons to fail when checking for the versions of the URL mapping to different language version of the site. I performed the test by writing the strings that were compared into a temporary file. I made the patch by removing that one line after the code was working (and testing once again).

I haven't heard back from the customer yet, to see if it is working for them, as well. I will check with them an report back here.

crotown’s picture

The customer has not tried the patch yet.

They have just switched to MAMP Pro for a local environment. It seems they have not grappled with the fact that this issue will recur as long as they need to include ":xxxx" to get a local site. Or perhaps they have configured around it.

A developer there is interested trying the patch and getting involved in this issue.

Gábor Hojtsy’s picture

Related: #1645156: URL generation only works on port 80 (that is about generation not detection).