After some testing on the Locale upgrade path I realized the language negotiation settings are upgraded correctly but the new configuration variables locale_language_providers_enabled_language and locale_language_providers_weight_language are not set. This causes the language detection and selection page to display no language detection method enabled although the language negotiation configuration is actually set.

I also spotted a possible bug in the upgrade path if the old language negotiation is set to "Path prefix only" and the default language is given a path prefix. In D6 this prefix is ignored while in D7 it is taken into account and thus site URLs may break.

Finally the language switcher block's delta has changed but locale_update_7001() does not take care of this either.

CommentFileSizeAuthor
#29 812416-locale-upgrade-path.patch13.77 KBDamien Tournoud
#28 812416-locale-upgrade-path.patch9.13 KBwebchick
#25 812416-locale-upgrade-path.patch13.85 KBDamien Tournoud
#21 812416-locale-upgrade-path.patch11.8 KBDamien Tournoud
#18 812416-locale-upgrade-path.patch11.65 KBDamien Tournoud
#16 locale-812416-16.patch2.5 KBplach
#7 locale-812416-7.patch1.31 KBplach
#1 locale_upgrade_path-812416-1.patch3.96 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Active » Needs review
FileSize
locale_upgrade_path-812416-1.patch3.96 KB

The attached patch should fix the issue by

  • removing the locale_language_providers_enabled_language variable, which is actually pretty redundant, and assigning locale_language_providers_weight_language the right value,
  • ensuring that the path prefix for the default language is always empty when the old language negotiation is set to "Path prefix only",
  • renaming the language switcher block's delta in the block table to match the new one.
Dries’s picture

Dries CreditAttribution: Dries commented

I haven't tested this but all these changes are valid changes and omissions from the upgrade path. Would be great if someone could test the patch as it is in the critical path to beta-1.

roborn’s picture

roborn CreditAttribution: roborn commented
Status: Needs review » Reviewed & tested by the community

I've tested an upgrade from D6-1.6 PT-pt to D7.x-dev with patch #1 applied, and I can confirm that #1 fixes the issue.
I'm marking this RTBC.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Alright, committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Closed (fixed) » Needs review
FileSize
locale-812416-7.patch1.31 KB

LOCALE_LANGUAGE_NEGOTIATION_URL_DOMAIN definition was moved to locale.inc but the update function was not updated accordingly. Moreover I discovered a bug with case 1: the language prefix was not reset in the system variable so URLs were broken anyway.

Unfortunately this is the first time I am actually able to test the upgrade path. *Now* it seems to work well.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: +beta blocker

If I understood rules well, upgrade path issues block beta release.

Jody Lynn’s picture

Jody Lynn
she/her
Primary language English
CreditAttribution: Jody Lynn commented

The patch looks reasonable, but I think we need a test for this in simpletest's upgrade tests as well.

Status: Needs review » Needs work
Issue tags: -D7 upgrade path, -beta blocker

The last submitted patch, locale-812416-7.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
Issue tags: +D7 upgrade path, +beta blocker

Weird failure.

#7: locale-812416-7.patch queued for re-testing.

int’s picture

int CreditAttribution: int commented
Status: Needs review » Reviewed & tested by the community

it seems good

Jody Lynn’s picture

Jody Lynn
she/her
Primary language English
CreditAttribution: Jody Lynn commented
Status: Reviewed & tested by the community » Needs review

What did you test in your review?

ksenzee’s picture

ksenzee
she/her
Location Seattle area
CreditAttribution: ksenzee commented

We do need some simpletests here. I am definitely not a language system expert but I will see what I can come up with.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Issue tags: +Needs tests

Tagging.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
locale-812416-16.patch2.5 KB

I performed some additional testing (there are 4 configuration modes to test) and found that the language switcher block update was failing due to system_update_7004. Rerolled the patch to exploit it to update the language switcher blocks delta and removed the related code from locale_update_7001.

Now the upgrade path work as expected :)

If Katherine wishes to provide some tests I'll be glad to review them, otherwise I'll try to write them myself.

ksenzee’s picture

ksenzee
she/her
Location Seattle area
CreditAttribution: ksenzee commented

I won't be able to get the tests written in the next 16 hours or so, so if anyone else has time please feel free. Otherwise I'll pick it back up then.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
FileSize
812416-locale-upgrade-path.patch11.65 KB

Here is a first shot at a test, but it doesn't pass :(

It seems that language negotiation doesn't kick in.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

Giving a look.

Status: Needs review » Needs work

The last submitted patch, 812416-locale-upgrade-path.patch, failed testing.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs work » Needs review
FileSize
812416-locale-upgrade-path.patch11.8 KB

@plach found out that the tests were failing because of the missing language_count variable.

Also when language negotiation is not active, the prefixes still are due to a related bug #780318: Path prefixes are always active on multilingual sites. I commented out this test for now.

babbage’s picture

babbage CreditAttribution: babbage commented

Fix for ugly spam changes to issue tags.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
FileSize
812416-locale-upgrade-path.patch13.85 KB

Completed the tests with a test for domain negotiation and additional tests for the language switcher block.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Issue tags: -Needs tests
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

These new tests look great. We have one commented, but it is the simplest case, so let's not hold up beta for it. Upgraded the related issue to critical.

+++ modules/simpletest/tests/upgrade/upgrade.locale.test
@@ -0,0 +1,145 @@
+    // No prefixed page should exist.
+    // @todo: this doesn't pass, see http://drupal.org/node/780318
+    // $this->drupalGet('en');
+    // $this->assertResponse(404);
+    // $this->drupalGet('fr');
+    // $this->assertResponse(404);

For consistency here we should test that the language switcher is not shown, but we can fix this after fixing the other issue.

Powered by Dreditor.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
FileSize
812416-locale-upgrade-path.patch9.13 KB

I committed #780318: Path prefixes are always active on multilingual sites so here's a re-roll with that hunk uncommented and the @todo removed.

Let's see if testbot likes it still.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
FileSize
812416-locale-upgrade-path.patch13.77 KB

Here is a complete patch.

Tested locally: the whole Locale test group and the whole Upgrade path test group pass.

int’s picture

int CreditAttribution: int commented

So is all green!

@webchick, One question, if this is the last beta blocker, when you will release the D7 beta1?
* Immediately?
* Some day soon?
* Next month?

It's probably that still are undetected bugs in the upgrade path, but i suppose that we will fix the upgrade path until drupal 7 final or also after.

If we detect one bug in the upgrade path we support the upgrade from D7 Beta 1 (if is none data were lost), correct?

steinmb’s picture

steinmb CreditAttribution: steinmb commented

YAY! Nice work :) +1 to Some say soon.

@int I have been looking through older critical issues that are tagged with won't fix but still are causing problems with the D6->D7 upgrade and found this one posted by you: #864020: Locale upgrade to D7 fails due to index added by i18nstrings module that is stopping locale.module update #7000 and #7001 (see last comment in the issue by me). The i18n project have not fixed/done anything at least nothing I'm able to find in CVS. Cross posted to i18n #931534: i18n causing Drupal 7 upgrade problems but still no reply.

int’s picture

int CreditAttribution: int commented

I know the i18n problem, and don't have nothing to fix here in drupal core.. the fix have to be in the module.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

I committed this to HEAD.

However, while we were testing DamZ's Drupal 7 d.o mirror at http://drupalorg.staging.dev.damz.org/ , we noticed that node bodies are missing on Project nodes. :( So I had to re-open #926794: revisions of unpublished nodes lose their body field on upgrade so we're not quite there yet...

But my hope is that beta could be as early as this evening. Especially since I have actual "work" work to get done at some point this week. ;)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)
Issue tags: -D7 upgrade path, -beta blocker

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

Issue tags: +D7 upgrade path, +beta blocker

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.