Add upgrade path for language domains and validation

Tagging.

Add parent

Would be great if you could work on this! Thanks for your contributions!

Interesting, I'd have used a regex to ensure no slashes, only letters, numbers and dots are in the setting. That would allow domains and IP addresses alike, at least IPv4. It becomes complicated with IPv6. Does this solution deal well with that? http://en.wikipedia.org/wiki/IPv6_address#Literal_IPv6_addresses_in_netw...

Agreed, automated tests would be great / important.

@attiks: cross-post. In the update function, you can warn them that the domains are now identical and they need to go and configure it differently.

@attiks: your new patch does not include the submission change? Also, update functions are still needed as discussed above.

Looking good. Some comments:

1. I'm not sure we can ignore ipv6 support. By the time Drupal 8 is released and as long as it is in production, ipv6 should be pretty commonplace. Granted this is supposed to be a domain name, not an IP address, but I think people would use IPs here for testing, no?
2. I'd not include a message if the domain was just updated to do the same thing. If there were any in error though, we should have a link to the config page to fix it (at the end, not as part of every single error message :).

If there were any in error though, we should have a link to the config page to fix it (at the end, not as part of every single error message).

@attiks: right, you are right, I missed that. Looking at the message then, t() is not used correctly there. We usually include the full a tags for links so we can include the full text for the link. However, we avoid using t() in update functions since the underlying database might change, and in D8 especially, it sounds unlikely that t() will work in the update function, there are just way too many changes going on underneath. So I'd drop the t().

Fix title now that function is moved. I think you'll at least need to re-roll the patch for the /core/* move (#22336: Move all core Drupal files under a /core folder to improve usability and upgrades).

It is best to avoid calling API functions like language_list() in an update function. Other update functions, like the one above, you can see use direct DB queries to do changes to the db. That ensures we control the whole code running and accidental hooks are not fired which expect updated schemas, etc. If the $languages[1] list is empty there, then your patch does not really do any update, so yeah, that sounds like a sizable issue :)

You need also to rename locale_update_8001() to locale_update_8002() or put it in the already existed update locale_update_8001(). As now after applying your patch your update 8001 will be duplicated with another one.

Lets introduce a new update function for this IMHO.

Perfect, thanks.

@attiks: can you provide an updated patch?

Tagging for base language system.

Tagging for language negotiation too.

#1250800: Language domain should work regardless of ports or protocols is now comitted to D8, so lets work on the upgrade path. Can you re-roll the patch? Also will need upgrade path testing after that. I can help with that.

I rerolled the patch and added a test for the upgrading method.

+++ b/core/modules/locale/locale.admin.incundefined
@@ -330,6 +330,17 @@ function language_negotiation_configure_url_form_validate($form, &$form_state) {
+  // Domain names can not contain protocol and/or ports

- can not => should not.
- end sentence with a dot.

+++ b/core/modules/locale/locale.admin.incundefined
@@ -330,6 +330,17 @@ function language_negotiation_configure_url_form_validate($form, &$form_state) {
+      $host = 'http://' . str_replace(array('http://', 'https://'), '', $value);
+      if (parse_url($host, PHP_URL_HOST) != $value) {

Maybe add a comment here that we check if the entered text is exactly a hostname (or change the above comment to that), given that we discussed this quite a bit.

+++ b/core/modules/locale/locale.installundefined
@@ -207,6 +207,40 @@ function locale_schema() {
+ * Convert domains to new format.

"Converts language domains to new format."

+++ b/core/modules/locale/locale.installundefined
@@ -207,6 +207,40 @@ function locale_schema() {
+  if (module_exists('language')) {

That should already be true since it is a requirement for locale, no?

+++ b/core/modules/locale/locale.installundefined
@@ -207,6 +207,40 @@ function locale_schema() {
+    // $used_domains keeps track of the domain names in use
...
+      // Domain names can not contain protocol and/or ports

Dots at end of line.

+++ b/core/modules/locale/locale.installundefined
@@ -207,6 +207,40 @@ function locale_schema() {
+            $message = 'Some languages are using the same domain name, you should change these domain names at ' . l('URL language detection configuration', 'admin/config/regional/language/configure/url');
+          }

Add a dot at end of sentence.

+++ b/core/modules/simpletest/tests/upgrade/upgrade.language.testundefined
@@ -47,4 +47,16 @@ class LanguageUpgradePathTestCase extends UpgradePathTestCase {
+   * Test url upgrade.

"Tests language domain upgrade path."

+++ b/core/modules/simpletest/tests/upgrade/upgrade.language.testundefined
@@ -47,4 +47,16 @@ class LanguageUpgradePathTestCase extends UpgradePathTestCase {
+    $this->assertTrue($this->performUpgrade(), t('The upgrade was completed successfully.'));

I'd add a newline before and after this one, since its the big dramatic piece in the function :)

+++ b/core/modules/simpletest/tests/upgrade/upgrade.language.testundefined
@@ -47,4 +47,16 @@ class LanguageUpgradePathTestCase extends UpgradePathTestCase {
+    $this->assertTrue($domains['ca'] == $language_domain, t('Url corrected well?'));

"Language domain for Catalan properly upgraded."

Thank's for review. I corrected the patch. Please review again.

+++ b/core/modules/locale/locale.admin.inc
@@ -330,6 +330,18 @@ function language_negotiation_configure_url_form_validate($form, &$form_state) {
+      // Ensure we have a protocol but only one protocol in the setting for parse_url() checking against the hostname.

Wrap at 80 chars. Otherwise looks good.

Thank's, I corrected it.

+++ b/core/modules/locale/locale.testundefined
@@ -2233,8 +2233,8 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
+    // URL.
+    $edit = array("domain[$langcode]" => $language_domain);
     $this->drupalPost("admin/config/regional/language/detection/url", $edit, t('Save configuration'));

What are the double quotes arround the array key good for? :) Also, can we use single quotes for the URL? I always prefer single quotes :)

Thank's,

Is this what you like?

My fault. I read wrong (didn't see the dollar sign) :/. The array key for the edit in the test was fine. Sorry!

Looks like then #44 should be RTBC. Uploading here again for making sure the right one is committed. It is not my patch, not changed from #44.

This looks fine. Committed/pushed to 8.x.

Yay, thanks! Removing outdated tags.

Broke head. locale_update_8001 already existed. Also, 8001 was put before 8000, hah. Attached patch moves the update function to after 8001 and renames to 8002.

Whoops committed the follow-up.

After an update to D8 I had the following error when running update.php:

Fatal error: Call to undefined function locale_language_negotiation_url_domains() in /home/dave/Dropbox/Projects/drupal8dev/core/modules/locale/locale.install on line 294

Note I did not have locale.module enabled on my D8 local install. I suspect that may be the issue. Simple fix to add a call to include includes/locale.inc but we need to catch these types of things in testing.

This allowed the update function to run in my case successfully.

Why the hell did the upgrade path tests not catch this..??

Recategorizing. We think the upgrade tests start from having the module enabled, so we probably need to expand the test coverage to account for the other case?

So the issue is pretty clear; calling an API function from within foo.install, and as I have just now learned we run upgrades whether a module is disabled or not.

So sounds like upgrade path coverage against the bare dump is missing? Or, wait, we need an upgrade path test for a filled dump with all the modules disabled.

I have filed #1460764: Missing test coverage for a D7 install with non-required modules installed but disabled as a critical task for D8 since I'm not sure what other bugs we're going to expose with disabled module update functions and this task is non-trivial as a follow-up in this issue.

davereid filed #1460764: Missing test coverage for a D7 install with non-required modules installed but disabled. As far as I can figure adding test coverage for the upgrade path for previously installed (a.k.a. disabled) modules is a pretty big task, so IMO we should commit this as-is to fix the upgrade path, and then work on making sure it doesn't happen in the future.

Great, thanks for the follow-up.

Committed and pushed to 8.x. Thanks for the fast turnaround!

Reverted status.

Thanks all.

Sorry I must have been asleep when I committed this. Crud api functions like this can't be used in updates anyway, so this needs fixing to add _update_foo versions.

Do those go in the .install?

They do. I'm not at pc at the moment but I'll likely roll the already committed patches back later so we can get this out of the criticals queue and add proper test coverage before it goes in again.

OK I've reverted three commits from this issue.

Here's the resultant patch for reference.

So back to NW on #49 then.

Ok, well, http://api.drupal.org/api/drupal/core%21includes%21locale.inc/function/l... is just a simple variable_get() wrapper. We could just as well replace all its occurances with variable_get() if that is better. Having two separately named wrappers for the same very simple thing is absolutely overkill.

Save function, same thing, just a variable_set(): http://api.drupal.org/api/drupal/core%21includes%21locale.inc/function/l...

Comments?

There's already #1348162: Add update_variable_*() open for a proper upgrade variable_get() wrapper, so if we fixed that issue we could use that here.

I've checked that 2/3rds of the update functions in node.install, locale.install and system.install all use variable_get/set, so sounds like using it here would not hurt anybody. #1348162: Add update_variable_*() can patch this one too when it is ready.

Using variable_get/set directly in the update function. Decided not to ruffle feathers and go in and change all places where the wrappers were used. I'm happy to do if that sounds like it would clear this up better. Feedback welcome.

I'd prefer it if we did #1348162: Add update_variable_*() first before adding any more update functions that we have to go back and fix later. Should we mark this postponed?

Ok, well, nobody seems to be working on that for the past three months. I don't see why we'd want to postpone landing this (and as a consequence moving language negotiation to language module and further cleanup of locale module, for which we are tirelessly working towards with issues like #1222106: Unify language negotiation APIs, declutter terminology too). Unlike #1348162: Add update_variable_*(), this is part of a stream of issues which constantly step on each other's toes to get in so they can form the puzzle pieces of the bigger overall goal.

I actually agree with Gábor here. This feels akin to holding up sensible block system improvements because something in WSCCI vaguely touches it, and forcing people to work on stuff outside of their immediate area.

variable_get/set()s in update functions already pre-date this patch. It's not making the situation any worse, other than it's one more find/replace to do when #1348162: Add update_variable_*() is completed. That issue is a critical task, so it'll definitely get done before D8 ships.

No it's about fixing the upgrade path now, not in a year's time.

Ok, well I fixed the code as per recommendation to not use API functions (outside of variable_get/set used in ~2/3rds of existing upgrade functions in core). I consider this back to RTBC. If you think the other improvement needs to be in first, please mark this postponed. I'll make sure to mark other issues postponed as a consequence, like our other RTBC issue at #1222106: Unify language negotiation APIs, declutter terminology.

Also in terms of head-to-head upgrade path, this one had locale_update_8002() in Drupal core before. Now its backed out, so other changes might get locale_update_8002() (like #1222106: Unify language negotiation APIs, declutter terminology and #746240: Race condition in locale() - duplicates in {locales_source}, which now compete for that number too) which would break head-to-head updates. So in practice, postponing this one postpones any language patches that would need an update function in locale.install added. (Unless if we skip this update function number for the future, in which case maybe #746240: Race condition in locale() - duplicates in {locales_source} can land since it only uses DB calls - although the DB system methods also don't have any update version counterparts).

The head-to-head upgrade path issue is only talking about unstable-n to unstable-n at the moment, so anything that happens in between those we'd not need to cover.

Ok, so since we don't have an unstable release yet, then its a free to take, so I'll reroll #746240: Race condition in locale() - duplicates in {locales_source} to compete for 8002 as well, and then whichever lands first will win that number despite this patch using it originally and the other two will need rerolls. At least my understanding of the guidelines then.

Tag again for sprint.

Update function 8002 is now taken. Rerolled with 8003.

#81: 1272840-81.patch queued for re-testing.

Asking for a re-test as I don't think this patch applies anymore.

Quick simple reroll. Should still be RTBC if it passes tests. (It will be passed backed to needs work otherwise anyway).

Funny, this patch fails on a tiny notice introduced by #1454538: Add langcode property to all entity types; for the user entity, distinguish entity language from user's language preference. Basically the $user->preferred_langcode might not be set yet. This looks pretty simple to fix, so we can IMHO fix it here instead of reopening that issue.

@attiks: heh, its been a while since you last touched it, right? Thanks for the review. The change was minimal so back to RTBC then :)

It applies now and still looks good. Committed to 8.x.

Superb, thanks everyone!