Language detection not working correctly for most Chinese readers (and add a user interface for all browser language mappings)

I was also testing the browser detection for Simplified Chinese, and this seems to be an issue in 5.15 as well. However, yang_yi_cn's code changes seem to be specific for Drupal 6. Does anyone know how to resolve this for 5.x?

Firefox 3 still uses zh-cn
http://www.w3.org/International/questions/qa-lang-priorities#answer

the correct xml:lang is zh-Hans
http://www.w3.org/International/questions/qa-css-lang#colon-lang
http://www.w3.org/International/articles/language-tags

Mandarin Chinese, Simplified Script: zh-Hans or zh-CN
http://tlt.its.psu.edu/suggestions/international/bylanguage/chinese.html...
http://www.w3.org/International/questions/qa-css-lang#bytheway

I am in favor to correctly map Firefox 3 (or other browser) languages with Drupal basic languages. So, fixing only 1 language is just a start, it is not a solution (although it is a most used language). And we need to allow any mapping of browser languages with Drupal languages, but it should be left for a contrib module.

Just change ""zh-hans" => array("Chinese, Simplified", "简体中文")," into "zh-cn" => array("Chinese, Simplified", "简体中文")," in locale.inc.
It'll be crazy to wait for browsers.

I use Drupal 6.13 for a multi language website, it works very smart, but in Chinese system it's not work.
I traced source code include/language.inc change code
line 83
if (preg_match("!([a-z-]+)(;q=([0-9\\.]+))?!", trim($browser_accept[$i]), $found)) {
to
if (preg_match("!([a-z-]+)(;q=([0-9\\.]+))?!", trim(strtolower($browser_accept[$i])), $found)) {
if ($found[1] == 'zh-tw') { $found[1] = 'zh-hant';}
if ($found[1] == 'zh-cn') { $found[1] = 'zh-hans';}
In firefox $_SERVER[HTTP_ACCEPT_LANGUAGE] would return zh-tw,zh-cn but IE 8 and Chrome would return zh-TW - for traditional Chinese and zh-CN for Simpled Chinese
than it work fine.

I would like to add that zh-hk and zh-sg are also used. Since Hong Kong uses traditional Chinese and Singapore uses simplified Chinese, I changed the patch of cshuangtw to:

if (preg_match("!([a-z-]+)(;q=([0-9\\.]+))?!", trim(strtolower($browser_accept[$i])), $found)) {
  if ($found[1] == 'zh-tw' || $found[1] == 'zh-hk') { $found[1] = 'zh-hant';}
  if ($found[1] == 'zh-cn' || $found[1] == 'zh-sg') { $found[1] = 'zh-hans';}

Now it seems to work with all Chinese variations quite nicely, at least I'm happy. Thanks for finding the correct piece of code.

is n't this issue a duplicate of #221712: locale_language_from_browser() doesn't parse language tags correctly, has a broken logic

This is not a duplicate of #221712, but somewhat related.

The bug #221712 is about language "en-us" not mapping to generic language "en" used by drupal. The proposed and probably correct solution there is to add the language "en" to the list of supported languages so that it will match.

This won't work for Chinese, because there is no generic language of Chinese as in "zh". Chinese has to be always defined as either simplified ("zh-hans") or traditional ("zh-hant"). This bug is about browsers using outdated country specific language identifiers (like "zh-cn" or "zh-tw") instead of the new standard and politically correct identifiers. The solution proposed in #221712 doesn't work here, instead there has to be some manual mapping of language identifiers as proposed above.

Chinese is the only language I know of, where the variants ("zh-hans" and "zh-hant") can't be determined from the language code. That is because some traditional Chinese users won't understand the simplified Chinese and vice versa. It's quite likely that English speakers in New Zealand are able to understand the US English, even though there might be small differences.

Bugs are fixed in the development version first, backported then.

No patch here.

Attached is a patch for Drupal 6.x Core, rewriting the "zh-tw" and "zh-cn" values provided by the browser into "zh-hant" and "zh-hans", respectively, to be what Drupal Core's language system expects. Uses str_replace(), which is a bit simpler and faster than mucking with regular expressions.

"WormFood" in #drupal brought this to my attention today.

This is pretty bad, guys. Right now, browser-based language detection for traditional or simplified Chinese does not work for many browsers. And I'm not talking old ones here; I just tested with Opera 11 and Safari 5.1, and both sent zh-cn and zh-tw instead of zh-hans and zh-hant. Firefox 13 sends zh-hans/t, but apparently this is a recent development (see OP). I don't have access to IE to test.

Yes, the browsers are in violation of a standard, blah blah - that can only take us so far. This is twenty percent or so of the world's population we're talking about here. We need to fix this. Whether that's a one-off that just fixes it for Chinese, or a more generic way to "alias" language codes (perhaps an alter hook invocation in _locale_prepare_predefined_list() or something), it needs to be done, and I'm willing to help.

I too agree, that something needs to be done, and I also am willing to help. I'm a native English speaker, currently living in China, and I want to see this problem get fixed.

I'm setting up a mutlilanguage Drupal 7 site, and I'm testing it with various browsers, and they all seem to work (didn't work last time I tried ), except for Epiphany/2.30.6. So I fired up the 'ole packet sniffer, to see exactly what language codes are being sent, and this is what I've discovered.

Mozilla (firefox 13), sends a language code of:
"Accept-Language: zh-cn,en-us;q=0.7,en;q=0.3", which works fine.

Epiphany, on the other hand, sends:
"Accept-Language: zh-cn, en-us;q=0.90, en;q=0.80, zh;q=0.70", which I expected to work, but it does not.

I am using netcat, and echo in my testing (I'm a Linux guy), so I can customize the header to anything I want. Changing the zh-cn, to zh-hans, makes it work as expected. Why is it that firefox works, but not Epiphany? Is Drupal stripping the "zh-cn" down to "zh", and matching that? That is what it seems to me, because if I use the Epiphany language string, and change the zh q= code to 0.81, I'll get the page in Chinese.

Drupal 7 patch, to change zh-cn to zh-hans internally

Post #3 gives the suggestion of adding the language code zn-cn to the language array in iso.inc (in Drupal 7), but that won't work, because it will change Drupal's internal code to zh-cn, and will then Drupal will not not be able to find the language files (unless you want to change everything, but why work against the flow?), and won't be able to automatically update the core language files.

A few people at the top of this node, give a patch for Drupal 6. Here is my adaptation of that patch for Drupal 7 that fixes this problem, and will allow browsers sending a code of zh-cn to be properly detected internally to drupal, as zh-hans.

find function locale_language_from_browser($languages), in locale.inc, and change it from:

  if (preg_match_all('@(?<=[, ]|^)([a-zA-Z-]+|\*)(?:;q=([0-9.]+))?(?:$|\s*,\s*)@', trim($_SERVER['HTTP_ACCEPT_LANGUAGE']), $matches, PREG_SET_ORDER)) {
    foreach ($matches as $match) {
      // We can safely use strtolower() here, tags are ASCII.
      // RFC2616 mandates that the decimal part is no more than three digits,
      // so we multiply the qvalue by 1000 to avoid floating point comparisons.
      $langcode = strtolower($match[1]);
      $qvalue = isset($match[2]) ? (float) $match[2] : 1;
      $browser_langcodes[$langcode] = (int) ($qvalue * 1000);
    }

to (insert 6 lines)

  if (preg_match_all('@(?<=[, ]|^)([a-zA-Z-]+|\*)(?:;q=([0-9.]+))?(?:$|\s*,\s*)@', trim($_SERVER['HTTP_ACCEPT_LANGUAGE']), $matches, PREG_SET_ORDER)) {
    foreach ($matches as $match) {
      // We can safely use strtolower() here, tags are ASCII.
      // RFC2616 mandates that the decimal part is no more than three digits,
      // so we multiply the qvalue by 1000 to avoid floating point comparisons.
      if (strtolower($match[1]) == 'zh-tw') { 
        $match[1] = 'zh-hant';
      }
      if (strtolower($match[1]) == 'zh-cn') { 
        $match[1] = 'zh-hans';
      }
      $langcode = strtolower($match[1]);
      $qvalue = isset($match[2]) ? (float) $match[2] : 1;
      $browser_langcodes[$langcode] = (int) ($qvalue * 1000);
    }

Do not change "zh" to "zh-hans", (like the D6 patch does) because it will (probably) cause problems.

Patch against D8 fixing this, the logic is fixed as well.

New patch without colors

I've checked this and it looks good.

Rerolled because of #1739994: Use the Language class universally instead of stdObj instances.

Relevant changed part:

+      // Chinese languages.
-       'zh-hans' => (object) array(
++      'zh-hans' => new Language(array(
 +        'langcode' => 'zh-hans',
-       ),
-       'zh-hant' => (object) array(
++      )),
++      'zh-hant' => new Language(array(
 +        'langcode' => 'zh-hant',
-       ),
++      )),

+++ b/core/modules/language/language.negotiation.incundefined
@@ -102,7 +111,7 @@ function language_from_browser($languages) {
-      $browser_langcodes[$generic_tag] = $qvalue;
+      $browser_langcodes[$generic_tag] = $qvalue - 1;

Why do we need this change? Since the languages are ordered by qvalue, this would just put the list a bit lower with the same ordering, no?

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.phpundefined
@@ -66,8 +73,8 @@ class LanguageBrowserDetectionUnitTest extends UnitTestBase {
-      'en-US,fr' => 'en',
-      'fr,en-US' => 'en',
+      'en-US,fr' => 'en-US',
+      'fr,en-US' => 'en-US',

Why would these change. This change does not seem related to the other changes and is not explained. How is the new behavior now logical while the old behavior was passing tests and theoretically nothing changes to make it work like this?

Gabor, I thought we discussed this, but here it goes

Languages are ordered by qvalue, but the $generic_tag (ex zh) is getting the qvalue from the passed value (zh-tw => zh-hant), so you end up with:
$browser_langcodes[zh] = 1000;
$browser_langcodes[zh-hant] = 1000;

Since the logic only changes the langcode if the weight is higher than the previously selected, it will never change to zh-hant, because zh-hans is defined first in the list of available languages.

You can easily test this by removing the - 1 and running the test again.

Besides that: if I browser specifies 'en-US,fr' as available languages and the site has a language en-us if should be the one used in favor of en. This patch fixes that as well: "Always use the most specific one, regardless of the order of the languages inside the array". The reason why this works now is because of the $qvalue - 1

@attiks: thanks, I certainly understand it a bit better now :) We did some epic discussions in #221712: locale_language_from_browser() doesn't parse language tags correctly, has a broken logic though originally when we put the current logic in place, and would love to take opinions of those people to make sure we are not flip-flopping between different preferences. I think the intention there was that if you need to have specific languages before general languages, you should set your language list up like that, period. It also sounds like an edge case to have specific languages and generic languages both installed on a site.

I just want to get this right since there was so much work there already. I hope my cross-comment will get some interest here. Thanks!

The problem is that there's a difference between en-US and zh-hans, the first is English in the US, the second is Chinese

Quoting from #221712-25: locale_language_from_browser() doesn't parse language tags correctly, has a broken logic: "When making the choice of linguistic preference available to the user, we remind implementors of the fact that users are not familiar with the details of language matching as described above, and should provide appropriate guidance. As an example, users might assume that on selecting "en-gb", they will be served any kind of English document if British English is not available. A user agent might suggest in such a case to add "en" to get the best matching behavior."

So we have to pich the most specific one, if it's available regardless of the order defined in Drupal.

For what it is worth, the patch I'm actually using is slightly different from what I posted here. I probably should have included zh-hk in addition to zh-tw in my original patch. I did a little more research into the Chinese language codes available, and I've also added zh-sg (Singapore) and zh-mo (Macau) into the patch I'm using on my drupal sites.

if (strtolower($match[1]) == 'zh-tw' || strtolower($match[1]) == 'zh-hk' || strtolower($match[1]) == 'zh-mo') {
$match[1] = 'zh-hant';
}
if (strtolower($match[1]) == 'zh-cn' || strtolower($match[1]) == 'zh-sg') {
$match[1] = 'zh-hans';
}

@WormFood: do you by any change have a list of all possible values? My knowledge about Chinese is a bit limited to say the least ;-)

@attiks: Those 5 language codes I listed in my last post are the only ones I'm aware of.
zh-cn (mainland China), and zh-sg (Singapore) use simplified Chinese.
zh-hk (Hong Kong), zh-tw (Taiwan), and zh-mo (Macau) use traditional Chinese.
I did find reference to a zh-chs (simplified), and zh-cht (traditional) language codes, but I suspect those have not been in use by web browsers for a very long time, if ever.

I know this is more than you asked for, but sometimes it helps to make more sense of things, if you understand some of the background. I'll try to keep this short. After the communists took over China, some of the Chinese characters were "simplified". Since Taiwan, Hong Kong, and Macau were not under communist control, they never changed their writing system, and they continue to use the "traditional" Chinese characters today. The Chinese written in Singapore has more of a mainland Chinese influence, so they use simplified characters in Singapore.

In case you're wondering where the "zh" part of the language code comes from; that is for "zhong guo" (中国), the name of China, in Chinese. The "han" part is probably for "han zi" (汉字 ), which is the word for "Chinese character", in Chinese. The "s" or "t", if you didn't guess by now, is for Simplified or Traditional. Chinese written with Latin (Western) letters is called "pinyin" (拼音 literally "spell sound"), and there may be a language code for pinyin, but I couldn't find reference to one.

Thank you attiks, and everyone else working on this issue. I'm very happy to finally see some actions being taken to fix this. I don't quite understand the testing framework part of the patch, otherwise I'd be trying to help in that area.

New patch, I also added zh-chs and zh-cht

@WormFood: thanks for the explanation, good to know

Quoting from #221712-25: locale_language_from_browser() doesn't parse language tags correctly, has a broken logic: "When making the choice of linguistic preference available to the user, we remind implementors of the fact that users are not familiar with the details of language matching as described above, and should provide appropriate guidance. As an example, users might assume that on selecting "en-gb", they will be served any kind of English document if British English is not available. A user agent might suggest in such a case to add "en" to get the best matching behavior."

So we have to pich the most specific one, if it's available regardless of the order defined in Drupal.

Well, the text you quote says we should pick the more general one when the user provided the more specific one. This patch looks like makes the change to the other way around instead, to pick the more specific one if the general was provided.

Either way it seems to be shaping up to be a more over-arching mapping of external language codes to internal language codes problem similar to #580260: Map local language codes to remote language codes. Not really sure if we want to go down on that route, and I acknowledge that Chinese has this problem the most badly. :/

@Gabor: the quote says the UA might suggest to add more generic one to get the best UX. Drupal as a platform can add automatic fallback to the most generic one in case the more specific one isn't defined, but that isn't part of the RFC.

Examples (assuming no qvalue are set):
Browser send en-us and Drupal defines en => use en // generic one (courtesy of Drupal)
Browser send en-us and Drupal defines en + en-us => use en-us // exact match
Browser send en-us and Drupal defines en + en-uk => use en // generic one (courtesy of Drupal)

Browser send en-us, fr and Drupal defines en, fr => use fr // exact match
Browser send en-us, fr and Drupal defines en, en-us + fr => use en-us // exact match, first one selected
Browser send en-us, fr and Drupal defines en, en-uk + fr => use fr // exact match

The change makes sense: we use the generic language if it is not specified, but we give it a slightly lower priority so as not to prevent more-specific matches from happening.

Obviously, we need to add a very detailed explanation of why we do that right above this line.

I also suggest we replace - 1 by - 0.1 so that we don't risk a collision with another weight specified in the header.

Needs work for documentation.

@Damien: thanks for you review and encouragement. It makes me much more comfortable saying this is a good direction :) @attiks: let's document this and then we can get it in. We can still figure out more generic language matching at #580260: Map local language codes to remote language codes (or its core equivalent) later.

I think this issue has the same condition http://drupal.org/node/870350

@Damien thanks for reviewing, new patch attached.

#32: i365615-chinese.patch queued for re-testing.

+      // Handle Chinese so it maps right, some browsers are sending
+      // wrong langcodes.

Please elaborate more on why this is needed. Especially, mention that browsers are sending a langcode that is not a valid IETF language tag.

What about "Handle Chinese so it maps right, some browsers are sending non standard langcodes, we map them to the right ones."

I discussed this a bit with damien in IRC

http://www.iana.org/assignments/language-subtag-registry defines the following

zh-Hans - simplified Chinese
zh-Hans-CN - PRC Mainland Chinese in simplified script
zh-Hans-HK - Hong Kong Chinese in simplified script
zh-Hans-MO - Macao Chinese in simplified script
zh-Hans-SG - Singapore Chinese in simplified script
zh-Hans-TW - Taiwan Chinese in simplified script

zh-Hant - traditional Chinese
zh-Hant-CN - PRC Mainland Chinese in traditional script
zh-Hant-HK - Hong Kong Chinese in traditional script
zh-Hant-MO - Macao Chinese in traditional script
zh-Hant-SG - Singapore Chinese in traditional script
zh-Hant-TW - Taiwan Chinese in traditional script

So we need mappings as follows:

zh-tw --> zh-hant-tw
zh-hk --> zh-hant-hk
zh-mo --> zh-hant-mo
zh-cht --> zh-hant

zh-cn --> zh-hans-cn
zh-sg --> zh-hans-sg
zh-chs --> zh-hans

Mappings are now defined in language.mappings.yml
The test is now a WebTest since it needs to load the config

1 todo left: how to make it easy for others to add/override the mappings

#37: i365615-chinese.patch queued for re-testing.

#37: i365615-chinese.patch queued for re-testing.

#37: i365615-chinese.patch queued for re-testing.

I agree it is the best to have a general mapping solution for language codes instead of just hard-wiring specific mappings. This was underlined in #580260: Map local language codes to remote language codes too. I thought it would be too much of a burden to solve here, but looks like you have a good handle on it :)

Anybody has an idea on how to make this easy for others to add new mappings? Is this solved by CMI, or do we have to read all configs of the form language.mappings.*?

I had a quick talk with heyrocker and we need to add an UI for this

I think the ancient zh-chs and zh-cht language codes are totally unnecessary. I'm pretty sure that if web browsers ever sent that language code, it was a very long time ago. The real problem with Drupal, was not recognizing zh-cn/zh-tw/etc as a legitimate language code.

I was reading about the different language codes, trying to understand the logic behind codes like zh-sg, and zh-mo, and it is my understanding is that while the written part may be the same language, but other aspects may be different for that locale, like the currency symbol, or the date format. How is drupal supposed to properly support this aspect of the differences in locales? This question also applies to languages more familiar to us, like en-us and en-gb. Slight spelling differences aside, we have other differences like date format and currency symbols. How are we supposed to change those settings for the different regional differences? I haven't installed Drupal 8 yet, but with Drupal 7, even tho I can change the date formats for the different languages, I don't have the choice to display the date in Chinese.

I agree, there should be some easy way to map the language codes to whatever you want. I think there should be a generic way, to map whatever language code the browser is sending, into whatever internal language code you want, and be able to set the date format accordingly. For example, maybe zh-cn will give you simplified Chinese, with a date format one way, but zh-sg will give you the same language, but with a different date format. This will give the site owner maximum flexibility, and would even allow them to configure their own custom language codes (maybe for testing) and date formats.

Also, the simple patch I posted to fix this problem seems to be identical for Drupal 8, as it is for Drupal 7, but Drupal 6 will be slightly different. If this is changed into a full fledged feature, rather than just a quick'n dirty hack, will it be backported to Drupal 6/7?

[edit] oops. I changed the status unintentionally. I don't see how to revert that, without a new post.

We can add a configure link at admin/config/regional/language/detection and just show everything in a huge textarea

OK?

What is the problem with Chinese dates? And will it not be fixed by the new mappings?
zh-tw: zh-hant-tw
zh-hk: zh-hant-hk
zh-mo: zh-hant-mo
zh-cht: zh-hant
zh-cn: zh-hans-cn
zh-sg: zh-hans-sg
zh-chs: zh-hans

will it be backported to Drupal 6/7?

It should be not a huge problem, the only difference is that config will get replaced by variable_get/variable_set

Not sure if that will be fixed by the new language mappings. Is it your intent to add zh-hant-tw, zh-hant-hk, zh-hant-mo, etc language codes? Because really, they're all the same languages. The only differences may be their date format. I'm not too sure in this area. I know regular Chinese dates are in the format of 2012年08月30日, but I don't get that option with Drupal 7. I'm not sure what date format is regular in those other locales (Taiwan, Hong Kong, Macau, Singapore), they may not need to be changed at all.

Also, I noticed in the list of language codes, that there is a code for Chinese written in pinyin, and Wades-Giles (using the Latin alphabet). For example, 北京, the nation's capital is written as "Beijing" in pinyin, and "Peking" in Wades-Giles. People don't usually use pinyin for every day use, it is more of a teaching language. I don't think that needs to be added as a language option (I think it would be a little silly), however, it would make Drupal support a complete set of Chinese language options. It would be a cool option for a Chinese language learning site.

As far as a big text area goes, I think that would be fine for testing, but I think it should have a normal interface, like the rest of the Drupal admin areas. Personally, I think it would be better to pick the date formats, at the same place you configure the language. Perhaps have the ability to add your language code (zh-cn) for example, map it to the proper internal code (zh-hans), and select the date/time formats at the same place. Then you could add zh-sg and zh-hans-sg, and also map them to zh-hans, but have a different date/time format. Keep the date format menu item in the same place, but just have it direct you to the language page, where those settings are. Just my 2 cents worth.

I see what you mean, let's first try to solve to language detection problem and open a new issue for the date problem.

Basically you have 1 site in zh-hans, but you want different date formats for zh-hans-tw and zh-hans-cn, right?

PS: If you open a new issue can you post the link in this one, thanks

In Drupal 7 I can do dates like 2012年03月09日 at en/admin/config/regional/date-time/locale

new patch including 'proper' admin interface

And a screenshot

#53: i365615-chinese.patch queued for re-testing.

@wormfood, there's an issue about the date problem, see #1345758: META: Provide locale (regional) formats framework for automated translation of non textual data

+++ b/core/modules/language/language.admin.incundefined
@@ -807,3 +807,183 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+    '#markup' => t("You can add custom mappings to map browser languages to Drupal languages."),

Use single quotes.

+++ b/core/modules/language/language.admin.incundefined
@@ -807,3 +807,183 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+ * Theme browser configuration form as table.

Requires "@ingroup themeable" phpdoc.

+++ b/core/modules/language/language.admin.incundefined
@@ -807,3 +807,183 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+  $question = t('Are you sure you want to delete %browser_langcode', array(

We need the question mark.

+++ b/core/modules/language/language.moduleundefined
@@ -596,3 +616,23 @@ function language_url_outbound_alter(&$path, &$options, $original_path) {
+function language_get_mappings() {
...
+function language_set_mappings($mappings) {

Clever, backporting made easy :-)

+++ b/core/modules/language/language.negotiation.incundefined
@@ -100,9 +111,20 @@ function language_from_browser($languages) {
+    // For chinese languages the generic tag is either zh-hans or zh-hant, so we
+    // need to handle this separately.
+    $generic_tag = '';
+    if (strlen($langcode) > 7 && (substr($langcode, 0, 7) == 'zh-hant' || substr($langcode, 0, 7) == 'zh-hans')) {
+      $generic_tag = substr($langcode, 0, 7);
+    }
+    else {
+      $generic_tag = strtok($langcode, '-');
+    }
+    if (!empty($generic_tag) && !isset($browser_langcodes[$generic_tag])) {
+      // Add the generic langcode, but make sure it has a lower qvalue as the
+      // more specific one, so the more specific one gets selected if it's
+      // defined by both the browser and Drupal.
+      $browser_langcodes[$generic_tag] = $qvalue - 0.1;

Do we need this chinese specific code now that we have the mapping configurable?

+++ b/core/modules/language/language.negotiation.incundefined
@@ -81,7 +81,18 @@ function language_from_browser($languages) {
+    $mappings = language_get_mappings();

language.negotiation.inc is included in install.core.inc for calling language_from_browser(). language_get_mappings() is defined in language.module, so it needs to be imported too in install.core.inc for the installation to work, or move language_get_mappings to language.negotiation.inc.

Otherwise you got an WSOD on installation.

New patch adresssing feedback from #58 and #59

We need "For chinese languages the generic tag is either zh-hans or zh-hant, so we need to handle this separately." because there's no 'zh' language, so the only way to find this generic Chinese language is to handle it separately.

Good stuff, looks like a useful UI to me. However, now we'd need tests for adding to/changing the config. Apart of that, minor comment mistakes:

+++ b/core/modules/language/language.admin.incundefined
@@ -807,3 +807,189 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+  // Add empty row

Dot at end of line.

+++ b/core/modules/language/language.admin.incundefined
@@ -807,3 +807,189 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+/**
+ * Delete a language mapping form.
+ */
...
+/**
+ * Delete a language mapping submit.
+ */

Delete a mapping form? A mapping submit?!

+++ b/core/modules/language/language.moduleundefined
@@ -596,3 +616,24 @@ function language_url_outbound_alter(&$path, &$options, $original_path) {
+  $config->save();
+}
\ No newline at end of file

Should have a newline at end of file.

comments fixed, tests added.

Tests added, tag removed

+++ b/core/modules/language/language.admin.incundefined
@@ -970,7 +973,7 @@
+ * Delete a browser language negotiation mapping.

@@ -982,7 +985,7 @@
+ * Delete a browser language negotiation mapping.

Should we say "Form for deleting a browser language negotiation mapping." and "Submit handler for deleting a browser language negotiation mapping."?

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.phpundefined
@@ -28,7 +28,7 @@
-  function testLanguageFromBrowser() {
+  function testLanguamergeFromBrowser() {

Typo. Should be testLanguageFromBrowser.

It should be the last review before RTBC :-)
Thanks attiks!

should be ok

If tests pass, it's fine for me. :)

RTBC

First off, WOW! How nice that you not only solved the immediate problem but gave people the ability to solve this on their own for future issues, too. :)

Since this is slated for D7 backport I need to be a bit more of a hard-ass on this review. Apologies in advance.

+++ b/core/modules/language/language.moduleundefined
@@ -596,3 +616,24 @@ function language_url_outbound_alter(&$path, &$options, $original_path) {
+ * Returns language mappings.
...
+ * Stores language mappings.

Could we get a bit more info here? I can imagine reading this function out of context of this issue and asking the question, "Language mappings between what and what?"

In fact, should we name this function something with "browser" in the name to help with that?

+++ b/core/modules/language/language.admin.incundefined
@@ -807,3 +807,192 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+    '#markup' => t('You can add custom mappings to map browser languages to Drupal languages.'),

Can you add a sentence that explains why I would want to / how I know I need to do that?

+++ b/core/modules/language/config/language.mappings.ymlundefined
@@ -0,0 +1,7 @@
+zh-tw: zh-hant-tw
+zh-hk: zh-hant-hk
+zh-mo: zh-hant-mo
+zh-cht: zh-hant
+zh-cn: zh-hans-cn
+zh-sg: zh-hans-sg
+zh-chs: zh-hans

We don't typically put comments in the default YML files that come with modules, but in this case I think it'd be a really good idea, because I can't imagine another place to document why we have these defaults and what specific problem we're trying to solve.

Just a line or two that summarizes it would suffice.

+++ b/core/modules/language/language.admin.incundefined
@@ -807,3 +807,192 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+  foreach ($mappings as $browser_langcode => $drupal_langcode) {
+    $form['mappings'][$browser_langcode] = array(
+      'browser_langcode' => array(
+        '#type' => 'textfield',
+        '#default_value' => $browser_langcode,
+        '#size' => 20,
+      ),
+      'drupal_langcode' => array(
+        '#type' => 'textfield',
+        '#default_value' => $drupal_langcode,
+        '#size' => 20,
+      ),
...
+  $form['mappings']['_new'] = array(

This seems to be breaking a UX pattern used elsewhere in Drupal, which is that anytime we show a list of things in a table, it's simply that: a list of things in a table. If you want to edit then, that's a separate "edit" action.

Doing the same here would allow us to get rid of that funky and special "_new" keyname, in favour of something more consistent and legible like "language_mapping."

So unless I'm wrong and we already do this in other places in the interface, I think we should move to that.

+++ b/core/modules/language/language.admin.incundefined
@@ -807,3 +807,192 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+        // Make sure browser_langcode is unique
...
+        // Make sure browser_langcode is unique

(nitpick) Periods at the end of these sentences.

+++ b/core/modules/language/language.admin.incundefined
@@ -807,3 +807,192 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+        elseif (preg_match('/[^a-z\-]/', $data['browser_langcode'])) {
+          form_set_error('mappings][' . $key . '][browser_langcode', 'Browser language codes can only contain lowercase letters and a hyphen(-).');
+        }
+        elseif (preg_match('/[^a-z\-]/', $data['drupal_langcode'])) {
+          form_set_error('mappings][' . $key . '][drupal_langcode', 'Drupal language codes can only contain lowercase letters and a hyphen(-).');
+        }

Just asking, but is there a way to consolidate this into one if statement somehow?

+++ b/core/modules/language/language.admin.incundefined
@@ -807,3 +807,192 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+        form_set_error('mappings][' . $key . '][drupal_langcode', 'Both browser and Drupal language codes must be provided.');

Can we not just make these required fields then?

+++ b/core/modules/language/language.negotiation.incundefined
@@ -100,9 +111,20 @@ function language_from_browser($languages) {
+    // For chinese languages the generic tag is either zh-hans or zh-hant, so we

(nitpick) Capitalize "Chinese."

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.phpundefined
@@ -66,8 +75,8 @@ class LanguageBrowserDetectionUnitTest extends UnitTestBase {
-      'en-US,fr' => 'en',
-      'fr,en-US' => 'en',
+      'en-US,fr' => 'en-US',
+      'fr,en-US' => 'en-US',

There was some really good discussion above as to why we changed this; can we make sure some of this is captured in code comments here so the next person doesn't flag an issue with these being inconsistent with the lines above?

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.phpundefined
@@ -128,4 +152,80 @@ class LanguageBrowserDetectionUnitTest extends UnitTestBase {
+  function testUIBrowserLanguageMappings() {

Let's get a one-liner of PHPDoc above this to explain what it's doing.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.phpundefined
@@ -128,4 +152,80 @@ class LanguageBrowserDetectionUnitTest extends UnitTestBase {
+    $message = t('Are you sure you want to delete !browser_langcode?', array(
+      '!browser_langcode' => $browser_langcode,

Is there a reason we do ! here rather than @? This is just plain-text, right?

Oh son of a monkey. I accidentally pasted my Dreditor review twice. If you got that in e-mail, please read again, it's a lot less confusing now. ;P

Thanks for reviewing, new patch attached:

• all feedback processed
• UI is still the same but I don't mind changing it, I'll try pinging Bojhan
• consolidating the validation logic isn't going to improve readability

@webchick, @attiks: it is not entirely true Drupal 7 did not have any in-place editing on the admin UI. It might be a bit obscure, but the book outline admin page lets you reoder and retitle book pages in a book (with edit links going to the full node page):

Drupal 7 also has in-place addition in the roles configuration screen and the fields configuration screen. As for more examples of in-place editing, the language UIs in fact introduced this earlier at two places in Drupal 8.

1. For URL prefix/domain editing, it makes ton of sense to have them edit all at once, since if you eg. need to swap domains, you can easily end up on different language admin pages, while you remove the domain from one language and add on another. Sensitive configuration that is likely changed all at once:

2. For interface translation, what we found is that the pattern is that people work with short strings and usually want to edit multiple of them for the same language is very short succession. This workflow is in part where the success of l10n_client module came from. So introduced the same thing:

In summary, I'd argue there are multiple places in core in Drupal 7 where in-place addition in a table was working and at least one place with in-place editing on the admin UI. Now there are at least two more places in Drupal 8 where in-place editing of multiple things at once is present on the admin UI.

Moving back to RTBC on those grounds and that @attiks took care of the rest of the concerns.

I am not particularly excited about introducing this pattern, it is uncommon and has all kinds of issues (e.g. people not clicking save, changing things then pressing delete, etc.). However in this case I can see that its useful, because you are going to be mapping a lot of languages, rather than just one (fyi, book and roles are really poor examples, they shouldn't have this pattern).

I do wish to note, that the description is a little bit strange. It should help in filling out this form, most users won't know what to fill in here - perhaps that can be part of release nodes orso.

Description is strange indeed, but people setting up multilingual sites and having this kind of problems are likely going to know a bit about how to handle the language problem, up till now they didn't had any way to do this.

Trying to explain this problem in a couple of lines is likely going to fail, because it depends on the browsers, languages defined in Drupal, use case, ...

[12:50] GaborHojtsy attiks: well, the language_negotiation_configure_browser_form() 'help' markup text should be in a hook_help() with language module really
[12:53] GaborHojtsy attiks: and then it can be a little expanded like "Browsers use different language codes to refer to the same languages. You can add and edit mappings from browser language codes to the language codes used by Drupal."

moved help to hook_help and changed the wording

Even resolved Bojhan's concerns now. :)

@@ -0,0 +1,9 @@
+# Most browser do not send abbreviated languages codes not standard

This doesn't sound like proper English. Needs a bit of tweaking. (Update: DrEditor cut off the next line it seems.)

+++ b/core/modules/language/language.moduleundefined
@@ -37,6 +37,10 @@ function language_help($path, $arg) {
+    case 'admin/config/regional/language/detection/browser':
+      $output = '<p>' . t('Browsers use different language codes to refer to the same languages. You can add and edit mappings from browser language codes to the language codes used by Drupal.') . '</p>';
+      return $output;

One thing that came to mind when reviewing this patch and looking at the screenshots was: how do people know what language codes Drupal supports? I felt that could use a bit more documentation/guidance for site builders. See #54 for example. That UI made me believe that I need to know the Drupal language codes, or that I need to look up the Drupal language codes somewhere. If they are pre-defined, it might make more sense to have a drop-down menu for the Drupal ones? I don't know. Maybe I'm slightly confused about this but as an end-user I feel a bit lost with the current UI.

Thanks for the review, I changed the wording inside the .yml file

The second comment is a bit more difficult:
1/ drop-down isn't going to work, because it implies that the default aren't going to be loaded, since you need the language module to define the new languages first you'll end up with a chicken and egg problem. If we really want a drop-down we need to define all possible language code (2 characters, 3 characters, 2 characters + country/region, ...) and it means we need to maintain that list as well. Related issue #923304-20: zh-cn and zh-tw incorrectly used

2/ a user has to know about language codes, but it's almost the same as Drupal 7, this patch only allows you to fine-tune how language codes are handled (new optional feature) and will probably be used by 'Chinese' sites so solve their problems and huge multilingual sites that for instance wants to use nl-nl and nl-be, instead of just nl (ex: 'kleed' versus 'jurk').

3/ we can link to http://www.w3.org/International/articles/language-tags/ but i doubt that it will help somebody that has no idea what to do on this screen in the first place.

PS: I know UI/UX is not optimal compared with adding a new language (admin/config/regional/language/add) and I'm open for any suggestions.

I guess, the only way to provide a dropdown would be if it would contain all predefined and all configued languages, like the locale import form. The chicken and egg problem is if the dropdown only has configured languages, then we cannot ship with the Chinese defaults, since it would not be editable on non-Chinese sites.

Also, I assume tHe drop-down would be for the target (internal to Drupal) language codes. Dropdowns for the external language code do not seem possible.

#80I completely agree
#81Providing a list for internal languages is not possible either, for the moment Drupal only ships with language codes, not language + country/region or something like language-extlang-script-region-variant-extension-privateuse (source)

If this is the only reason to hold back on this patch, can you open a follow-up issue to address this, so we can start backporting this?

If the target (internal) languages are not among ones shipped with Drupal, then why we do we map to them? Id they are among the ones shipped, we could build a dropdown like the import form, with the configured AND predefind languages along each other, no?

@Gabor, because you a user can define custom languages like nl-be and he probably wants to map nl to nl-be at that time as well, in theory we can use a drop down for the target (Drupal) language, but not if we want to ship the defaults needed to solve the 'Chinese' problem.

I think we just have to decide:

1. Use textboxes for both languages as it's right now
2. Use a dropdown for the target (Drupal) language code and ship without defaults

Personally I prefer option 1, but I'm fine with 2.

If the target (Drupal) language codes are not actually ones predefined in Drupal (ie. translated on localize.drupal.org), then people would use different language codes (ones equal to codes on localize.drupal.org) to sync with the community, right? How do we plan to bridge that gap and what sense does this mapping have if it does not actually map to language codes that people will use?

if I was to setup a site for the Benelux I would define my languages as follows: nl-be, nl-nl, fr-be and fr-lu because there're regional difference between nl-be and nl-nl. As a start I would copy nl.po to nl-be.po and nl-nl.po as a start, but I would tweak the translations based on the region.

How to sync back to the community is a different (but also important) issue, but in the long wrong we need some system that allows extending/overriding a 'base' language. 99% of the translations are the same in Dutch, so we would have nl, but also nl-nl and nl-be that contain small alterations of nl. If a site is using nl-be right now, it shouldn't be allowed to sync back to l.d.o.

My browser languages are setup as "nl-be, nl, nl-nl, en" so it can be important for a site admin to alter this depending on his/her target audience/browser (nl-nl isn't listed in FF, but it is in IE)

New patch after discussion in irc

[15:15]	GaborHojtsy attiks: I think its concerning that we configure this up in a way that is not compatible with other parts of our configuration
[15:16]	GaborHojtsy	attiks: so if you add all Chinese from the default list of languages in Drupal
[15:16]	GaborHojtsy	attiks: it would still not work at all, right?
[15:16]	GaborHojtsy	attiks: it does not really solve the problem unless you add all your Chinese as custom languages
[15:17]	attiks	GaborHojtsy: simplified and tradition Cinese is in the list, but you'll need the mapping
[15:17]	attiks	GaborHojtsy: no need to define a custom Chinese language
[15:18]	GaborHojtsy	attiks: so zh-hant and zh-hans
[15:19]	attiks	GaborHojtsy: are provided by Drupal
[15:27]	GaborHojtsy	attiks: yeah, ok
[15:28]	GaborHojtsy	attiks: so this is pretty confusing that we'd ship with mapping for languages that Drupal does not ship with or support via UI translation, etc.
[15:28]	GaborHojtsy	attiks: were these all part of the original problem space or it just extended to this point?
[15:29]	attiks	GaborHojtsy: most are from original problem, they are extended to handle all Chinese variants
[15:30]	attiks	GaborHojtsy: if it makes you feel better, we can ship with other defaults
[15:30]	attiks	GaborHojtsy: +# Browsers use different language codes to refer to the same languages,
[15:30]	attiks	+# these defaults handles the most common cases.
[15:30]	attiks	+zh-tw: zh-hant # Taiwan Chinese in traditional script
[15:30]	attiks	+zh-hk: zh-hant # Hong Kong Chinese in traditional script
[15:30]	attiks	+zh-mo: zh-hant # Macao Chinese in traditional script
[15:30]	attiks	+zh-cht: zh-hant # traditional Chinese
[15:30]	attiks	+zh-cn: zh-hans # PRC Mainland Chinese in simplified script
[15:30]	attiks	+zh-sg: zh-hans # Singapore Chinese in simplified script
[15:30]	attiks	+zh-chs: zh-hans # simplified Chinese
[15:33]	GaborHojtsy	attiks: yup, that sounds like would fit in with our model of languages
[15:33]	attiks	GaborHojtsy: I'll reroll the patch
[15:34]	GaborHojtsy	attiks: which would then allow us to have a *dropdown* for target language
[15:34]	GaborHojtsy	attiks: which was Dries' main concern
[15:34]	GaborHojtsy	attiks: that you'd need to cross-check target langcodes manually
[15:35]	attiks	GaborHojtsy: so target -> dropdown limited to what list?
[15:35]	GaborHojtsy	attiks: I'd use the same list as the .po import form (copy-paste code?
[15:35]	GaborHojtsy	attiks: i.e the configured languages and the predefined languages
[15:35]	attiks	GaborHojtsy: will do
[15:35]	GaborHojtsy	attiks: once someone configures a more fine-tuned Chinese language
[15:36]	GaborHojtsy	attiks: they could assign that as a target language AFTER that
[15:36]	attiks	GaborHojtsy: correct

Much better UX IMHO, reflects on what Dries proposed, very close :)

There are some minor UI label fixes now that we display languages vs. request the more obscure language codes. And one more remaining piece for me looks like where a new item is added, it is not possible to specify the language in a dropdown. It should be the same for consistency (and data validation automatically provided by the dropdown). If we are concerned that its cumbersome to figure out you still need to add a corresponding language, we can link the "languages used by Drupal" to the language list. We do the same at other places to backlink there for quick navigation.

no idea what I was thinking, but this one should be better.

What is that bottom row?

To add a new mapping

Screenie of new UI as briefly discussed with Bojhan

EDIT: Fieldset is collapsed by default.

Patch implementing the new UI

Looks good to me., resolves Dries' concern, resolves Bojhan's concerns :)

Great work all! :)

I reviewed this patch in some more detail and found the following issues:

+++ b/core/modules/language/language.admin.inc
@@ -813,3 +813,203 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+function language_negotiation_configure_browser_form($form, &$form_state) {
...
+  // Initialize a language list to the ones available, including English.
+  drupal_static_reset('language_list');
+  $languages = language_list();

It would be great to have a code comment here that explains why this static needs to be reset. Even better would be to eliminate it, if possible.

+++ b/core/modules/language/language.admin.inc
@@ -813,3 +813,203 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+  $form['mappings'] = array(
+    '#type' => 'container',

AFAICS, the #type container is not needed here and can be removed.

+++ b/core/modules/language/language.admin.inc
@@ -813,3 +813,203 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+  $form_state['redirect'] = 'admin/config/regional/language/detection';

Normally, this is specified in the form submission handler. Any particular reason for why it's specified in the constructor?

+++ b/core/modules/language/language.admin.inc
@@ -813,3 +813,203 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+function language_negotiation_configure_browser_form_validate($form, &$form_state) {
...
+  // Check all mappings.
+  $mappings = array();
+  if (isset($form_state['values']['mappings'])) {
+    $mappings = $form_state['values']['mappings'];
+    foreach ($mappings as $key => $data) {
+      // Make sure browser_langcode is unique.
+      if (array_key_exists($data['browser_langcode'], $unique_values)) {
+        form_set_error('mappings][' . $key . '][browser_langcode', 'Browser language codes must be unique.');
+      }
+      elseif (preg_match('/[^a-z\-]/', $data['browser_langcode'])) {
+        form_set_error('mappings][' . $key . '][browser_langcode', 'Browser language codes can only contain lowercase letters and a hyphen(-).');
+      }
+      $unique_values[$data['browser_langcode']] = $data['drupal_langcode'];
+    }
+  }
+
+  // Check new mapping.
+  $data = $form_state['values']['new_mapping'];
+  if (!empty($data['browser_langcode'])) {
+    // Make sure browser_langcode is unique.
+    if (array_key_exists($data['browser_langcode'], $unique_values)) {
+      form_set_error('mappings][' . $key . '][browser_langcode', 'Browser language codes must be unique.');
+    }
+    elseif (preg_match('/[^a-z\-]/', $data['browser_langcode'])) {
+      form_set_error('mappings][' . $key . '][browser_langcode', 'Browser language codes can only contain lowercase letters and a hyphen(-).');
+    }
+    $unique_values[$data['browser_langcode']] = $data['drupal_langcode'];
+  }

1) We're missing t() around the error messages.

2) It looks like the second loop does exactly the same as the first, only on a different element. We can eliminate it entirely by creating a new array upfront and merging the 'mappings' with the 'new_mapping', so that there is only one loop.

+++ b/core/modules/language/language.module
@@ -581,3 +605,32 @@ function language_url_outbound_alter(&$path, &$options, $original_path) {
+function language_get_browser_drupal_langcode_mappings() {
...
+  if ($config->isNew()) {
+    config_install_default_config('module', 'language');
+    $config = config('language.mappings');
+  }

If browser language negotiation is enabled, this code runs on every request. Drupal should not dynamically create or change configuration on arbitrary requests.

The calling code seems to be perfectly able to handle an empty mapping. So let's just remove this automatic creation, please.

+++ b/core/modules/language/language.negotiation.inc
@@ -105,9 +125,20 @@ function language_from_browser($languages) {
+    // For Chinese languages the generic tag is either zh-hans or zh-hant, so we
+    // need to handle this separately.

Any chance to explain the code logic for the Chinese language handling a bit more? It's not easy to follow what is being done, and more importantly, why it's being done.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.php
@@ -128,4 +159,68 @@ class LanguageBrowserDetectionUnitTest extends UnitTestBase {
+    $this->assertRaw($message, 'Question found.');
...
+    $this->assertText('Browser language codes must be unique.', 'Error message displayed.');
...
+    $this->assertText(t('Browser language codes must be unique.'), 'Error message displayed.');

When asserting literal/human-readable strings on a page, it is much better for debugging to not specify a custom assertion message - so if the test will fail, then it is immediately clear what string was expected but didn't appear.

Thanks for the review, new patch addressing all but one of Sun's concerns:

language_negotiation_configure_browser_form_validate is still split in 2 similar parts: the first part is checking existing entries, the second part is checking a new entry, if one is provided. I had it with an array merge before, but it wasn't easy to read.

Thanks! :)

Alright, let's go with this! (There are still some minor issues in the code, but let's not hold up this great patch on them.)

@sun if you can create a follow-up issue for the minor stuff, I'll fix them

Really awesome work on this, attiks! Looks good from my side, too.

Committed and pushed to 8.x. YAY! Moving to 7.x for backport. Backporting something of this magnitude, however, will need David's sign-off. Temporarily re-assigning to him.

+++ b/core/modules/language/language.admin.incundefined
@@ -813,3 +813,200 @@ function language_negotiation_configure_session_form($form, &$form_state) {
+    '#title' => t('Drupal langauge'),

Typo in the UI text.

Also added a short changelog entry, since we will not be able to track this easily if it falls back into Drupal 7.

I found 2 more 'langauges'

Oops. :P

Committed and pushed to 8.x. Back to 7.x.

Removing sprint tag. We don't really have the luxury to direct D8 developers to work on backports given the close proximity of the D8 feature freeze.

Adde change notice at http://drupal.org/node/1776566 (for front facing changes). Feel free to update/change as needed. I consider this done for D8.

I just read through this issue, and it seems pretty awesome despite the fact that I only understand about half of it :)

What's the benefit of introducing a giant user interface for this in Drupal 7, vs. leaving it to a contrib module? (Are the bugs/use cases that require a new user interface to fix actually that common?) From what I could tell, this issue started off focused on the specific issue with Chinese, but then expanded... So for Drupal 7, is it possible to fix the Chinese issue in core and also add enough of the "language mappings" API that a contrib module could expand on it, but leave the user interface and the rest out? That would seem to be a more traditional (and safer) bug fix for a stable release.

Hard for me to comment more, so I'm unassigning myself for now :)

#106 they are pretty common, it affects all multilingual Chinese sites, so that's lots of people. But it also affects sites using country specific language codes (like en-us, en-uk)

We should be able to split it into UI and API, but the UI is pretty straight forward, so I don't think we gain a lot by splitting it. If we split it, it means somebody has to write and maintain this in contrib and a lot of people are not going to find it.

My 2c: backport everything. I don't mind doing the backport, but I would like to know if backporting the whole patch is acceptable.

New follow-up issue created #1784380: Remove zh-cht and zh-chs from language.mappings.yml

I think it would be fine/safe to backport the whole thing, its just new configuration at a place where we never had config.

I would like to strongly recommend that the zh-chs and zh-cht language codes are removed from this patch. They're very old language codes, that are simply not being used anymore. I don't think they were ever sent by web browsers, or if they were, it was a very long time ago. It is just more stuff that just does not need to be there. I only mentioned those codes before, as other Chinese language codes, but didn't expect them to be included in the patch.

By the way, great job! I hope this helps the adoption of Drupal in China. I really think this is one factor that was hindering that.

oops, changed the status by mistake (damn GUIs). Changing it back.

Damn... My biggest client is having this problem on 10+ D7 sites. Roughly 90% of their users are Chinese speaking and from Google Analytics I'm seeing that between 10 and 40% of browsers are reporting zh-cn, zh-tw, or zh-hk (0% are reporting hans or hant). Unfortunately, only 3% of visitors are actually starting out on the correct language sub-domain front page which is not the best user experience.

I'm going to see if my client gives me the go ahead to work on the backport. Otherwise I'll use some of the older code and just implement the hack version. Thanks everyone who worked so hard to get this sorted out!

#112: Kevin, are you able to work on this?

#106: @David_Rothstein, can you give feedback on "My 2c: backport everything. I don't mind doing the backport, but I would like to know if backporting the whole patch is acceptable."

Making a contrib module to fix seems like overkill in this case.
Better to fix it in core.
But, if feeling strongly that it should be in contrib... maybe it could be part of a module multilingual sites are probably already using and we could work with such a maintainer to incorporate into an update to their module?

At the moment the priority was on getting the site up and running so I've implemented the hacked version. I have started playing around with the backport but honestly have not had much time.

Preferably the whole thing would be backported but if that can't happen then at least the hard coded zh-cn, zh-tw, and zh-hk should be done.

#106: @David_Rothstein, can you give feedback on "My 2c: backport everything. I don't mind doing the backport, but I would like to know if backporting the whole patch is acceptable."

I don't see a problem backporting the whole thing, but I also think it's going to require a lot of testing and review to make sure it's working correctly (and mostly bug-free) before we add a new user interface like this in the middle of a stable release. (It's effectively the equivalent of adding a new module, even if the code doesn't live in its own module.)

So I would think it would be a lot quicker just to fix the Chinese issue directly and deal with the new user interface elsewhere (or later)?... but whichever works is OK.

Straight backport

New (working) patch, update function added, tests fixed

I tested this and it works beautifully in D7 too. I just fixed some minor stuff that in the D8 patch was already correct.

+++ b/includes/locale.inc
@@ -118,6 +118,14 @@ function locale_language_from_interface() {
+ * The algorithm works as follows:

Missing empty line above.

+++ b/modules/locale/locale.admin.inc
@@ -1436,3 +1436,201 @@ function locale_date_format_reset_form_submit($form, &$form_state) {
+

Surplus empty line.

As per @David's request above other peole are strongly encouraged to test this.

Rerolled after the latest commits.

Back to RTBC.

Again, as per @David's request above other people are strongly encouraged to test this before and after commit.

It's been a month and no more reviews so I looked at this for a little bit. This is not a complete review though.

Code:

1. +function language_negotiation_configure_browser_form($form, &$form_state) {
   +  $form = array();
   

   Why is this destroying the $form array that was passed in?

2. The new items in hook_menu() appear to have been added to the wrong section (all the existing menu items are grouped by functionality, but this one got added underneath a section with a code comment that says "Localize date formats" which makes no sense).
3. Several places in the patch use array_key_exists() rather than isset()... is there a reason for that?

4. +  $form['new_mapping']['drupal_langcode'] = array(
   +    '#type' => 'select',
   +    '#title' => t('Drupal language'),
   +    '#options' => $language_options,
   +    '#default_value' => '',
   +  );
   

   I can't figure out why the default value is an empty string - that's not one of the options, is it? Just leave it off if the idea is to have no default value.

User interface:

1. This patch uses "Drupal language" as a form element label and other references to the word "Drupal" in the UI... generally we avoid that (see "Wording" section of http://drupal.org/node/604342). I think having "Drupal" appear in help text sometimes is probably unavoidable and not so bad, but I'm not sure I've ever seen it used extensively in form labels like it is being used here.
2. Editing an existing row and leaving the language code blank gave me a form error with no error message when I submitted. That's an edge case, but it was pretty weird.
3. Saving the form sends me back to admin/config/regional/language/configure with no confirmation message, rather than leaving me on the same page (and displaying a confirmation message) like the other language detection configuration pages do... that's inconsistent and also left me wondering a bit if my changes were actually saved (though they in fact were).
4. On the delete confirmation form, the breadcrumbs are messed up (they just say "Home") - I think that's because this was defined as MENU_CALLBACK rather than MENU_VISIBLE_IN_BREADCRUMB. Why?
5. The delete confirmation form has a completely blank description, which is pretty nonstandard. I think that's because an empty string was explicitly passed here:

   +  return confirm_form($form, $question, $path, '');
   

   Any reason?

I stopped reviewing after that - I think this patch needs more review and testing before it's ready to be included in a stable release. (And maybe a real usability review too - from someone who knows more about that than me :)

In terms of timing, this patch adds lots of new translatable strings, so I don't think we can slip it in right before a release. The next D7 bugfix release will probably either be February 6 or March 6 (not really sure yet)... if this issue is completely ready by early February it could still get into a March release, I think (or whichever one comes after that if we release in February). Obviously a more limited initial patch that just dealt with the Chinese language issue (and no user interface changes) still would have a chance to get in as soon as it's ready, but it's up to you all :)

adding steps to test will help get more people to test this, and that's pretty important for this issue.

#125 This is a straight backport from Drupal 8, using the same code and UI language

Code:
1/ fixed
2/ fixed
3/ straight backport
4/ fixed

UI:
1/ straight backport
2/ you're right, the field is set as required, so should show an error message, but not sure if this is related to this patch.
3/ changed, you now stay on the same form
4/ fixed
5/ fixed

#126Testing is covered by the test framework, not sure what other testing you would like to see.

The easiest way to test this is using http://simplytest.me/project/drupal/7.x
Open advanced options, click 'Add a patch' and paste http://drupal.org/files/i365615-127.patch in the field
Hit launch sandbox
Once installed, log in and enable the locale module
Go to admin/config/regional/language and add a new language
Go to admin/config/regional/language/configure/browser

@attiks I'm not sure that covers the issue for chinese readers...

when we add a new language, we should pick chinese?
what would be the steps to set chinese as a browser language, and then ... make content, and view it?

Updated to use issue summary template.

Screenshot of latest D7 UI. Note fieldset is collapsed by default.

Updated with embedded image.

Adds link to D8 screenshot

Screenshot of 7.x entry point before patch is applied:

Screenshot of 7.x entry point after patch is applied:

@David_Rothstein, in #117 you said:

I don't see a problem backporting the whole thing, but I also think it's going to require a lot of testing and review to make sure it's working correctly (and mostly bug-free) before we add a new user interface like this in the middle of a stable release.

Can you provide any details about what to check for?

Here are some really basic steps to test for issues on existing sites:

1) Set up a site with language settings
2) Confirm that language detection does not work correctly for Simplified Chinese in Firefox
3) Apply patch
4) Update site
5) Confirm that language detection is working correctly
6) Check for any problems with existing language settings

Adds D7 UI screenshot

added remaining task for improving steps to test, and made it a list

Re #127, yeah, we can probably let array_key_exists() slide to another issue, but I don't think violating Drupal's UI guidelines is something we should introduce in a stable release and then fix later. Some of this might need to be fixed in Drupal 8 too (I didn't check).

@pameeela, no specific details that I can think of... what I did in #125 was just play around with all the options on the new UI and tried to find ways it broke or was inconsistent. Basically, imagine that it's going to be released to hundreds of thousands of production websites soon after it's committed, and test accordingly :)

OK. next steps here are to address at least the ui points of #125

Alternative UI, feedback needed:

• Separate edit screen
• 'Drupal language' replaced by 'Site language'
• 'Edit / Delete' replaced with 'edit / delete' to be conform with other screens

@David_Rothstein:

OK makes sense. YesCT mentioned testing whether it breaks anything on a site that is using one of the languages affected (such as Simplified Chinese), and how to make sure it wouldn't interfere with the existing detection settings, so I was wondering about this as well. Are there steps to test the browser detection before and after? Or is this not a major concern?

In general #136 makes sense to me (and good catch on the capitalization issue). I don't know about the separate edit screen, though... I wouldn't say that's a blocker for D7 backport or anything, and from what I can see that issue was discussed for a long time above before the final UI here (without the separate edit screen) was decided on, so it seems OK as is.

#137, that sounds like a good test to me. It doesn't look to me like the functional code in this patch has a chance to interfere with the other language detection settings (because it's modifying code that's already there rather than changing when it runs) but wouldn't hurt to check that anyway. Maybe somone else has a better idea of how exactly to test the Chinese issues? My basic understanding of that is (1) broken before patch, (2) works sensibly after applying the patch, (3) works however you configure it after changing the configuration via the new UI.... but it's pretty simplistic :)

New patch:
'Drupal language' replaced by 'Site language'
'Delete' replaced with 'delete' to be conform with other screens

New screenshot with updated UI:

I've had a play with the UI and seems to address all of the concerns. But does the actual browser detection need testing still? If so what are the steps for this?

I'll be writing steps to reproduce this issue by or during the 2013-03-08 Wednesday 16:00 - 18:00 UTC Core Mentoring Hours. That should make it easier to get more people to test this.

actually, operations are being capitalized because it makes translation easier (see #421118: [Meta] Standardize capitalization on actions)
that is clear for drupal 8. I'm not sure about what to do in 7 here.

Adds link to before & after screenshots and updates wording throughout

I updated the Issue Summary with Steps to reproduce for Firefox. The steps go up through applying the patch in #139. I'll add the steps AFTER the patch has been applied a bit later, unless someone else gets to it first.

I also added two more Remaining Tasks. Does anyone know if selecting "Simplified Chinese" uses ALL the various Simplified Chinese encodings, or just certain ones? If no one here knows, I'll ask around in Mozilla's IRC channels. That's something I'd like to know anyway, as I use a decent amount of Chinese text.

Transition, the steps you've added do not change the Accept-Language header that Firefox sends, if that's what you're thinking; instead, it will re-interpret the page in the current window with the selected encodings if the server or page lied about or didn't tell the browser what encoding the page was in and Firefox guessed incorrectly.

To change how Firefox sends Accept-Language headers, go to Preferences, the Content tab, then the "Choose…" button by "Choose your preferred language for displaying pages." For Safari, I think it uses languages you've set as preferred in System Preferences somehow - I can't recall the specifics.

1) Set up a Drupal site with Locale and Content translation module.
2) Config Drupal site with multiple languages with Chinese Simplified, Chinese Traditional, English(Default)

3) Config language detection and enable "Browser" Detection method(determine the language from the browser's language settings).

4) Change how Firefox sends Accept-Language headers. Go to Preferences, the Content tab, then the "Choose…" button by "Choose your preferred language for displaying pages."

5) Confirm that language detection is working correctly with English Accept-Language headers.

Request Headers:
Accept-Language: en-us,en;q=0.5

Response Headers:
HTTP/1.1 301 Moved Permanently
Location: http://www.lixiphp.com/en

6) Confirm that language detection is working correctly with Chinse Simplified Accept-Language headers.

Request Headers:
Accept-Language: zh-cn,en-us;q=0.7,en;q=0.3

Response Headers:
HTTP/1.1 301 Moved Permanently
Location: http://www.lixiphp.com/zh-hans

7) Confirm that language detection does not work correctly with Chinese Traditional Accept-Language headers. which default go to zh-hans.

Request Headers:
Accept-Language: zh-tw,en-us;q=0.7,en;q=0.3

Response Headers:
HTTP/1.1 301 Moved Permanently
Location: http://www.lixiphp.com/zh-hans

Get the browser detection bugs from zh-tw Accept-Language header.

test site: http://www.lixiphp.com

8) Apply patch
9) Update site with Accept-Language header.
10) Confirm that language detection is working correctly with Chinese Traditional Accept-Language headers.

Request Headers:
Accept-Language: zh-tw,en-us;q=0.7,en;q=0.3

Response Headers:
HTTP/1.1 301 Moved Permanently
Location: http://www.lixiphp.com/zh-hant

@attiks the patch works well, nice work.
@YesCT finish the patch review, reproduce the bugs and solved by patch from #128.

FWIW, rather than reconfiguring browsers to send proper headers, you may find it easier to use cURL for testing.

curl -I -H 'Accept-Language: zh-tw' http://example.com/

-I says "just show me the response's HTTP headers, not the response body" (this actually causes curl to make a HTTP HEAD request instead of GET, which some misconfigured web servers may not handle correctly, but in most cases it should work), and -H says "send the following outgoing HTTP header."

Can somebody mark this RTBC?

#139: i365615-139.patch queued for re-testing.

I'll try to reroll this.

during http://drupal.org/patch/reroll

$ git rebase 7.x
First, rewinding head to replay your work on top of it...
Applying: 139
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging modules/locale/locale.test

So here it is. No interdiff... just a reroll

@YesCT thanks for the re roll, back to TRBC

#153: i365615-153.patch queued for re-testing.

#153: i365615-153.patch queued for re-testing.

Thanks for the continued work on this. I spent more time with this and gave it a complete review. I found a number of issues still, though, including some that are definitely commit blockers.

Here's the list (in rough priority order):

1.  /**
   + * Set default mappings for Chinese languages.
   + */
   +function locale_update_7006() {
   +  language_set_browser_drupal_langcode_mappings(array(
   

   This will cause a fatal error if the update is run while the module is disabled (since the API function won't be loaded). The function is a simple wrapper around variable_set(), so I'd suggest just calling variable_set() directly.

2. @@ -161,9 +181,23 @@ function locale_language_from_browser($languages) {
   .........
   +      // Add the generic langcode, but make sure it has a lower qvalue as the
   +      // more specific one, so the more specific one gets selected if it's
   +      // defined by both the browser and Drupal.
   +      $browser_langcodes[$generic_tag] = $qvalue - 0.1;
   .........
   @@ -1690,8 +1708,8 @@ class LocaleBrowserDetectionTest extends DrupalUnitTestCase {
          'en-US,en,fr-CA,fr,es-MX' => 'en',
          'fr,en' => 'en',
          'en,fr' => 'en',
   -      'en-US,fr' => 'en',
   -      'fr,en-US' => 'en',
   +      'en-US,fr' => 'en-US',
   +      'fr,en-US' => 'en-US',
   

   While this looks like it makes sense, it's a behavior change for existing sites and I'm trying to figure out the impact. After reading comments #19 through #28, it sounds like the biggest change is a situation like this:

   Browser send en-us, fr and Drupal defines en, fr => use fr // exact match

   Whereas previously, English would be used? Is that a concern - do we have to worry about sites starting to report in the issue queue that people who are used to see English are now seeing other languages instead and don't know why, and/or the site needs to add "en-us" as a custom language to get around it? I'm having trouble evaluating the impact of making this change in a stable release even if it was deemed a good idea for Drupal 8.

3. +    $form['mappings'][$browser_langcode] = array(
   +      'browser_langcode' => array(
   +        '#type' => 'textfield',
   +        '#default_value' => $browser_langcode,
   +        '#size' => 20,
   +        '#required' => TRUE,
   +      ),
   +      'drupal_langcode' => array(
   +        '#type' => 'select',
   +        '#options' => $language_options,
   +        '#default_value' => $drupal_langcode,
   +        '#required' => TRUE,
   +      ),
   +    );
   

   These form elements have no titles, which is bad for accessibility. (This is also most likely the cause of the issue I found in my previous review: "Editing an existing row and leaving the language code blank gave me a form error with no error message when I submitted.")

   I think you can just add a #title which is the same as the column name, but then also set #title_display to 'invisible' and it should work fine.

4. +function language_negotiation_configure_browser_form($form, &$form_state) {
   +  // Initialize a language list to the ones available, including English.
   +  $languages = language_list();
   +
   +  $existing_languages = array();
   +  foreach ($languages as $langcode => $language) {
   +    $existing_languages[$langcode] = $language->name;
   +  }
   +
   +  // If we have no languages available, present the list of predefined languages
   +  // only. If we do have already added languages, set up two option groups with
   +  // the list of existing and then predefined languages.
   +  if (empty($existing_languages)) {
   +    $language_options = language_list();
   +    $default = key($language_options);
   +  }
   

   This logic doesn't make sense to me - $existing_languages will only be empty if language_list() is empty too, right? So the code in the if() statement looks broken. Also, at least with Drupal core, is it even possible for the language list to be empty? (I always have at least "English" in there.) But maybe it's possible with contrib.

5. In the same area of the code as above:

   +  if (empty($existing_languages)) {
   +    $language_options = language_list();
   +    $default = key($language_options);
   +  }
   +  else {
   +    $default = key($existing_languages);
   +    $language_options = array(
   

   $default is defined but doesn't appear to ever be used.

6. +  $data = $form_state['values']['new_mapping'];
   +  if (!empty($data['browser_langcode'])) {
   +    // Make sure browser_langcode is unique.
   +    if (array_key_exists($data['browser_langcode'], $unique_values)) {
   +      form_set_error('mappings][' . $key . '][browser_langcode', t('Browser language codes must be unique.'));
   +    }
   +    elseif (preg_match('/[^a-z\-]/', $data['browser_langcode'])) {
   +      form_set_error('mappings][' . $key . '][browser_langcode', t('Browser language codes can only contain lowercase letters and a hyphen(-).'));
   

   $key is the wrong variable here (and results in the error message being set on the wrong form element if there's a validation error when adding a new mapping).

7. +    case 'admin/config/regional/language/configure/browser':
   +      $output = '<p>' . t('Browsers use different language codes to refer to the same languages. You can add and edit mappings from browser language codes to the <a href="@configure-languages">languages used by Drupal</a>.', array('@configure-languages' => url('admin/config/regional/language'))) . '</p>';
   +      return $output;
   

   This help text seems a bit incomplete... as written, it almost implies that Drupal won't recognize a browser language if it isn't in this table. I was a little confused by that (at least until I read the rest of the code). Perhaps it needs to state that this table is for custom mappings only, and that Drupal will make a best effort to map any language code defined by the browser even if it's not listed here?

8. +function theme_language_negotiation_configure_browser_form_table($variables) {
   +  $form = $variables['form'];
   +  $rows = array();
   +  $link_attributes = array(
   +    'attributes' => array(
   +      'class' => array('image-style-link'),
   +    ),
   +  );
   

   "image-style-link" definitely doesn't look like the correct class here...

9. +  $output = theme('table', array('header' => $header, 'rows' => $rows, 'attributes' => array('id' => 'lang-neg-browser')));
   

   I think this needs 'empty' text for when the table is empty, to match the pattern used elsewhere in Drupal. Something like "No browser language mappings available."

   Also (minor): The abbreviation in the HTML ID looks a little odd to me (I thought we tried to avoid abbreviations)... would "language-negotiation-browser" be a problem?

10. Related to my earlier review: There's still no confirmation message whatsoever when you submit the form? (All the other language detection configuration forms do "The configuration options have been saved."

11. +class LocaleBrowserDetectionTest extends DrupalWebTestCase {
    +
    +  public static $modules = array('locale');
    

    This looks like Drupal 8 code which slipped into the Drupal 7 backport.

12. +/**
    + * Browser language negotiation form validation.
    + */
    +function language_negotiation_configure_browser_form_validate($form, &$form_state) {
    

    Here (and in the other form definition functions) the API docs seem to be missing several things; see the "Form-generating functions" section of https://drupal.org/coding-standards/docs#functions.

Also, I think we may be getting into the territory where there are too many fixes being made for Drupal 7 only... The normal procedure is to fix issues in Drupal 8 first before backporting. (If it's only a few fixes, I'd let them slip and forward-port to Drupal 8 afterwards, given that this is a big patch, but it's getting to be a lot of them.)

To help facilitate that, here's an interdiff between the Drupal 7 patches in #123 and #153... As far as I can see, most of these things aren't in Drupal 8 yet but would need to be. (As well as some of the new issues I mentioned in my review above, and maybe some other people's reviews too?)

#157 I addressed all comments

Regarding "Browser send en-us, fr and Drupal defines en, fr => use fr // exact match": most browser will add en automatically in this case (checked on chrome and ff)

Back to D7 for testbot, change it back later

#158 So we need to fix it in D8 first?

the new patch for d7 has come back green from the testbot. (noting that in case we retest it later and lose that info)

oops. Didn't mean to keep this assigned to me. Unassigning.

Yeah, looks like the improvements that are needed should be done to in d8, and then can come back here to do it all in d7.

New patch for D8, contains:

• Feedback from #157
• Changes done as described in #158
• Patch from #1784380: Remove zh-cht and zh-chs from language.mappings.yml

grrrr, typo

All right, code changes look good. I don't think those two language codes will damage the site if they keep being there, but its fine. All other things are straightforward cleanups.

Most of these changes look completely unrelated to the Chinese support, why are they all in here?

@catch: the Chinese support is already committed above. The code style, etc. issues found by David Rothstein can be forked into another issue, if that is preferred, but the whole patch is about cleaning up what was already committed, so it can be more directly committed to Drupal 7. Sounds like opening yet another issue for the minor(er) cleanups and leave this for the not so minor cleanups may not be very effective at the end?!

These aren't all minor cleanups, actually; they include an accessibility fix and fixes for some very non-standard UI patterns.

Reviewing #163:

1. A few things from #158 that look relevant for D8 also are still missing from the D8 patch:
   • Removal of $form_state['redirect'] in language_negotiation_configure_browser_form_submit() to fix the confusing redirect after submit.
   • Incorrect breadcrumbs due to MENU_CALLBACK rather than MENU_VISIBLE_IN_BREADCRUMB on admin/config/regional/language/configure/browser/delete/%.
   • Removal of $form = array() at the top of language_negotiation_configure_browser_form().
   • Removal of '#default_value' => '' on the 'new_mapping' select element at the bottom of language_negotiation_configure_browser_form().

2. @@ -768,7 +782,6 @@ function language_negotiation_configure_browser_delete_form_submit($form, &$form
        unset($mappings[$browser_langcode]);
        language_set_browser_drupal_langcode_mappings($mappings);
      }
   -  $form_state['redirect'] = 'admin/config/regional/language/detection/browser';
   

   This isn't right; it results in the user remaining on the delete confirmation form after confirming the deletion! (Actually, that probably explains the first point above also; I think $form_state['redirect'] was removed from this form but should have been removed from the other instead.)

3. +    drupal_set_message('The configuration options have been saved.');
   

   Missing t().

4. -      $output = '<p>' . t('Browsers use different language codes to refer to the same languages. You can add and edit mappings from browser language codes to the <a href="@configure-languages">languages used by Drupal</a>.', array('@configure-languages' => url('admin/config/regional/language'))) . '</p>';
   +      $output = '<p>' . t('Browsers use different language codes to refer to the same languages. You can add and edit mappings from browser language codes to the <a href="@configure-languages">languages used by Drupal</a>, without a custom mapping Drupal will make a best effort to map it.', array('@configure-languages' => url('admin/config/regional/language'))) . '</p>';
   

   This is grammatically incorrect. How about something like:

   Browsers use different language codes to refer to the same languages. Drupal will make a best effort to determine the correct language based on the code that the browser sends, but for finer control you can add and edit mappings from browser language codes to the <a href="@configure-languages">languages used by Drupal</a>.

Also, this issue hasn't been only about the Chinese support for a while, actually?

Needs reroll

Re-roll.

Addressing issues in #168.

This patch is working properly and there is no need for rerolling the stuff