Posted by sethiele on August 16, 2011 at 1:06pm
24 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | language system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | D8MI, language-base, negotiation |
Issue Summary
Problem/Motivation
When I add a new language and fill out the details form (/admin/config/regional/language/edit/de e.g. German), I use the language domain. There I have to add the protocol. But this added protocol overwrite the "normal" switch betwene http and https.
This means when I add http://www.page.de and open the page with http://www.page.de all is OK. But if I open the page with https://www.page.de all links are changed to http and i lost my https (in links an for form targets).
Proposed resolution
I think the protocol should be removed OR the module should change the protocol to the active one.
Resolution
The check only happens on the domain name, both the protocol and port are ignored.
Comments
#1
This needs to be fixed in d8 first and backported
#2
The two proposed solutions are about UX. Do people copy&paste the url from the browser or do they type in the url?
Additional this is a bug, but the current UI shouldn't be changed. If we would remove the protocol from the form, the strings has to be changed etc.
So the probably better solution is to change the protocol to the active one, as this isn't a too big change.
Removing this "feature" to have different languages per http/https isn't a big thing, as i thing this isn't a wanted feature.
#3
The last submitted patch, 1250800-locale-protocoll-independent.patch, failed testing.
#4
New patch attached, 2 fixes:
1/ parse_url expects a protocol, so for domain all https:// get replaced by http://
2/ fixed the if statement
Tested locally and should be ok
#5
+++ b/includes/locale.incundefined@@ -207,8 +207,11 @@ function locale_language_from_url($languages) {
+ $domain = str_replace(array('http://', 'https://'), array('http://', 'http://'), $language->domain);
It would be enought to replace https with http
6 days to next Drupal core point release.
#6
good catch
#7
even cleaner patch
#8
The last submitted patch, i1250800.patch, failed testing.
#9
try again
#10
#11
I think its a good strategy to compare the hosts only, not protocols. Not sure we should hardcode http and https there though. We can use parse_url() to get the right component of the $_SERVER['HTTP_HOST'], just like we do for the locale setting. Consistency is good :)
#12
Changed a bit, see comment
// Only compare the domains not the protocols or ports.
// Server HTTP_HOST should contain only the host, but double check
// Remove protocol and add http:// so parse_url works
Testing will be a PITA, the simpletest framework doesn't like switching to https while running inside http. And there's a very slim change testbots run http on different ports
#13
A little bit more clarification, on my test machine $_SERVER['HTTP_HOST'] returns just the hostname, without the protocol, parse_url needs a protocol to do its work.
#14
for http/https test, see http://drupalcode.org/project/securepages.git/blob/HEAD:/securepages.test
#15
first try to adding http/https mixed testing
#16
tests will probably fail, the https login works, but all requests after return 403
#17
New patch attached since admin/config/regional/language/edit/xx doesn't check for http, I created a separate issue for this #1261440: Fix language domain configuration description vs. reality, but in the end that issue has to change the description of the language domain.
#18
I opened a separate issue for the test, see #1261454: Add tests for http/https language domain negotiation
#19
Fixing tag.
#20
Retiling for the wider range it covers, and given other issues on ports were marked duplicate.
Looking at the patch, it seems to recompute the $server_host variable for each language, although that is language independent. Also, as per the previous code and http://php.net/manual/en/reserved.variables.server.php, HTTP_HOST should not contain any protocol, it comes from the request headers. So attempting to remove the protocol from there seems like futile?
#21
Tagging.
#22
@Gabor you're right, can not even remember why it was added.
This patch also include the test from #1261454: Add tests for http/https language domain negotiation
#23
New patch including #1261440: Fix language domain configuration description vs. reality
#24
+++ b/modules/locale/locale.testundefined@@ -1901,7 +1901,7 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
// URL.
- $edit = array('prefix' => '', 'domain' => "http://$language_domain");
+ $edit = array('prefix' => '', 'domain' => "https://$language_domain:99");
It would be definitive helpful to document why https is used here
About the str_replace, this code works as expected
$foo = 'tttttattttb';$foo = str_replace(array('a', 'b'), '', $foo);
dsm($foo);
So updated the patch based on this.
#25
Fix typo
#26
rerolled, added description back and added some more comments
#27
+++ b/includes/locale.incundefined@@ -207,10 +207,16 @@ function locale_language_from_url($languages) {
+ $host = 'http://' . str_replace(array('http://', 'https://'), '', $language->domain); ¶
Here are some trailing whitespaces.
The rest looks fine. This patch itself can't be backported 1by1 perhaps there is a string change.
#28
spaces are gone.
i'll backport if this is is accepted.
#29
+++ b/modules/locale/locale.admin.incundefined@@ -307,7 +307,7 @@ function _locale_languages_common_controls(&$form, $language = NULL) {
- '#description' => t('URL <strong>including protocol</strong> to use for this language, if your <em>Detection and selection</em> settings use URL domains. For the default language, this value may be left blank. <strong>Modifying this value may break existing URLs. Use with caution in a production environment.</strong> Example: Specifying "http://example.de" or "http://de.example.com" as language domains for German results in URLs like "http://example.de/contact" and "http://de.example.com/contact", respectively.'),
It's kind of problematic that the text is changed quite a bit... For example why did you removed the second example at the end?
-3 days to next Drupal core point release.
#30
@dereine: i didn't, see #1261440: Fix language domain configuration description vs. reality
#31
Well, to make the description shorter of course. One example is enough to get the point across.
#32
new patch, fixed the missing ", and changed URLs to an URL
#33
Thanks for the clarification
Patch looks fine now for me, the description is also better now.
#34
Coming here from #1261454: Add tests for http/https language domain negotiation where there was an issue with the testbots not doing https. I don't actually know why they shouldn't get that setup if it's important; If tests work locally and you just need the testbots to do it, please open an issue in http://drupal.org/project/issues/drupaltestbot.
#35
Moved my original request to the right queue, #1261522: Can all bots run https test
#36
Looks good. Committed to 8.x.
#37
D7 patch
#38
again
#39
clean backport, no changes, just rerolled
#40
+++ b/includes/locale.inc@@ -207,10 +207,16 @@ function locale_language_from_url($languages) {
+ $host = 'http://' . str_replace(array('http://', 'https://'), '', $language->domain);
Form validation in locale.admin.inc should be responsible for ensuring that the domain value does not contain a protocol prefix. Doing this on every single request is not performant.
+++ b/modules/locale/locale.test@@ -1930,8 +1930,8 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
- $edit = array('prefix' => '', 'domain' => "http://$language_domain");
+ // URL. We use https and a port to make sure that only the domain name is used.
+ $edit = array('prefix' => '', 'domain' => "https://$language_domain:99");
I don't see where this is testing that negotiation with a custom protocol or port actually works?
20 days to next Drupal core point release.
#41
Form validation in locale.admin.inc should be responsible for ensuring that the domain value does not contain a protocol prefix. Doing this on every single request is not performant.You are right that this should be definitive added to the form validation, but the reason why it is there, is that currently the saved domain has http/https in it, so it should still work. Sadly the patch got commited already.
#42
Could we add an update to correct existing entries for D7?
Does this need to be moved back to 8.x to remove that first?
#43
#40: I don't see where this is testing that negotiation with a custom protocol or port actually works?
I'm probably missing something but the actual request made goes to http://$language_domain, so if it passes it means the patch works, both protocol and port number aren't taken into account.
The reason it is written as such is because the testbots don't like https and different port numbers.
#44
#40: Doing this on every single request is not performant.
You're right, but updating existing D7 can be a problem because locale_add_language can be added to install profiles and they will break when domain has to be more restrictive than it is now. But you're right locale.admin.inc has to check this properly in D8, I'll opened a follow up issue #1272840: Add upgrade path for language domains and validation to fix this and provide an upgrade path.
#45
Agreed that for Drupal 7 it is not possible for backwards compatibility to modify the domain format. We can do so in Drupal 8. @attiks please move over the arguments and provide some description on #1272840: Add upgrade path for language domains and validation. Let's keep this issue for the backportable patch that does not change the domain's expected format.
#46
Okay the first concern of sun can't happen in d7, so make the issue to needs review to see what the testbot thinks.
#47
#38: i1250800_7.patch queued for re-testing.
#48
We have a followup for sun's concern for D8, so moving this back to RTBC for 7.x.
#49
Committed to 7.x. Marking fixed.
#50
Automatically closed -- issue fixed for 2 weeks with no activity.
#51
Just helped a user named @pomliane on IRC with an issue related to this. Now the instructions provided are broken since they say you should specify the setting without a protocol/port while the code that actually uses the setting was not updated to add a protocol/port when generating absolute URLs. So if you follow the instructions and specify de.example.com, you get links like http://example.com/de.example.com/node instead of http://de.example.com. The URL generation portion of the code should be updated as well.
This bug applies to both Drupal 8.x and 7.x (@pomliane hit it on 7.10).
Drupal 7 should definitely need a fix that transforms the value on the fly proper to avoid an API change. Drupal 8 already has an issue at #1272840: Add upgrade path for language domains and validation which could be extended to cover URL generation, however, we need to take care of having the fix in D8 and D7 as well, so therefore my thinking to reopen this one first on D8.
#52
Patch for D8 attached, it should fix it
#53
The last submitted patch, i1250800_52.patch, failed testing.
#54
#52: i1250800_52.patch queued for re-testing.
#55
That looks like it would need tests. It did that not appear as a bug when the patch was committed because the expected URL is not tested.
#56
Tagging as current focus.
#57
Tagging for base language system.
#58
Patch for D8, including test for the url() function
#59
Fixed a comment typo
#60
+++ b/core/modules/locale/locale.testundefined@@ -2212,6 +2212,40 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+
two empty lines
-26 days to next Drupal core point release.
#61
new patch based on #60
#62
Marking needs review for the test. Looks like it does not actually pass?
#63
Tagging for language negotiation too.
#64
I ran the tests as well and they pass, let's wait for the bot
#65
The last submitted patch, i1250800_61.patch, failed testing.
#66
patch to see what's going on
#67
The last submitted patch, i1250800_66.patch, failed testing.
#68
very strange ...
#69
The last submitted patch, i1250800_68.patch, failed testing.
#70
anyone an idea why base_path() returns '/checkout/' in the testbot environment but the url() function does not append it to the url?
#71
I think that is likely due to test system internals. I'd check any other language domain tests already there. How do they do this?
#72
I changed the test, apparently base_path isn't really working on the testbot, and it looks like there's no proper solution, so I hard-coded the right URL.
#73
Correct me if I'm wrong but it looks like the current code and patches is still hard-coding url *output* to http:// and not supporting $options['https']? We seem to be missing a test condition for
$italian_url = url('admin', array('https' => TRUE, 'language' => $languages['it']));#74
@Dave Reid: Very true. Should we also carry over ports to the other domain in language switching, if we are running custom ports? (Possibly).
#75
@Dave Reid: good catch, I quickly added the test, but it's failing, to be continued ...
@Gábor: I'll have a look for custom port tests
#76
New patch with https support
#77
@attiks: I don't think "HTTPS support" would be that if the url() was requested with https, we use https and otherwise not. Most, if not all url() calls in Drupal core don't specify if they want HTTPS, they just expect if the request was on HTTPS, it would be retained. I'm not sure how the 'https' variable is used/computed, but looking at url() it seems like it would fall back on $base_url (the current base URL either on https or http) if the option to use either was not directly specified. That seems to be missing from here to carry over the protocol from the current base URL to the language base URL, right? Or am I missing something?
#78
New patch based on #77
#79
The last submitted patch, i1250800_78.patch, failed testing.
#80
fixed patch with extra test for current url scheme.
#81
#82
forgot to check the 'https' variable
#83
I've asked @davereid on IRC to look at the patch, and he said it looks good. I reviewed the patch myself too. I was concerned for the $options['https'] part but since url() will only do that transformation for non-existent base urls and for external URLs (which makes sense), we need to do that here. So my comments only pertain to the test so we can tighten it up and get this in.
+++ b/core/modules/locale/locale.testundefined@@ -2212,6 +2212,59 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+ $langcode_browser_fallback = 'it';
+ $language = (object) array(
+ 'langcode' => $langcode_browser_fallback,
+ );
+ language_save($language);
+ $languages = language_list();
Let's call this $langcode, its not about browser fallback, is it? I think this is a result of copy-paste.
+++ b/core/modules/locale/locale.testundefined@@ -2212,6 +2212,59 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+ $edit = array(
+ 'language[enabled][locale-browser]' => TRUE,
+ 'language[enabled][locale-url]' => TRUE,
+ 'language[weight][locale-browser]' => -8,
+ 'language[weight][locale-url]' => -10,
+ );
Why do we enable/need the browser detection method? Does that change anything for this test? We only do url() not doing request tests here, so looks like not.
+++ b/core/modules/locale/locale.testundefined@@ -2212,6 +2212,59 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+ $correct_link = $url_scheme . 'it.example.com/?q=admin';
...
+ $correct_link = 'https://it.example.com/?q=admin';
...
+ $correct_link = 'https://it.example.com/?q=admin';
Can we unify these somewhere and compute the 'it.example.com/?q=admin' part first with a condition on whether the testbot is running with clean URLs or not just to ensure we don't expect it runs without clean URLs for sure?
+++ b/core/modules/locale/locale.testundefined@@ -2212,6 +2212,59 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+ // Test https via options
...
+ //Test https via current url scheme
Comments should have a dot at end of line and lead with a space after //.
Thanks!
#84
Patch based on #83 ;-)
#85
Ok, looks good to me know.
#86
Looks good. I've committed/pushed this to 8.x, moving back to 7.x for backport.
#87
There is also an upgrade path component to be worked on in #1272840: Add upgrade path for language domains and validation, contributions welcome there too!
#88
The docblock is goofy. I'll roll a followup for D8 and a D7 backport. (References: http://drupal.org/node/1354#functions, http://drupal.org/node/1354#general)
#89
Simple doc fix for D8. The D7 backport is mostly naïve. I replaced
$domains[$options['language']->langcode]with$options['language']->domainin D7, but no other changes, so please review the D7 patch closely. Thanks!#90
Oh, and it has tests, so untagging.
#91
Nobrainer for D8 (but minor).
#92
Committed to 8.x. Moving to 7.x. Thanks!
#93
Needs at least the patch posted for the testbot on d7 and some reciew as xjm pointed out.
#94
Renamed for the bot.
#95
Oops, and restoring priority.
#96
The last submitted patch, 1250800-94.patch, failed testing.
#97
There are at least two points where this needs D7 backports:
+++ b/modules/locale/locale.testundefined@@ -2256,6 +2256,60 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+ $langcode = 'it';
+ $language = (object) array(
+ 'langcode' => $langcode,
+ );
+ language_save($language);
No langugae_save() in Drupal 7. Can/should use locale_add_language().
+++ b/modules/locale/locale.testundefined@@ -2256,6 +2256,60 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+ 'domain[it]' => 'it.example.com',
Domain is configured on the language (should be done above) in Drupal 7.
@Jelle_S, @attiks: can you help bring this forward?
#98
I'll have a look later today
#99
@attiks: any progress? Thanks!
#100
@Jelle_S is working on it right now, it works inside devel/php, but not as a test :/
#101
First try, please review closely
#102
The last submitted patch, i1250800_101.patch, failed testing.
#103
Strange: failing on
The English link points to the correct domain. Other upgrade.locale.test 114 LocaleUpgradePathTestCase->testLocaleUpgradeDomain()
Can someone do a local review, test is passing on my machine
#104
+++ b/modules/locale/locale.testundefined@@ -2264,6 +2264,50 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+ locale_add_language($langcode, 'Italian', 'Italian', LANGUAGE_LTR, 'it.example.com', '', TRUE, FALSE); ¶
trailing whitespace
+++ b/modules/locale/locale.testundefined@@ -2264,6 +2264,50 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+ variable_set('locale_language_negotiation_url_part', 1);
+ ¶
+ // Build the link we're going to test based on the clean url setting.
+ $link = (!empty($GLOBALS['conf']['clean_url'])) ? 'it.example.com/admin' : 'it.example.com/?q=admin';
+ global $is_https;
+ ¶
+ $languages = language_list();
tabs
I'm not sure why the test is failing but I saw these styling issues in dreditor.
#105
code cleaned
#106
removed debug stuff
#107
The last submitted patch, i1250800_106.patch, failed testing.
#108
@Gábor: need your help on why test bot is failing, attached patch includes a delete of the added language, but I'm afraid it won't help
#109
The last submitted patch, i1250800_108.patch, failed testing.
#110
Ran the tests and grabbed the output for this page. Problem if you attempted to backport this 1-1 to D7 but D7 we need to remove ports and protocols on the fly before using the config value. You don't do that. Your original patch included code for that in the negotiation code, so we'd have to do something similar here too.
#111
I fixed the domain inside drupal-6.locale.database, try again bot
#112
While that might pass, we should not change how a Drupal 6 database would look :) I guess you are making this change to see it pass first and then go back to not modify it and make the D7 version work proper :)
#113
@Gabor: I think we need #1272840: Add upgrade path for language domains and validation for Drupal 7 as well, this will solve this
#114
@attiks: *no*, that would break assumptions that contrib modules, install profiles, etc. make about how to set and what to expect in the domain setting. It could have ripple effects in various ways. We need to modify the code where it generates URLs to handle/parse/tear apart the domain setting like in the negotiation setting.
#115
@Gabor: something like this
#116
+++ b/includes/locale.inc@@ -430,8 +430,23 @@ function locale_language_url_rewrite_url(&$path, &$options) {
+ ¶
leading space
20 days to next Drupal core point release.
#117
adding test for domains with a prefix right now
#118
+++ b/includes/locale.incundefined@@ -430,8 +430,23 @@ function locale_language_url_rewrite_url(&$path, &$options) {
+ // Triple check that domain only includes domain name
+ // Remove protocol and/or ports
I think we should make this comment tell more about what is going on.
// Take the domain without ports or protocols so we can apply the protocol needed. The setting might include a protocol. This is changed in Drupal 8 but we need to keep backwards compatibility for Drupal 7.
(also wrap this properly :)
+++ b/includes/locale.incundefined@@ -430,8 +430,23 @@ function locale_language_url_rewrite_url(&$path, &$options) {
+ $options['base_url'] = $url_scheme . $host;
+ if (isset($options['https']) && variable_get('https', FALSE)) {
+ if ($options['https'] === TRUE) {
+ $options['base_url'] = str_replace('http://', 'https://', $options['base_url']);
+ }
+ elseif ($options['https'] === FALSE) {
+ $options['base_url'] = str_replace('https://', 'http://', $options['base_url']);
+ }
+ }
Add a one line comment above this to separate from the parse_url() wrangling IMHO. Something like.
// Apply the appropriate protocol to the URL.
+++ b/modules/locale/locale.testundefined@@ -2264,6 +2264,50 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+ locale_add_language($langcode, 'Italian', 'Italian', LANGUAGE_LTR, 'it.example.com', '', TRUE, FALSE);
Change this to save the language with http://it.example.com/ or somesuch to demonstrate/test the overall domain modification behavior in D7.
Or alternatively, add another language with the full protocol + domain setup and test URL generation for that too.
#119
New patch with extra tests
#120
Looks good now. Would it be possible to do a foreach on array('it', 'fr') in the test instead of copying the (almost) same code twice?
#121
+++ b/modules/locale/locale.testundefined@@ -2264,6 +2264,77 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+ // Add the Frech language, with protocol.
Frech language. :)
Also, it might be good to use
format_string()rather thant()on the assertion message strings. Reference: http://drupal.org/simpletest-tutorial-drupal7#t#122
with foreach
#123
Frech -> French
About the use of t(), all others are using this as well, so better fix it in a separate issue?
#124
Nah, core has it both ways, so we might as well use the correct one. :) Edit: However, I totally forgot this was a backport, so yeah, leave 'em alone for now. Thanks!
#125
I think this looks good on a visual review. I've asked a few people to look at it and help double-check.
#126
The patch in #123 seems to work. The correct URLs are generated whether or not you enter the protocol.
#127
Tested as well; couldn't make it break (which I guess is a good sigh ;) )
+1 for commit.
/Robin
#128
This should probably be tested on HTTPS before it's RTBC.
#129
I tested on http and https.
/Robin
#130
Committed and pushed to 7.x. Thanks!
#131
Off-sprint then. Thanks!
#132
needs backport to D7 removed
#133
Automatically closed -- issue fixed for 2 weeks with no activity.
#134
This resulted in a pretty unfortunate bug at #1572394: Language detection by domain only works on port 80. Anybody interested in helping solve it?