Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#48 | i1572394-48.patch | 1.15 KB | crotown |
#37 | i1572394-37.patch | 1.77 KB | attiks |
#19 | i1572394-19.patch | 701 bytes | attiks |
#13 | i1572394-13-test.patch | 712 bytes | attiks |
#13 | i1572394-13.patch | 1.73 KB | attiks |
Comments
Comment #1
Sweetchuckquick fix
Problem with this patch: language detection cannot be port number based.
Comment #2
Gábor HojtsyTo 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.
Comment #3
Gábor HojtsyBTW the same code in Drupal 8 is in language_from_url() and is in core/modules/language/language.negotiation.inc.
Comment #4
Gábor HojtsyComment #5
Gábor HojtsyI'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!
Comment #6
attiks CreditAttribution: attiks commentedI 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.
Comment #7
attiks CreditAttribution: attiks commentedI don't know if this is the fastest code:
$http_host = current(explode(':', $http_host));
Comment #8
attiks CreditAttribution: attiks commentedCorrect version of i1572394-6.patch
Comment #10
attiks CreditAttribution: attiks commented#8: i1572394-6.patch queued for re-testing.
Comment #11
Gábor HojtsyLooks 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.
Comment #12
catchThanks. Committed/pushed to 8.x, moving to 7.x for backport.
Comment #13
attiks CreditAttribution: attiks commentedComment #14
Gábor HojtsyLooks good to go to me if it passes tests :)
Comment #15
attiks CreditAttribution: attiks commentedLet's hope the first one fails and only the second passes ;p
Comment #16
David_Rothstein CreditAttribution: David_Rothstein commentedAccording 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?
Comment #17
Gábor HojtsyIn that case, the D8 one would need to be fixed first (again). Doh.
Comment #18
attiks CreditAttribution: attiks commentedI'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
Comment #19
attiks CreditAttribution: attiks commentedComment #20
Robin Millette CreditAttribution: Robin Millette commentedInstead of this
I generally use
Comment #21
attiks CreditAttribution: attiks commentedRobin, i don't mind using your code, but will it make a difference?
Comment #22
Robin Millette CreditAttribution: Robin Millette commentedI 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.
Comment #23
Alan D. CreditAttribution: Alan D. commentedFrom 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.
This can throw warnings if there were no ":" in the $http_host string.
Comment #24
attiks CreditAttribution: attiks commented@catch which one do you prefer?
Comment #25
Gábor HojtsyWhy would list() = explode() throw warnings if there was no :, and how was that avoided then with current(explode()) earlier?
Comment #26
attiks CreditAttribution: attiks commented@Gábor: I think the last example from #23 can throw warnings if there's no ':'. Do you prefer #19 or #20?
Comment #27
Gábor Hojtsy@attiks: yeah, I asked why? Why would that throw a warning and current(explode()) would not?
Comment #28
attiks CreditAttribution: attiks commentedcurrent(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. AFAIKlist($http_host) = explode(':', $http_host);
will never throw a warning. Andlist($http_host,$port) = explode(':', $http_host);
will complain ("Undefined offset: 1") if there's no ':' inside $http_host.I hope it answers your question.
Comment #29
Gábor HojtsyOh, 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? :)
Comment #30
attiks CreditAttribution: attiks commentedMy idea exactly :) , but since I have your attention: Do you prefer #19 or #20?
Comment #31
Gábor HojtsyI don't really care, either works.
Comment #32
attiks CreditAttribution: attiks commentedRTBC for #19?
Comment #33
Gábor HojtsyComment #34
Alan D. CreditAttribution: Alan D. commentedSorry guys, I should have made that clearer! It was in response to "list() is quite safe in my experience, no secondary effects".
Comment #35
sun#19: i1572394-19.patch queued for re-testing.
Comment #36
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks! Moving to 7.x.
Comment #37
attiks CreditAttribution: attiks commentedComment #38
Gábor HojtsyStraight backport.
Comment #39
David_Rothstein CreditAttribution: David_Rothstein commentedThis and several other issues look like they weren't actually committed/pushed. So, #19 is still RTBC for Drupal 8.
Comment #40
Dries CreditAttribution: Dries commentedPushed it now, I hope. :)
Comment #41
Gábor HojtsyIndeed: http://drupalcode.org/project/drupal.git/commitdiff/10631112ea0bdaa43ca9...
Comment #42
SweetchuckVery big thank you for every body :-)
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/4777845
Comment #44
Gábor HojtsyNot on the sprint anymore than. Thanks.
Comment #45
attiks CreditAttribution: attiks commentedComment #46
SweetchuckTo be continued #1645156: URL generation only works on port 80 when using domain based language negotation
Comment #48
crotown CreditAttribution: crotown commentedHere 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.
Comment #49
crotown CreditAttribution: crotown commentedPutting back tags I removed.
Comment #50
Gábor HojtsyThe 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?
Comment #51
crotown CreditAttribution: crotown commentedI 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.
Comment #52
crotown CreditAttribution: crotown commentedThe 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.
Comment #53
Gábor HojtsyRelated: #1645156: URL generation only works on port 80 when using domain based language negotation (that is about generation not detection).