Safari selects wrong language with two common roots #234

Closed
dboune opened this Issue Sep 28, 2016 · 8 comments

Comments

2 participants
@dboune

dboune commented Sep 28, 2016

Version Information

  • WordPress: 4.6.1
  • MultilingualPress: 2.4.7

Steps to Reproduce

  1. Base site with language English
  2. Additional site with language en-GB
  3. Ensure priority for both languages to the same value (I'm using 2)
  4. Only redirect from main site, not from other language
  5. Browse to main site from chrome, firefox, and safari

What I Expected

With system/browser language set to US english, no redirect occurs. This is the case in Chrome and in Firefox.

What Happened Instead

In Safari only, redirect occurs to en-GB despite system language set to US English.

*Note that not setting equal priority will cause the higher priority site to always load in any browser, regardless of system language.

Overall expectation

The larger desired behavior here is that if a user comes to the site with US English, no redirection occurs. If they come to the site with en-GB, they are redirected to the en-GB site.

Browser Information (all current versions on OS X)

Here are the Accept-Language header values for various browsers with a US English configuration

Safari
[en-us] => 1

Firefox
[en-US] => 1
[en] => 0.5

Chrome
[en-US] => 1
[en] => 0.8

@tfrommen

This comment has been minimized.

Show comment
Hide comment
@tfrommen

tfrommen Sep 29, 2016

Contributor

Hi, and thanks for this (detailed) issue.

The problem is that Safari seems to be sending lowercase values in the Accept-Language header. MultilingualPress, however, is matching against HTTP codes with the language part being lowercase, while the region part is uppercase.

I did a quick search and learned that a lowercase value seems to be perfectly valid.

MultilingualPress is trying to match en-us, case-sensitively, against en-US—and fails. There are, however, two MultilingualPress languages with the same language part like the Accept-Language header value (i.e., en). These are ordered by priority (and then alphabetically). Since they both have the same priority, en-GB goes before en-US). Result: since redirection is enabled on the US English site, the user gets redirected to the GB language version.

If you are a developer, you could try the following changes to inc/redirect/Mlp_Language_Negotiation.php:

  • in Mlp_Language_Negotiation::parse_accept_header(), add the following line as first line inside the foreach loop (in line 175):

$name = strtolower( $name );,

so you have the following:

foreach ( $fields as $name => $priority ) {

    // The following line is the new one.
    $name = strtolower( $name );

    $out[ $name ] = $priority;

    // ...
}
  • in Mlp_Language_Negotiation::get_user_priority(), set both language codes to lowercase as well, so you get the following:
$lang_http = $language->get_name( 'http_name' );

// The following line is the first of the new ones.
$lang_http = strtolower( $lang_http );

// ...

$lang_short = $language->get_name( 'language_short' );

// The following line is the second and last of the new ones.
$lang_short = strtolower( $lang_short );

This should do the trick. If it really does, it will soon get shipped in a new release.

Thanks,
Thorsten

Contributor

tfrommen commented Sep 29, 2016

Hi, and thanks for this (detailed) issue.

The problem is that Safari seems to be sending lowercase values in the Accept-Language header. MultilingualPress, however, is matching against HTTP codes with the language part being lowercase, while the region part is uppercase.

I did a quick search and learned that a lowercase value seems to be perfectly valid.

MultilingualPress is trying to match en-us, case-sensitively, against en-US—and fails. There are, however, two MultilingualPress languages with the same language part like the Accept-Language header value (i.e., en). These are ordered by priority (and then alphabetically). Since they both have the same priority, en-GB goes before en-US). Result: since redirection is enabled on the US English site, the user gets redirected to the GB language version.

If you are a developer, you could try the following changes to inc/redirect/Mlp_Language_Negotiation.php:

  • in Mlp_Language_Negotiation::parse_accept_header(), add the following line as first line inside the foreach loop (in line 175):

$name = strtolower( $name );,

so you have the following:

foreach ( $fields as $name => $priority ) {

    // The following line is the new one.
    $name = strtolower( $name );

    $out[ $name ] = $priority;

    // ...
}
  • in Mlp_Language_Negotiation::get_user_priority(), set both language codes to lowercase as well, so you get the following:
$lang_http = $language->get_name( 'http_name' );

// The following line is the first of the new ones.
$lang_http = strtolower( $lang_http );

// ...

$lang_short = $language->get_name( 'language_short' );

// The following line is the second and last of the new ones.
$lang_short = strtolower( $lang_short );

This should do the trick. If it really does, it will soon get shipped in a new release.

Thanks,
Thorsten

@dboune

This comment has been minimized.

Show comment
Hide comment
@dboune

dboune Sep 29, 2016

Hi Thorsten,

Thanks for the code. I've made the necessary modifications, but unfortunately that doesn't seem to do it. in Mlp_Language_Negotiation::get_redirect_match the possible matches are sorted by priority. Here are the results. If we pop an entry off, then we end up with the GB site, even though our browser language is set to en-us. I can change the priority, but then I can't get a redirect to en-GB if the browser (rather the whole system in the case of safari) is set to use that language in any browser at all.

Array
(
    [1] => Array
        (
            [priority] => 2
            [url] => http://example.com/
            [language] => en-US
            [site_id] => 1
        )

    [0] => Array
        (
            [priority] => 2
            [url] => http://example.com/gb/?noredirect=en_GB
            [language] => en-GB
            [site_id] => 4
        )

)

dboune commented Sep 29, 2016

Hi Thorsten,

Thanks for the code. I've made the necessary modifications, but unfortunately that doesn't seem to do it. in Mlp_Language_Negotiation::get_redirect_match the possible matches are sorted by priority. Here are the results. If we pop an entry off, then we end up with the GB site, even though our browser language is set to en-us. I can change the priority, but then I can't get a redirect to en-GB if the browser (rather the whole system in the case of safari) is set to use that language in any browser at all.

Array
(
    [1] => Array
        (
            [priority] => 2
            [url] => http://example.com/
            [language] => en-US
            [site_id] => 1
        )

    [0] => Array
        (
            [priority] => 2
            [url] => http://example.com/gb/?noredirect=en_GB
            [language] => en-GB
            [site_id] => 4
        )

)
@tfrommen

This comment has been minimized.

Show comment
Hide comment
@tfrommen

tfrommen Sep 30, 2016

Contributor

Hi,

thanks for reporting back.

I will try this locally with faked data for both Accept-Language and possible translations.

The problem, as I see it now, is that MultilingualPress does not differentiate between language codes that match with their language and region parts, and the ones that only match with their language part. This is, of course, not ideal.

Contributor

tfrommen commented Sep 30, 2016

Hi,

thanks for reporting back.

I will try this locally with faked data for both Accept-Language and possible translations.

The problem, as I see it now, is that MultilingualPress does not differentiate between language codes that match with their language and region parts, and the ones that only match with their language part. This is, of course, not ideal.

@dboune

This comment has been minimized.

Show comment
Hide comment
@dboune

dboune Sep 30, 2016

It does seem that way. I am guessing that in the Firefox and Chrome cases it work out better because they report the language with a lower priority. (region = 1, language = .5/.8). In that way the priority is then properly modified (priority * browser_priority) and the correct site locale can then be selected. With Safari, MultilingualPress adds the non-region language entry because Safari doesn't provide it, and copies the priority, so both end up equal.

Side note: MultilingualPress also adds an empty entry on for Firefox and Chrome, though it doesn't seem to have any ill effect in my setup. I wasn't able to look into why that is.

Mlp_Language_Negotiation::parse_accept_header return values for various browsers

Safari
[en-us] => 1
[en] => 1

Firefox
[en-us] => 1
[en] => 0.5
[] => 0.5

Chrome
[en-us] => 1
[en] => 0.8
[] => 0.8

dboune commented Sep 30, 2016

It does seem that way. I am guessing that in the Firefox and Chrome cases it work out better because they report the language with a lower priority. (region = 1, language = .5/.8). In that way the priority is then properly modified (priority * browser_priority) and the correct site locale can then be selected. With Safari, MultilingualPress adds the non-region language entry because Safari doesn't provide it, and copies the priority, so both end up equal.

Side note: MultilingualPress also adds an empty entry on for Firefox and Chrome, though it doesn't seem to have any ill effect in my setup. I wasn't able to look into why that is.

Mlp_Language_Negotiation::parse_accept_header return values for various browsers

Safari
[en-us] => 1
[en] => 1

Firefox
[en-us] => 1
[en] => 0.5
[] => 0.5

Chrome
[en-us] => 1
[en] => 0.8
[] => 0.8

@tfrommen

This comment has been minimized.

Show comment
Hide comment
@tfrommen

tfrommen Sep 30, 2016

Contributor

Thanks for looking into it. You made the same observations as I did. :)

Currently, I'm thinking about adding a language-only match with a lower priority than the full one. This lower value could be calculated by the original priority of the full language, multiplied with a filterable factor, which defaults to maybe 0.7.

I also saw the entry with the empty key. This happens because MultilingualPress returns '' for the short version of language-only value (that already is short). ;) I will fix this as well. Expect an update in the according branch next week.

Thanks again!

Contributor

tfrommen commented Sep 30, 2016

Thanks for looking into it. You made the same observations as I did. :)

Currently, I'm thinking about adding a language-only match with a lower priority than the full one. This lower value could be calculated by the original priority of the full language, multiplied with a filterable factor, which defaults to maybe 0.7.

I also saw the entry with the empty key. This happens because MultilingualPress returns '' for the short version of language-only value (that already is short). ;) I will fix this as well. Expect an update in the according branch next week.

Thanks again!

@tfrommen

This comment has been minimized.

Show comment
Hide comment
@tfrommen

tfrommen Oct 11, 2016

Contributor

Hey @dboune,

I finally managed to give this a try.

If you have time, feel free to check out the new feature branch. All changes were made in inc/redirect/Mlp_Language_Negotiation.php, so you could also just go ahead and copy that file into your MultilingualPress source.

The commit includes all changes like proposed:

  • introduce a filterable factor (by default .7) for language-only redirect matches;
  • ignore potential empty short language values (i.e., '').

Thanks,
Thorsten

Contributor

tfrommen commented Oct 11, 2016

Hey @dboune,

I finally managed to give this a try.

If you have time, feel free to check out the new feature branch. All changes were made in inc/redirect/Mlp_Language_Negotiation.php, so you could also just go ahead and copy that file into your MultilingualPress source.

The commit includes all changes like proposed:

  • introduce a filterable factor (by default .7) for language-only redirect matches;
  • ignore potential empty short language values (i.e., '').

Thanks,
Thorsten

@tfrommen tfrommen added this to the v2.4.8 milestone Oct 11, 2016

@tfrommen tfrommen self-assigned this Oct 11, 2016

@dboune

This comment has been minimized.

Show comment
Hide comment
@dboune

dboune Oct 14, 2016

@tfrommen This worked for me! Thank you! Safari on IOS also had the same issue (not unexpected), and both are now working as expected for my particular scenario. I have not tested more than two locales with the same root.

dboune commented Oct 14, 2016

@tfrommen This worked for me! Thank you! Safari on IOS also had the same issue (not unexpected), and both are now working as expected for my particular scenario. I have not tested more than two locales with the same root.

@tfrommen

This comment has been minimized.

Show comment
Hide comment
@tfrommen

tfrommen Oct 17, 2016

Contributor

Great! Thanks for testing, @dboune - I will merge this and release it sometime soon.

Contributor

tfrommen commented Oct 17, 2016

Great! Thanks for testing, @dboune - I will merge this and release it sometime soon.

@tfrommen tfrommen closed this in 9bff76b Oct 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment