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.

Files: 
CommentFileSizeAuthor
#123 i1250800_123.patch4.49 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 38,332 pass(es).
[ View ]
#122 i1250800_121.patch4.49 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 38,322 pass(es).
[ View ]
#119 i1250800_119.patch5.91 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 38,326 pass(es).
[ View ]
#115 i1250800_115.patch4.07 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 38,329 pass(es).
[ View ]
#111 i1250800_111.patch4.37 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 38,306 pass(es).
[ View ]
#110 site_name.png119.36 KBGábor Hojtsy
#108 i1250800_108.patch4 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 38,151 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#106 i1250800_106.patch3.82 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 38,099 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#105 i1250800_105.patch4.65 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 38,101 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#101 i1250800_101.patch4.67 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 38,102 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#94 1250800-94.patch4 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 37,897 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#89 1250800-d8-followup.patch543 bytesxjm
PASSED: [[SimpleTest]]: [MySQL] 34,355 pass(es).
[ View ]
#89 1250800-d7.patch4 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 38,146 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#84 i1250800_84.patch4.13 KBJelle_S
PASSED: [[SimpleTest]]: [MySQL] 34,231 pass(es).
[ View ]
#82 i1250800_82.patch4.13 KBJelle_S
PASSED: [[SimpleTest]]: [MySQL] 33,934 pass(es).
[ View ]
#80 i1250800_80.patch4.09 KBJelle_S
PASSED: [[SimpleTest]]: [MySQL] 33,942 pass(es).
[ View ]
#78 i1250800_78.patch3.51 KBJelle_S
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1250800_78.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#76 i1250800_76.patch3.39 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 33,952 pass(es).
[ View ]
#72 i1250800_72.patch2.57 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 33,948 pass(es).
[ View ]
#68 i1250800_68.patch2.48 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 33,373 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#66 i1250800_66.patch2.42 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 33,359 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#61 i1250800_61.patch2.37 KBJelle_S
FAILED: [[SimpleTest]]: [MySQL] 33,374 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#59 i1250800_59.patch2.37 KBJelle_S
FAILED: [[SimpleTest]]: [MySQL] 33,372 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#58 i1250800_58.patch2.44 KBJelle_S
FAILED: [[SimpleTest]]: [MySQL] 33,372 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#52 i1250800_52.patch603 bytesattiks
PASSED: [[SimpleTest]]: [MySQL] 33,348 pass(es).
[ View ]
#38 i1250800_7.patch3.14 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 35,948 pass(es).
[ View ]
#37 i1250800-d7.patch3.14 KBattiks
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1250800-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#32 i1250800.patch3.14 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 32,907 pass(es).
[ View ]
#28 i1250800.patch3.14 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 32,909 pass(es).
[ View ]
#26 i1250800.patch3.15 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 32,922 pass(es).
[ View ]
#24 1250800-locale-protocoll-independent.patch1.65 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 32,906 pass(es).
[ View ]
#23 i1250800.patch2.99 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 32,908 pass(es).
[ View ]
#22 i1250800.patch1.66 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 32,917 pass(es).
[ View ]
#17 i1250800.patch1.19 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 33,224 pass(es).
[ View ]
#15 i1250800.patch930 bytesattiks
PASSED: [[SimpleTest]]: [MySQL] 10 pass(es).
[ View ]
#12 i1250800.patch930 bytesattiks
PASSED: [[SimpleTest]]: [MySQL] 33,215 pass(es).
[ View ]
#9 i1250800.patch708 bytesattiks
PASSED: [[SimpleTest]]: [MySQL] 33,666 pass(es).
[ View ]
#7 i1250800.patch769 bytesattiks
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1250800_1.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#6 i1250800.patch833 bytesattiks
PASSED: [[SimpleTest]]: [MySQL] 33,662 pass(es).
[ View ]
#4 i1250800.patch869 bytesattiks
PASSED: [[SimpleTest]]: [MySQL] 33,677 pass(es).
[ View ]
#2 1250800-locale-protocoll-independent.patch856 bytesdawehner
FAILED: [[SimpleTest]]: [MySQL] 33,663 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Comments

Version:7.7» 8.x-dev

This needs to be fixed in d8 first and backported

Status:Active» Needs review
Issue tags:+Needs tests
StatusFileSize
new856 bytes
FAILED: [[SimpleTest]]: [MySQL] 33,663 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1250800-locale-protocoll-independent.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new869 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,677 pass(es).
[ View ]

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

Status:Needs review» Needs work

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new833 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,662 pass(es).
[ View ]

good catch

StatusFileSize
new769 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1250800_1.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

even cleaner patch

Status:Needs review» Needs work

The last submitted patch, i1250800.patch, failed testing.

StatusFileSize
new708 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,666 pass(es).
[ View ]

try again

Status:Needs work» Needs review

Status:Needs review» Needs work

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 :)

Status:Needs work» Needs review
StatusFileSize
new930 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,215 pass(es).
[ View ]

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

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.

StatusFileSize
new930 bytes
PASSED: [[SimpleTest]]: [MySQL] 10 pass(es).
[ View ]

first try to adding http/https mixed testing

tests will probably fail, the https login works, but all requests after return 403

StatusFileSize
new1.19 KB
PASSED: [[SimpleTest]]: [MySQL] 33,224 pass(es).
[ View ]

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.

I opened a separate issue for the test, see #1261454: Add tests for http/https language domain negotiation

Issue tags:+needs backport to D7

Fixing tag.

Title:Language domain - protocol - blocked http and https switchingLangauge domain should work regardless of ports or protocols
Status:Needs review» Needs work

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?

Issue tags:+D8MI

Tagging.

Status:Needs work» Needs review
StatusFileSize
new1.66 KB
PASSED: [[SimpleTest]]: [MySQL] 32,917 pass(es).
[ View ]

@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

StatusFileSize
new2.99 KB
PASSED: [[SimpleTest]]: [MySQL] 32,908 pass(es).
[ View ]

StatusFileSize
new1.65 KB
PASSED: [[SimpleTest]]: [MySQL] 32,906 pass(es).
[ View ]

+++ 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.

Title:Langauge domain should work regardless of ports or protocolsLanguage domain should work regardless of ports or protocols
Issue tags:-Needs tests

Fix typo

StatusFileSize
new3.15 KB
PASSED: [[SimpleTest]]: [MySQL] 32,922 pass(es).
[ View ]

rerolled, added description back and added some more comments

Status:Needs review» Needs work

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new3.14 KB
PASSED: [[SimpleTest]]: [MySQL] 32,909 pass(es).
[ View ]

spaces are gone.
i'll backport if this is is accepted.

Status:Needs review» Needs work

+++ 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.

Well, to make the description shorter of course. One example is enough to get the point across.

Status:Needs work» Needs review
StatusFileSize
new3.14 KB
PASSED: [[SimpleTest]]: [MySQL] 32,907 pass(es).
[ View ]

new patch, fixed the missing ", and changed URLs to an URL

Status:Needs review» Reviewed & tested by the community

Thanks for the clarification

Patch looks fine now for me, the description is also better now.

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.

Moved my original request to the right queue, #1261522: Can all bots run https test

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

Looks good. Committed to 8.x.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new3.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1250800-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

D7 patch

StatusFileSize
new3.14 KB
PASSED: [[SimpleTest]]: [MySQL] 35,948 pass(es).
[ View ]

again

Status:Needs review» Reviewed & tested by the community

clean backport, no changes, just rerolled

Status:Reviewed & tested by the community» Needs work

+++ 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.

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.

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?

#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.

#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.

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.

Status:Needs work» Needs review

Okay the first concern of sun can't happen in d7, so make the issue to needs review to see what the testbot thinks.

#38: i1250800_7.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

We have a followup for sun's concern for D8, so moving this back to RTBC for 7.x.

Status:Reviewed & tested by the community» Fixed

Committed to 7.x. Marking fixed.

Status:Fixed» Closed (fixed)

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

Version:7.x-dev» 8.x-dev
Priority:Major» Critical
Status:Closed (fixed)» Active

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.

Status:Active» Needs review
StatusFileSize
new603 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,348 pass(es).
[ View ]

Patch for D8 attached, it should fix it

Status:Needs review» Needs work
Issue tags:-needs backport to D7, -D8MI

The last submitted patch, i1250800_52.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+needs backport to D7, +D8MI

#52: i1250800_52.patch queued for re-testing.

Issue tags:+Needs tests

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.

Issue tags:+sprint

Tagging as current focus.

Issue tags:+language-base

Tagging for base language system.

StatusFileSize
new2.44 KB
FAILED: [[SimpleTest]]: [MySQL] 33,372 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Patch for D8, including test for the url() function

StatusFileSize
new2.37 KB
FAILED: [[SimpleTest]]: [MySQL] 33,372 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Fixed a comment typo

Status:Needs review» Needs work

+++ 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.

StatusFileSize
new2.37 KB
FAILED: [[SimpleTest]]: [MySQL] 33,374 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

new patch based on #60

Status:Needs work» Needs review

Marking needs review for the test. Looks like it does not actually pass?

Issue tags:+negotiation

Tagging for language negotiation too.

I ran the tests as well and they pass, let's wait for the bot

Status:Needs review» Needs work

The last submitted patch, i1250800_61.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.42 KB
FAILED: [[SimpleTest]]: [MySQL] 33,359 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

patch to see what's going on

Status:Needs review» Needs work

The last submitted patch, i1250800_66.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.48 KB
FAILED: [[SimpleTest]]: [MySQL] 33,373 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

very strange ...

Status:Needs review» Needs work

The last submitted patch, i1250800_68.patch, failed testing.

anyone an idea why base_path() returns '/checkout/' in the testbot environment but the url() function does not append it to the url?

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?

Status:Needs work» Needs review
StatusFileSize
new2.57 KB
PASSED: [[SimpleTest]]: [MySQL] 33,948 pass(es).
[ View ]

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.

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']));

Status:Needs review» Needs work

@Dave Reid: Very true. Should we also carry over ports to the other domain in language switching, if we are running custom ports? (Possibly).

@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

Status:Needs work» Needs review
StatusFileSize
new3.39 KB
PASSED: [[SimpleTest]]: [MySQL] 33,952 pass(es).
[ View ]

New patch with https support

Status:Needs review» Needs work

@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?

Status:Needs work» Needs review
StatusFileSize
new3.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1250800_78.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

New patch based on #77

Status:Needs review» Needs work

The last submitted patch, i1250800_78.patch, failed testing.

StatusFileSize
new4.09 KB
PASSED: [[SimpleTest]]: [MySQL] 33,942 pass(es).
[ View ]

fixed patch with extra test for current url scheme.

Status:Needs work» Needs review

StatusFileSize
new4.13 KB
PASSED: [[SimpleTest]]: [MySQL] 33,934 pass(es).
[ View ]

forgot to check the 'https' variable

Status:Needs review» Needs work

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!

Status:Needs work» Needs review
StatusFileSize
new4.13 KB
PASSED: [[SimpleTest]]: [MySQL] 34,231 pass(es).
[ View ]

Patch based on #83 ;-)

Status:Needs review» Reviewed & tested by the community

Ok, looks good to me know.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Looks good. I've committed/pushed this to 8.x, moving back to 7.x for backport.

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!

Assigned:Unassigned» xjm

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)

Version:7.x-dev» 8.x-dev
Assigned:xjm» Unassigned
Status:Patch (to be ported)» Needs review
StatusFileSize
new4 KB
FAILED: [[SimpleTest]]: [MySQL] 38,146 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new543 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,355 pass(es).
[ View ]

Simple 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!

Issue tags:-Needs tests

Oh, and it has tests, so untagging.

Priority:Critical» Minor
Status:Needs review» Reviewed & tested by the community

Nobrainer for D8 (but minor).

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

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

Status:Reviewed & tested by the community» Needs review

Needs at least the patch posted for the testbot on d7 and some reciew as xjm pointed out.

StatusFileSize
new4 KB
FAILED: [[SimpleTest]]: [MySQL] 37,897 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Renamed for the bot.

Priority:Minor» Critical

Oops, and restoring priority.

Status:Needs review» Needs work

The last submitted patch, 1250800-94.patch, failed testing.

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?

Assigned:Unassigned» attiks

I'll have a look later today

@attiks: any progress? Thanks!

@Jelle_S is working on it right now, it works inside devel/php, but not as a test :/

Status:Needs work» Needs review
StatusFileSize
new4.67 KB
FAILED: [[SimpleTest]]: [MySQL] 38,102 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

First try, please review closely

Status:Needs review» Needs work

The last submitted patch, i1250800_101.patch, failed testing.

Status:Needs work» Needs review

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

Status:Needs review» Needs work

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new4.65 KB
FAILED: [[SimpleTest]]: [MySQL] 38,101 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

code cleaned

StatusFileSize
new3.82 KB
FAILED: [[SimpleTest]]: [MySQL] 38,099 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

removed debug stuff

Status:Needs review» Needs work

The last submitted patch, i1250800_106.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4 KB
FAILED: [[SimpleTest]]: [MySQL] 38,151 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

@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

Status:Needs review» Needs work

The last submitted patch, i1250800_108.patch, failed testing.

StatusFileSize
new119.36 KB

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.

site_name.png

Status:Needs work» Needs review
StatusFileSize
new4.37 KB
PASSED: [[SimpleTest]]: [MySQL] 38,306 pass(es).
[ View ]

I fixed the domain inside drupal-6.locale.database, try again bot

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 :)

@Gabor: I think we need #1272840: Add upgrade path for language domains and validation for Drupal 7 as well, this will solve this

@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.

StatusFileSize
new4.07 KB
PASSED: [[SimpleTest]]: [MySQL] 38,329 pass(es).
[ View ]

@Gabor: something like this

Status:Needs review» Needs work

+++ 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.

adding test for domains with a prefix right now

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new5.91 KB
PASSED: [[SimpleTest]]: [MySQL] 38,326 pass(es).
[ View ]

New patch with extra tests

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?

+++ 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 than t() on the assertion message strings. Reference: http://drupal.org/simpletest-tutorial-drupal7#t

StatusFileSize
new4.49 KB
PASSED: [[SimpleTest]]: [MySQL] 38,322 pass(es).
[ View ]

with foreach

StatusFileSize
new4.49 KB
PASSED: [[SimpleTest]]: [MySQL] 38,332 pass(es).
[ View ]

Frech -> French

About the use of t(), all others are using this as well, so better fix it in a separate issue?

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!

I think this looks good on a visual review. I've asked a few people to look at it and help double-check.

Status:Needs review» Reviewed & tested by the community

The patch in #123 seems to work. The correct URLs are generated whether or not you enter the protocol.

Tested as well; couldn't make it break (which I guess is a good sigh ;) )

+1 for commit.

/Robin

Status:Reviewed & tested by the community» Needs review

This should probably be tested on HTTPS before it's RTBC.

Status:Needs review» Reviewed & tested by the community

I tested on http and https.

/Robin

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 7.x. Thanks!

Issue tags:-sprint

Off-sprint then. Thanks!

Assigned:attiks» Unassigned
Issue tags:-needs backport to D7

needs backport to D7 removed

Status:Fixed» Closed (fixed)

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

This resulted in a pretty unfortunate bug at #1572394: Language detection by domain only works on port 80. Anybody interested in helping solve it?

Issue summary:View changes

Updated issue summary.