Found this issue while working on #1519942: Decouple locale module from user module, but since this one is independently backportable, and that one is blocked on another patch landing as well, I decided to break this out to its own issue.

The problem in short is that OpenID assembles a list of more specific to least specific languages to match from the local languages based on the language preference information it got, but then iterates through all of them (not stopping at the first one matched), so it ends up overwriting the most specific match it found with the least specific at the end. This is apparent with the test update included. The fix is very simple, break after the first match, which should be the most specific.

The patch includes making the OpenID tests more language aware, instead of just matching from en-GB with a site that is only configured to have 'en' enabled. Also attached a test-only patch to fail.

Needs backport to 7.x too.

CommentFileSizeAuthor
#5 interdiff.txt2.15 KBgábor hojtsy
#5 openid-most-specific-language-match-5.patch7 KBgábor hojtsy
#5 openid-most-specific-language-match-5-test-only-to-FAIL.patch5.15 KBgábor hojtsy
#1 openid-most-specific-language-match-1.patch6.58 KBgábor hojtsy
openid-most-specific-language-match-test-only-to-FAIL.patch4.74 KBgábor hojtsy
openid-most-specific-language-match.patch6.58 KBgábor hojtsy

Comments

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new openid-most-specific-language-match-1.patch6.58 KB

Little grammar fix in code comment in test. Not uploading failing test version, since that should not be affected.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

(I haven't applied the patch)

+++ b/core/modules/openid/openid.module
@@ -255,21 +255,24 @@ function openid_form_user_register_form_alter(&$form, &$form_state) {
-      $candidate_languages[] = $parts[0];
+      $candidate_langcodes[] = $parts[0];

You changed language to langcode ... nice!

+++ b/core/modules/openid/openid.test
@@ -513,10 +519,16 @@ class OpenIDRegistrationTestCase extends OpenIDWebTestCase {
-      'openid.sreg.language' => 'en-GB',
+      'openid.sreg.language' => 'pt-BR',

Why changed from English to Portuguese? The reason why is not 'documented'.

I've kept status on needs review as the patch looks good.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Well, the reason for changing from en-GB always resolving to 'en' or an unknown language resolving to '' (empty string) is that #1519942: Decouple locale module from user module wants to change the user defaults (just like we changed node defaults before) to save in the site default language by default. So there is going to be no differentiation of users 'en' because of 'en-GB' in these tests and those because that was the default. So it made sense to have other configured languages being tested. The pt variants are good because they are built-in in the predefined language list (easy to add) and exist in three variants, a more generic pt and more specific pt-pt and pt-br that we can test for.

Not sure what of this should be documented in the tests, I've attempted to explain a bit with each language_save().

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

I would say add #3 to the tests.

It makes sense to me now. So probably to others too when they read the tests :)

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new openid-most-specific-language-match-5-test-only-to-FAIL.patch5.15 KB
new openid-most-specific-language-match-5.patch7 KB
new interdiff.txt2.15 KB

Ok, added more comments :) Also a little tweak to have pt and pt-pt when testing for pt-br to prove pt-pt would not be picked :) Uploading test only patch too (to fail), since tests changed a tiny bit.

Status: Needs review » Needs work

The last submitted patch, openid-most-specific-language-match-5-test-only-to-FAIL.patch, failed testing.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Needs review

The FAIL.patch was expected to fail (test only), so back to needs review. Looks good enough now? :)

balintbrews’s picture

balintbrews
he/him/his
Primary language Hungarian
Location Netherlands 🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

We have discussed the details of this issue with Gábor on IRC, then I reviewed the patch, what I found correct, and works well in my opinion.

robloach’s picture

robloach
he/him
commented

OpenID fixes?! Yes please!

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D7

Makes sense, thanks for the test additions. Committed/pushed to 8.x, moving to 7.x for backport.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: -sprint

Multiple people I've asked to help backport this to Drupal 7 did not really follow through and I don't have resources to do this myself, so removing the sprint tag. All it would take to backport is:

- don't do the rename to langcode as its D8 only
- DO add the break with the comment :)
- for the tests, depend on locale module for languages instead of language module and use http://api.drupal.org/api/drupal/includes%21locale.inc/function/locale_a... instead of language_save() (the later is nonexistent in D7)

That should be it.

rdickert’s picture

rdickert commented
Assigned: Unassigned » rdickert

Taking issue for backport.

rdickert’s picture

rdickert commented
Version: 7.x-dev » 8.x-dev
Assigned: rdickert » Unassigned
Status: Patch (to be ported) » Fixed

As confirmed by Gábor Hojtsy, this issue applied to a new feature in d8 that was not backported to d7; therefore, no need to backport the bug fix. Marking as fixed.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

The mentioned issue was #771540: Fetch timezone and language using OpenID SREG/AX, not backported to Drupal 7. I'll post there to be aware of this issue if it ever gets backported.

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