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.
Comment | File | Size | Author |
---|---|---|---|
#123 | i1250800_123.patch | 4.49 KB | attiks |
#122 | i1250800_121.patch | 4.49 KB | attiks |
#119 | i1250800_119.patch | 5.91 KB | attiks |
#115 | i1250800_115.patch | 4.07 KB | attiks |
#111 | i1250800_111.patch | 4.37 KB | attiks |
Comments
Comment #1
marcingy CreditAttribution: marcingy commentedThis needs to be fixed in d8 first and backported
Comment #2
dawehnerThe 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.
Comment #4
attiks CreditAttribution: attiks commentedNew 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
Comment #5
dawehnerIt would be enought to replace https with http
6 days to next Drupal core point release.
Comment #6
attiks CreditAttribution: attiks commentedgood catch
Comment #7
attiks CreditAttribution: attiks commentedeven cleaner patch
Comment #9
attiks CreditAttribution: attiks commentedtry again
Comment #10
attiks CreditAttribution: attiks commentedComment #11
Gábor HojtsyI 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 :)
Comment #12
attiks CreditAttribution: attiks commentedChanged 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
Comment #13
attiks CreditAttribution: attiks commentedA 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.
Comment #14
attiks CreditAttribution: attiks commentedfor http/https test, see http://drupalcode.org/project/securepages.git/blob/HEAD:/securepages.test
Comment #15
attiks CreditAttribution: attiks commentedfirst try to adding http/https mixed testing
Comment #16
attiks CreditAttribution: attiks commentedtests will probably fail, the https login works, but all requests after return 403
Comment #17
attiks CreditAttribution: attiks commentedNew 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.
Comment #18
attiks CreditAttribution: attiks commentedI opened a separate issue for the test, see #1261454: Add tests for http/https language domain negotiation
Comment #19
webchickFixing tag.
Comment #20
Gábor HojtsyRetiling 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?
Comment #21
Gábor HojtsyTagging.
Comment #22
attiks CreditAttribution: attiks commented@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
Comment #23
attiks CreditAttribution: attiks commentedNew patch including #1261440: Fix language domain configuration description vs. reality
Comment #24
dawehnerIt would be definitive helpful to document why https is used here
About the str_replace, this code works as expected
So updated the patch based on this.
Comment #25
dawehnerFix typo
Comment #26
attiks CreditAttribution: attiks commentedrerolled, added description back and added some more comments
Comment #27
dawehnerHere are some trailing whitespaces.
The rest looks fine. This patch itself can't be backported 1by1 perhaps there is a string change.
Comment #28
attiks CreditAttribution: attiks commentedspaces are gone.
i'll backport if this is is accepted.
Comment #29
dawehnerIt'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.
Comment #30
attiks CreditAttribution: attiks commented@dereine: i didn't, see #1261440: Fix language domain configuration description vs. reality
Comment #31
yoroy CreditAttribution: yoroy commentedWell, to make the description shorter of course. One example is enough to get the point across.
Comment #32
attiks CreditAttribution: attiks commentednew patch, fixed the missing ", and changed URLs to an URL
Comment #33
dawehnerThanks for the clarification
Patch looks fine now for me, the description is also better now.
Comment #34
rfayComing 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.
Comment #35
attiks CreditAttribution: attiks commentedMoved my original request to the right queue, #1261522: Can all bots run https test
Comment #36
Dries CreditAttribution: Dries commentedLooks good. Committed to 8.x.
Comment #37
attiks CreditAttribution: attiks commentedD7 patch
Comment #38
attiks CreditAttribution: attiks commentedagain
Comment #39
attiks CreditAttribution: attiks commentedclean backport, no changes, just rerolled
Comment #40
sunForm 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.
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.
Comment #41
dawehnerYou 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.
Comment #42
catchCould we add an update to correct existing entries for D7?
Does this need to be moved back to 8.x to remove that first?
Comment #43
attiks CreditAttribution: attiks commented#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.
Comment #44
attiks CreditAttribution: attiks commented#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.
Comment #45
Gábor HojtsyAgreed 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.
Comment #46
dawehnerOkay the first concern of sun can't happen in d7, so make the issue to needs review to see what the testbot thinks.
Comment #47
attiks CreditAttribution: attiks commented#38: i1250800_7.patch queued for re-testing.
Comment #48
catchWe have a followup for sun's concern for D8, so moving this back to RTBC for 7.x.
Comment #49
Dries CreditAttribution: Dries commentedCommitted to 7.x. Marking fixed.
Comment #51
Gábor HojtsyJust 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.
Comment #52
attiks CreditAttribution: attiks commentedPatch for D8 attached, it should fix it
Comment #54
Gábor Hojtsy#52: i1250800_52.patch queued for re-testing.
Comment #55
Gábor HojtsyThat 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.
Comment #56
Gábor HojtsyTagging as current focus.
Comment #57
Gábor HojtsyTagging for base language system.
Comment #58
Jelle_SPatch for D8, including test for the url() function
Comment #59
Jelle_SFixed a comment typo
Comment #60
attiks CreditAttribution: attiks commentedtwo empty lines
-26 days to next Drupal core point release.
Comment #61
Jelle_Snew patch based on #60
Comment #62
Gábor HojtsyMarking needs review for the test. Looks like it does not actually pass?
Comment #63
Gábor HojtsyTagging for language negotiation too.
Comment #64
attiks CreditAttribution: attiks commentedI ran the tests as well and they pass, let's wait for the bot
Comment #66
attiks CreditAttribution: attiks commentedpatch to see what's going on
Comment #68
attiks CreditAttribution: attiks commentedvery strange ...
Comment #70
Jelle_Sanyone an idea why base_path() returns '/checkout/' in the testbot environment but the url() function does not append it to the url?
Comment #71
Gábor HojtsyI think that is likely due to test system internals. I'd check any other language domain tests already there. How do they do this?
Comment #72
attiks CreditAttribution: attiks commentedI 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.
Comment #73
Dave ReidCorrect 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']));
Comment #74
Gábor Hojtsy@Dave Reid: Very true. Should we also carry over ports to the other domain in language switching, if we are running custom ports? (Possibly).
Comment #75
attiks CreditAttribution: attiks commented@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
Comment #76
attiks CreditAttribution: attiks commentedNew patch with https support
Comment #77
Gábor Hojtsy@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?
Comment #78
Jelle_SNew patch based on #77
Comment #80
Jelle_Sfixed patch with extra test for current url scheme.
Comment #81
Jelle_SComment #82
Jelle_Sforgot to check the 'https' variable
Comment #83
Gábor HojtsyI'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.
Let's call this $langcode, its not about browser fallback, is it? I think this is a result of copy-paste.
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.
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?
Comments should have a dot at end of line and lead with a space after //.
Thanks!
Comment #84
Jelle_SPatch based on #83 ;-)
Comment #85
Gábor HojtsyOk, looks good to me know.
Comment #86
catchLooks good. I've committed/pushed this to 8.x, moving back to 7.x for backport.
Comment #87
Gábor HojtsyThere is also an upgrade path component to be worked on in #1272840: Add upgrade path for language domains and validation, contributions welcome there too!
Comment #88
xjmThe 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)
Comment #89
xjmSimple doc fix for D8. The D7 backport is mostly naïve. I replaced
$domains[$options['language']->langcode]
with$options['language']->domain
in D7, but no other changes, so please review the D7 patch closely. Thanks!Comment #90
xjmOh, and it has tests, so untagging.
Comment #91
Gábor HojtsyNobrainer for D8 (but minor).
Comment #92
Dries CreditAttribution: Dries commentedCommitted to 8.x. Moving to 7.x. Thanks!
Comment #93
Gábor HojtsyNeeds at least the patch posted for the testbot on d7 and some reciew as xjm pointed out.
Comment #94
xjmRenamed for the bot.
Comment #95
xjmOops, and restoring priority.
Comment #97
Gábor HojtsyThere are at least two points where this needs D7 backports:
No langugae_save() in Drupal 7. Can/should use locale_add_language().
Domain is configured on the language (should be done above) in Drupal 7.
@Jelle_S, @attiks: can you help bring this forward?
Comment #98
attiks CreditAttribution: attiks commentedI'll have a look later today
Comment #99
Gábor Hojtsy@attiks: any progress? Thanks!
Comment #100
attiks CreditAttribution: attiks commented@Jelle_S is working on it right now, it works inside devel/php, but not as a test :/
Comment #101
attiks CreditAttribution: attiks commentedFirst try, please review closely
Comment #103
attiks CreditAttribution: attiks commentedStrange: 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
Comment #104
aspilicious CreditAttribution: aspilicious commentedtrailing whitespace
tabs
I'm not sure why the test is failing but I saw these styling issues in dreditor.
Comment #105
attiks CreditAttribution: attiks commentedcode cleaned
Comment #106
attiks CreditAttribution: attiks commentedremoved debug stuff
Comment #108
attiks CreditAttribution: attiks commented@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
Comment #110
Gábor HojtsyRan 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.
Comment #111
attiks CreditAttribution: attiks commentedI fixed the domain inside drupal-6.locale.database, try again bot
Comment #112
Gábor HojtsyWhile 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 :)
Comment #113
attiks CreditAttribution: attiks commented@Gabor: I think we need #1272840: Add upgrade path for language domains and validation for Drupal 7 as well, this will solve this
Comment #114
Gábor Hojtsy@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.
Comment #115
attiks CreditAttribution: attiks commented@Gabor: something like this
Comment #116
attiks CreditAttribution: attiks commentedleading space
20 days to next Drupal core point release.
Comment #117
attiks CreditAttribution: attiks commentedadding test for domains with a prefix right now
Comment #118
Gábor HojtsyI 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 :)
Add a one line comment above this to separate from the parse_url() wrangling IMHO. Something like.
// Apply the appropriate protocol to the URL.
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.
Comment #119
attiks CreditAttribution: attiks commentedNew patch with extra tests
Comment #120
Gábor HojtsyLooks 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?
Comment #121
xjmFrech language. :)
Also, it might be good to use
format_string()
rather thant()
on the assertion message strings. Reference: http://drupal.org/simpletest-tutorial-drupal7#tComment #122
attiks CreditAttribution: attiks commentedwith foreach
Comment #123
attiks CreditAttribution: attiks commentedFrech -> French
About the use of t(), all others are using this as well, so better fix it in a separate issue?
Comment #124
xjmNah, 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!
Comment #125
Gábor HojtsyI think this looks good on a visual review. I've asked a few people to look at it and help double-check.
Comment #126
pixeliteThe patch in #123 seems to work. The correct URLs are generated whether or not you enter the protocol.
Comment #127
Robin Monks CreditAttribution: Robin Monks commentedTested as well; couldn't make it break (which I guess is a good sigh ;) )
+1 for commit.
/Robin
Comment #128
dergachev CreditAttribution: dergachev commentedThis should probably be tested on HTTPS before it's RTBC.
Comment #129
Robin Monks CreditAttribution: Robin Monks commentedI tested on http and https.
/Robin
Comment #130
webchickCommitted and pushed to 7.x. Thanks!
Comment #131
Gábor HojtsyOff-sprint then. Thanks!
Comment #132
attiks CreditAttribution: attiks commentedneeds backport to D7 removed
Comment #134
Gábor HojtsyThis resulted in a pretty unfortunate bug at #1572394: Language detection by domain only works on port 80. Anybody interested in helping solve it?
Comment #134.0
Gábor HojtsyUpdated issue summary.