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.

CommentFileSizeAuthor
#123 i1250800_123.patch4.49 KBattiks
#122 i1250800_121.patch4.49 KBattiks
#119 i1250800_119.patch5.91 KBattiks
#115 i1250800_115.patch4.07 KBattiks
#111 i1250800_111.patch4.37 KBattiks
#110 site_name.png119.36 KBGábor Hojtsy
#108 i1250800_108.patch4 KBattiks
#106 i1250800_106.patch3.82 KBattiks
#105 i1250800_105.patch4.65 KBattiks
#101 i1250800_101.patch4.67 KBattiks
#94 1250800-94.patch4 KBxjm
#89 1250800-d8-followup.patch543 bytesxjm
#89 1250800-d7.patch4 KBxjm
#84 i1250800_84.patch4.13 KBJelle_S
#82 i1250800_82.patch4.13 KBJelle_S
#80 i1250800_80.patch4.09 KBJelle_S
#78 i1250800_78.patch3.51 KBJelle_S
#76 i1250800_76.patch3.39 KBattiks
#72 i1250800_72.patch2.57 KBattiks
#68 i1250800_68.patch2.48 KBattiks
#66 i1250800_66.patch2.42 KBattiks
#61 i1250800_61.patch2.37 KBJelle_S
#59 i1250800_59.patch2.37 KBJelle_S
#58 i1250800_58.patch2.44 KBJelle_S
#52 i1250800_52.patch603 bytesattiks
#38 i1250800_7.patch3.14 KBattiks
#37 i1250800-d7.patch3.14 KBattiks
#32 i1250800.patch3.14 KBattiks
#28 i1250800.patch3.14 KBattiks
#26 i1250800.patch3.15 KBattiks
#24 1250800-locale-protocoll-independent.patch1.65 KBdawehner
#23 i1250800.patch2.99 KBattiks
#22 i1250800.patch1.66 KBattiks
#17 i1250800.patch1.19 KBattiks
#15 i1250800.patch930 bytesattiks
#12 i1250800.patch930 bytesattiks
#9 i1250800.patch708 bytesattiks
#7 i1250800.patch769 bytesattiks
#6 i1250800.patch833 bytesattiks
#4 i1250800.patch869 bytesattiks
#2 1250800-locale-protocoll-independent.patch856 bytesdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Version: 7.7 » 8.x-dev

This needs to be fixed in d8 first and backported

dawehner’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
856 bytes

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.

attiks’s picture

Status: Needs work » Needs review
FileSize
869 bytes

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

dawehner’s picture

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.

attiks’s picture

Status: Needs work » Needs review
FileSize
833 bytes

good catch

attiks’s picture

FileSize
769 bytes

even cleaner patch

Status: Needs review » Needs work

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

attiks’s picture

FileSize
708 bytes

try again

attiks’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

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

attiks’s picture

Status: Needs work » Needs review
FileSize
930 bytes

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

attiks’s picture

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.

attiks’s picture

attiks’s picture

FileSize
930 bytes

first try to adding http/https mixed testing

attiks’s picture

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

attiks’s picture

FileSize
1.19 KB

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.

attiks’s picture

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

webchick’s picture

Issue tags: +Needs backport to D7

Fixing tag.

Gábor Hojtsy’s picture

Title: Language domain - protocol - blocked http and https switching » Langauge 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?

Gábor Hojtsy’s picture

Issue tags: +D8MI

Tagging.

attiks’s picture

Status: Needs work » Needs review
FileSize
1.66 KB

@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

attiks’s picture

dawehner’s picture

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

dawehner’s picture

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

Fix typo

attiks’s picture

FileSize
3.15 KB

rerolled, added description back and added some more comments

dawehner’s picture

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.

attiks’s picture

Status: Needs work » Needs review
FileSize
3.14 KB

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

dawehner’s picture

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.

attiks’s picture

yoroy’s picture

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

attiks’s picture

Status: Needs work » Needs review
FileSize
3.14 KB

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the clarification

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

rfay’s picture

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.

attiks’s picture

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

Dries’s picture

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

Looks good. Committed to 8.x.

attiks’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.14 KB

D7 patch

attiks’s picture

FileSize
3.14 KB

again

attiks’s picture

Status: Needs review » Reviewed & tested by the community

clean backport, no changes, just rerolled

sun’s picture

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.

dawehner’s picture

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.

catch’s picture

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?

attiks’s picture

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

attiks’s picture

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

Gábor Hojtsy’s picture

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.

dawehner’s picture

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.

attiks’s picture

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

catch’s picture

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.

Dries’s picture

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.

Gábor Hojtsy’s picture

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.

attiks’s picture

Status: Active » Needs review
FileSize
603 bytes

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.

Gábor Hojtsy’s picture

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

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

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Issue tags: +sprint

Tagging as current focus.

Gábor Hojtsy’s picture

Issue tags: +language-base

Tagging for base language system.

Jelle_S’s picture

FileSize
2.44 KB

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

Jelle_S’s picture

FileSize
2.37 KB

Fixed a comment typo

attiks’s picture

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.

Jelle_S’s picture

FileSize
2.37 KB

new patch based on #60

Gábor Hojtsy’s picture

Status: Needs work » Needs review

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

Gábor Hojtsy’s picture

Issue tags: +negotiation

Tagging for language negotiation too.

attiks’s picture

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.

attiks’s picture

Status: Needs work » Needs review
FileSize
2.42 KB

patch to see what's going on

Status: Needs review » Needs work

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

attiks’s picture

Status: Needs work » Needs review
FileSize
2.48 KB

very strange ...

Status: Needs review » Needs work

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

Jelle_S’s picture

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

Gábor Hojtsy’s picture

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?

attiks’s picture

Status: Needs work » Needs review
FileSize
2.57 KB

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.

Dave Reid’s picture

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

Gábor Hojtsy’s picture

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

attiks’s picture

@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

attiks’s picture

Status: Needs work » Needs review
FileSize
3.39 KB

New patch with https support

Gábor Hojtsy’s picture

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?

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
3.51 KB

New patch based on #77

Status: Needs review » Needs work

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

Jelle_S’s picture

FileSize
4.09 KB

fixed patch with extra test for current url scheme.

Jelle_S’s picture

Status: Needs work » Needs review
Jelle_S’s picture

FileSize
4.13 KB

forgot to check the 'https' variable

Gábor Hojtsy’s picture

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!

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
4.13 KB

Patch based on #83 ;-)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Ok, looks good to me know.

catch’s picture

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.

Gábor Hojtsy’s picture

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!

xjm’s picture

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)

xjm’s picture

Version: 7.x-dev » 8.x-dev
Assigned: xjm » Unassigned
Status: Patch (to be ported) » Needs review
FileSize
4 KB
543 bytes

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!

xjm’s picture

Issue tags: -Needs tests

Oh, and it has tests, so untagging.

Gábor Hojtsy’s picture

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

Nobrainer for D8 (but minor).

Dries’s picture

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

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

Gábor Hojtsy’s picture

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.

xjm’s picture

FileSize
4 KB

Renamed for the bot.

xjm’s picture

Priority: Minor » Critical

Oops, and restoring priority.

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

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?

attiks’s picture

Assigned: Unassigned » attiks

I'll have a look later today

Gábor Hojtsy’s picture

@attiks: any progress? Thanks!

attiks’s picture

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

attiks’s picture

Status: Needs work » Needs review
FileSize
4.67 KB

First try, please review closely

Status: Needs review » Needs work

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

attiks’s picture

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

aspilicious’s picture

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.

attiks’s picture

Status: Needs work » Needs review
FileSize
4.65 KB

code cleaned

attiks’s picture

FileSize
3.82 KB

removed debug stuff

Status: Needs review » Needs work

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

attiks’s picture

Status: Needs work » Needs review
FileSize
4 KB

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

Gábor Hojtsy’s picture

FileSize
119.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

attiks’s picture

Status: Needs work » Needs review
FileSize
4.37 KB

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

Gábor Hojtsy’s picture

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

attiks’s picture

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

Gábor Hojtsy’s picture

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

attiks’s picture

FileSize
4.07 KB

@Gabor: something like this

attiks’s picture

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.

attiks’s picture

adding test for domains with a prefix right now

Gábor Hojtsy’s picture

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

attiks’s picture

Status: Needs work » Needs review
FileSize
5.91 KB

New patch with extra tests

Gábor Hojtsy’s picture

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?

xjm’s picture

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

attiks’s picture

FileSize
4.49 KB

with foreach

attiks’s picture

FileSize
4.49 KB

Frech -> French

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

xjm’s picture

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!

Gábor Hojtsy’s picture

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

pixelite’s picture

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.

Robin Monks’s picture

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

+1 for commit.

/Robin

dergachev’s picture

Status: Reviewed & tested by the community » Needs review

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

Robin Monks’s picture

Status: Needs review » Reviewed & tested by the community

I tested on http and https.

/Robin

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Off-sprint then. Thanks!

attiks’s picture

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.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.