Problem/Motivation
Browsers understand language codes differently to Drupal. The issue was originally created to because this causes serious problems for Simplified Chinese. IE and Firefox will send 'zh-cn', but Drupal understands this language as 'zh-hans'. However this applies to other languages as well.
Proposed resolution
A mapping system to map external language codes to internal language codes and default mappings for some common situations.
This has been committed and pushed to 8.x (in #365615-99: Language detection not working correctly for most Chinese readers (and add a user interface for all browser language mappings), #365615-103: Language detection not working correctly for most Chinese readers (and add a user interface for all browser language mappings) and #365615-231: Language detection not working correctly for most Chinese readers (and add a user interface for all browser language mappings)) and is being backported to 7.x.
Steps to reproduce
Instructions for Firefox 19.0.2 running on OS 10.7.5
From Firefox, go to the following Menu progression:
View > Character Encoding > Auto-Detect > Simplified Chinese
Make sure "Simplified Chinese" has a check next to it.
There are multiple Simplified Chinese encodings. The step above SHOULD detect them all (that's still being researched). To test the individual Simplified Chinese encodings, take the following menu progression:
View > Character Encoding > More Encodings > East Asian
This will lead you to a list of multiple encodings for Simplified Chinese, along with other East Asian languages. Choose the encoding you wish to test.
Once you've adjusted your language detections settings, be sure you have an up-to-date install of Drupal 7.x. See the Drupal Core Git instructions section on 'Getting ready to create or apply patches' for details on updating your installation.
Once your install is updated, log in and be sure the "Locale" module is active. Go to "DrupalSite/admin/modules" OR from the administration menu at the top of the screen, click "Modules". The Locale module is part of the Core group. Module names are listed in alphabetic order. Be sure the box next to "Locale" is checked, then scroll to the bottom of the page and click the "Save Configuration" button.
Next, go to your Drupal 7.x installation and go to "admin/config/regional/language/configure". Here, be sure the "Browser" detection method is enabled and drag it to the top of the list of detection methods. Click the "Save Settings" button at the bottom of the screen. Nothing should happen.
Next, apply the patch from comment 139.
NEXT: Description of what should happen after the patch is applied.
Remaining tasks
Reroll. Review. Commit.
User interface changes
A new user interface is added to edit mappings of external vs. internal language codes.
D8 screenshot in #92
D7 screenshot in #131
Before and after screenshots of entry point in D7 in #132
Original report by yang_yi_cn
language.inc, line 95, in function language_from_browser() :
change from
foreach ($browser_langs as $langcode => $q) {
if (isset($languages['1'][$langcode])) {
return $languages['1'][$langcode];
}
}
to
foreach ($browser_langs as $langcode => $q) {
if (($langcode == 'zh-cn') && isset($languages['1']['zh-hans'])) {
return $languages['1']['zh-hans'];
}
if (isset($languages['1'][$langcode])) {
return $languages['1'][$langcode];
}
}
As you can see, for Simplified Chinese langugage, both IE and Firefox will send 'zh-cn' as the preferred language code, which is different from Drupal's 'zh-hans' code, thus the browser detection will not work for Simplified Chinese.
Maybe there's also some other languages having this issue, but this is what I got now.
Comment | File | Size | Author |
---|---|---|---|
#228 | i365615-228.patch | 8.97 KB | Gábor Hojtsy |
#227 | interdiff.txt | 3.36 KB | Gábor Hojtsy |
#227 | i365615-227.patch | 8.94 KB | Gábor Hojtsy |
#220 | interdiff.365615.202.220.txt | 6.11 KB | YesCT |
#220 | i365615-220.patch | 8.94 KB | YesCT |
Comments
Comment #1
kcouch CreditAttribution: kcouch commentedI 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?
Comment #2
PasqualleFirefox 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.
Comment #3
shenzhuxi CreditAttribution: shenzhuxi commentedJust change ""zh-hans" => array("Chinese, Simplified", "简体中文")," into "zh-cn" => array("Chinese, Simplified", "简体中文")," in locale.inc.
It'll be crazy to wait for browsers.
Comment #4
cshuangtw CreditAttribution: cshuangtw commentedI 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.
Comment #5
juhovh CreditAttribution: juhovh commentedI 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:
Now it seems to work with all Chinese variations quite nicely, at least I'm happy. Thanks for finding the correct piece of code.
Comment #6
PasqualleComment #7
GiorgosKis n't this issue a duplicate of #221712: locale_language_from_browser() doesn't parse language tags correctly, has a broken logic
Comment #8
juhovh CreditAttribution: juhovh commentedThis 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.
Comment #9
plachBugs are fixed in the development version first, backported then.
No patch here.
Comment #10
smokrisAttached 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.
Comment #11
Garrett Albright CreditAttribution: Garrett Albright commented"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.
Comment #12
WormFood CreditAttribution: WormFood commentedI 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.
Comment #13
WormFood CreditAttribution: WormFood commentedDrupal 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:
to (insert 6 lines)
Do not change "zh" to "zh-hans", (like the D6 patch does) because it will (probably) cause problems.
Comment #14
attiks CreditAttribution: attiks commentedComment #15
attiks CreditAttribution: attiks commentedPatch against D8 fixing this, the logic is fixed as well.
Comment #16
attiks CreditAttribution: attiks commentedNew patch without colors
Comment #17
lazysoundsystem CreditAttribution: lazysoundsystem commentedI've checked this and it looks good.
Comment #18
penyaskitoRerolled because of #1739994: Use the Language class universally instead of stdObj instances.
Relevant changed part:
Comment #19
Gábor HojtsyWhy 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?
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?
Comment #20
attiks CreditAttribution: attiks commentedGabor, 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 languageen-us
if should be the one used in favor ofen
. 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
Comment #21
Gábor Hojtsy@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!
Comment #22
attiks CreditAttribution: attiks commentedThe 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.
Comment #23
WormFood CreditAttribution: WormFood commentedFor 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';
}
Comment #24
attiks CreditAttribution: attiks commented@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 ;-)
Comment #25
WormFood CreditAttribution: WormFood commented@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.
Comment #26
attiks CreditAttribution: attiks commentedNew patch, I also added zh-chs and zh-cht
@WormFood: thanks for the explanation, good to know
Comment #27
Gábor HojtsyWell, 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. :/
Comment #28
attiks CreditAttribution: attiks commented@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
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe 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.
Comment #30
Gábor Hojtsy@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.
Comment #31
shenzhuxi CreditAttribution: shenzhuxi commentedI think this issue has the same condition http://drupal.org/node/870350
Comment #32
attiks CreditAttribution: attiks commented@Damien thanks for reviewing, new patch attached.
Comment #34
attiks CreditAttribution: attiks commented#32: i365615-chinese.patch queued for re-testing.
Comment #35
Damien Tournoud CreditAttribution: Damien Tournoud commentedPlease elaborate more on why this is needed. Especially, mention that browsers are sending a langcode that is not a valid IETF language tag.
Comment #36
attiks CreditAttribution: attiks commentedWhat about "Handle Chinese so it maps right, some browsers are sending non standard langcodes, we map them to the right ones."
Comment #37
attiks CreditAttribution: attiks commentedI discussed this a bit with damien in IRC
http://www.iana.org/assignments/language-subtag-registry defines the following
So we need mappings as follows:
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
Comment #39
attiks CreditAttribution: attiks commented#37: i365615-chinese.patch queued for re-testing.
Comment #41
attiks CreditAttribution: attiks commented#37: i365615-chinese.patch queued for re-testing.
Comment #43
attiks CreditAttribution: attiks commented#37: i365615-chinese.patch queued for re-testing.
Comment #44
Gábor HojtsyI 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 :)
Comment #45
attiks CreditAttribution: attiks commentedAnybody 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.*?
Comment #46
attiks CreditAttribution: attiks commentedI had a quick talk with heyrocker and we need to add an UI for this
Comment #47
WormFood CreditAttribution: WormFood commentedI 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.
Comment #48
attiks CreditAttribution: attiks commentedWe can add a configure link at admin/config/regional/language/detection and just show everything in a huge textarea
OK?
Comment #49
attiks CreditAttribution: attiks commentedWhat 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
It should be not a huge problem, the only difference is that config will get replaced by variable_get/variable_set
Comment #50
WormFood CreditAttribution: WormFood commentedNot 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.
Comment #51
attiks CreditAttribution: attiks commentedI 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
Comment #52
attiks CreditAttribution: attiks commentedIn Drupal 7 I can do dates like 2012年03月09日 at en/admin/config/regional/date-time/locale
Comment #53
attiks CreditAttribution: attiks commentednew patch including 'proper' admin interface
Comment #54
attiks CreditAttribution: attiks commentedAnd a screenshot
Comment #56
attiks CreditAttribution: attiks commented#53: i365615-chinese.patch queued for re-testing.
Comment #57
attiks CreditAttribution: attiks commented@wormfood, there's an issue about the date problem, see #1345758: META: Provide locale (regional) formats framework for automated translation of non textual data
Comment #58
penyaskitoUse single quotes.
Requires "@ingroup themeable" phpdoc.
We need the question mark.
Clever, backporting made easy :-)
Do we need this chinese specific code now that we have the mapping configurable?
Comment #59
penyaskitolanguage.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.
Comment #60
attiks CreditAttribution: attiks commentedNew 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.
Comment #61
Gábor HojtsyGood 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:
Dot at end of line.
Delete a mapping form? A mapping submit?!
Should have a newline at end of file.
Comment #62
attiks CreditAttribution: attiks commentedcomments fixed, tests added.
Comment #63
attiks CreditAttribution: attiks commentedTests added, tag removed
Comment #64
penyaskitoShould we say "Form for deleting a browser language negotiation mapping." and "Submit handler for deleting a browser language negotiation mapping."?
Typo. Should be testLanguageFromBrowser.
It should be the last review before RTBC :-)
Thanks attiks!
Comment #65
attiks CreditAttribution: attiks commentedshould be ok
Comment #66
penyaskitoIf tests pass, it's fine for me. :)
Comment #67
penyaskitoRTBC
Comment #68
webchickFirst 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.
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?
Can you add a sentence that explains why I would want to / how I know I need to do that?
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.
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.
(nitpick) Periods at the end of these sentences.
Just asking, but is there a way to consolidate this into one if statement somehow?
Can we not just make these required fields then?
(nitpick) Capitalize "Chinese."
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?
Let's get a one-liner of PHPDoc above this to explain what it's doing.
Is there a reason we do ! here rather than @? This is just plain-text, right?
Comment #69
webchickOh 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
Comment #70
attiks CreditAttribution: attiks commentedThanks for reviewing, new patch attached:
Comment #71
Gábor Hojtsy@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.
Comment #72
Gábor HojtsyMoving back to RTBC on those grounds and that @attiks took care of the rest of the concerns.
Comment #73
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #74
attiks CreditAttribution: attiks commentedDescription 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, ...
Comment #75
attiks CreditAttribution: attiks commented[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."
Comment #76
attiks CreditAttribution: attiks commentedmoved help to hook_help and changed the wording
Comment #77
Gábor HojtsyEven resolved Bojhan's concerns now. :)
Comment #78
Dries CreditAttribution: Dries commentedThis doesn't sound like proper English. Needs a bit of tweaking. (Update: DrEditor cut off the next line it seems.)
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.
Comment #79
attiks CreditAttribution: attiks commentedThanks 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.
Comment #80
Gábor HojtsyI 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.
Comment #81
Gábor HojtsyAlso, 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.
Comment #82
attiks CreditAttribution: attiks commented#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?
Comment #83
Gábor HojtsyIf 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?
Comment #84
attiks CreditAttribution: attiks commented@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:
Personally I prefer option 1, but I'm fine with 2.
Comment #85
Gábor HojtsyIf 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?
Comment #86
attiks CreditAttribution: attiks commentedif 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)
Comment #87
attiks CreditAttribution: attiks commentedNew patch after discussion in irc
Comment #88
Gábor HojtsyMuch 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.
Comment #89
attiks CreditAttribution: attiks commentedno idea what I was thinking, but this one should be better.
Comment #90
Bojhan CreditAttribution: Bojhan commentedWhat is that bottom row?
Comment #91
attiks CreditAttribution: attiks commentedTo add a new mapping
Comment #92
attiks CreditAttribution: attiks commentedScreenie of new UI as briefly discussed with Bojhan
EDIT: Fieldset is collapsed by default.
Comment #93
attiks CreditAttribution: attiks commentedPatch implementing the new UI
Comment #94
Gábor HojtsyLooks good to me., resolves Dries' concern, resolves Bojhan's concerns :)
Comment #95
sunGreat work all! :)
I reviewed this patch in some more detail and found the following issues:
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.
AFAICS, the #type container is not needed here and can be removed.
Normally, this is specified in the form submission handler. Any particular reason for why it's specified in the constructor?
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.
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.
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.
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.
Comment #96
attiks CreditAttribution: attiks commentedThanks 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.
Comment #97
sunThanks! :)
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.)
Comment #98
attiks CreditAttribution: attiks commented@sun if you can create a follow-up issue for the minor stuff, I'll fix them
Comment #99
webchickReally 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.
Comment #100
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedTypo in the UI text.
Comment #101
Gábor HojtsyAlso added a short changelog entry, since we will not be able to track this easily if it falls back into Drupal 7.
Comment #102
attiks CreditAttribution: attiks commentedI found 2 more 'langauges'
Comment #103
webchickOops. :P
Committed and pushed to 8.x. Back to 7.x.
Comment #104
Gábor HojtsyRemoving 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.
Comment #105
Gábor HojtsyAdde 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.
Comment #106
David_Rothstein CreditAttribution: David_Rothstein commentedI 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 :)
Comment #107
attiks CreditAttribution: attiks commented#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.
Comment #108
attiks CreditAttribution: attiks commentedNew follow-up issue created #1784380: Remove zh-cht and zh-chs from language.mappings.yml
Comment #109
Gábor HojtsyI think it would be fine/safe to backport the whole thing, its just new configuration at a place where we never had config.
Comment #110
WormFood CreditAttribution: WormFood commentedI 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.
Comment #111
WormFood CreditAttribution: WormFood commentedoops, changed the status by mistake (damn GUIs). Changing it back.
Comment #112
Kevin Morse CreditAttribution: Kevin Morse commentedDamn... 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!
Comment #113
attiks CreditAttribution: attiks commented#112: Kevin, are you able to work on this?
Comment #114
attiks CreditAttribution: attiks commented#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."
Comment #115
YesCT CreditAttribution: YesCT commentedMaking 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?
Comment #116
Kevin Morse CreditAttribution: Kevin Morse commentedAt 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.
Comment #117
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #118
attiks CreditAttribution: attiks commentedStraight backport
Comment #119
attiks CreditAttribution: attiks commentedNew (working) patch, update function added, tests fixed
Comment #120
plachI tested this and it works beautifully in D7 too. I just fixed some minor stuff that in the D8 patch was already correct.
Missing empty line above.
Surplus empty line.
Comment #121
plachAs per @David's request above other peole are strongly encouraged to test this.
Comment #123
plachRerolled after the latest commits.
Comment #124
plachBack to RTBC.
Again, as per @David's request above other people are strongly encouraged to test this before and after commit.
Comment #125
David_Rothstein CreditAttribution: David_Rothstein commentedIt'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:
Why is this destroying the $form array that was passed in?
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:
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 :)
Comment #126
YesCT CreditAttribution: YesCT commentedadding steps to test will help get more people to test this, and that's pretty important for this issue.
Comment #127
attiks CreditAttribution: attiks commented#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
Comment #128
attiks CreditAttribution: attiks commented#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
Comment #129
YesCT CreditAttribution: YesCT commented@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?
Comment #129.0
YesCT CreditAttribution: YesCT commentedUpdated to use issue summary template.
Comment #130
pameeela CreditAttribution: pameeela commentedScreenshot of latest D7 UI. Note fieldset is collapsed by default.
Comment #131
pameeela CreditAttribution: pameeela commentedUpdated with embedded image.
Comment #131.0
pameeela CreditAttribution: pameeela commentedAdds link to D8 screenshot
Comment #132
pameeela CreditAttribution: pameeela commentedScreenshot of 7.x entry point before patch is applied:
Screenshot of 7.x entry point after patch is applied:
Comment #133
pameeela CreditAttribution: pameeela commented@David_Rothstein, in #117 you said:
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
Comment #133.0
pameeela CreditAttribution: pameeela commentedAdds D7 UI screenshot
Comment #133.1
YesCT CreditAttribution: YesCT commentedadded remaining task for improving steps to test, and made it a list
Comment #134
David_Rothstein CreditAttribution: David_Rothstein commentedRe #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 :)
Comment #135
YesCT CreditAttribution: YesCT commentedOK. next steps here are to address at least the ui points of #125
Comment #136
attiks CreditAttribution: attiks commentedAlternative UI, feedback needed:
Comment #137
pameeela CreditAttribution: pameeela commented@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?
Comment #138
David_Rothstein CreditAttribution: David_Rothstein commentedIn 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 :)
Comment #139
attiks CreditAttribution: attiks commentedNew patch:
'Drupal language' replaced by 'Site language'
'Delete' replaced with 'delete' to be conform with other screens
Comment #140
pameeela CreditAttribution: pameeela commentedNew 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?
Comment #141
TransitionKojo CreditAttribution: TransitionKojo commentedI'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.
Comment #142
YesCT CreditAttribution: YesCT commentedactually, 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.
Comment #142.0
YesCT CreditAttribution: YesCT commentedAdds link to before & after screenshots and updates wording throughout
Comment #143
TransitionKojo CreditAttribution: TransitionKojo commentedI 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.
Comment #144
Garrett Albright CreditAttribution: Garrett Albright commentedTransition, 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.
Comment #145
lixiphp CreditAttribution: lixiphp commented1) 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
Comment #146
lixiphp CreditAttribution: lixiphp commented8) 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.
Comment #147
Garrett Albright CreditAttribution: Garrett Albright commentedFWIW, 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."Comment #148
attiks CreditAttribution: attiks commentedCan somebody mark this RTBC?
Comment #149
pameeela CreditAttribution: pameeela commentedComment #150
YesCT CreditAttribution: YesCT commented#139: i365615-139.patch queued for re-testing.
Comment #152
YesCT CreditAttribution: YesCT commentedI'll try to reroll this.
Comment #153
YesCT CreditAttribution: YesCT commentedduring 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
Comment #154
attiks CreditAttribution: attiks commented@YesCT thanks for the re roll, back to TRBC
Comment #155
attiks CreditAttribution: attiks commented#153: i365615-153.patch queued for re-testing.
Comment #156
YesCT CreditAttribution: YesCT commented#153: i365615-153.patch queued for re-testing.
Comment #157
David_Rothstein CreditAttribution: David_Rothstein commentedThanks 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):
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.
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:
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.
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.
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.
$default is defined but doesn't appear to ever be used.
$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).
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?
"image-style-link" definitely doesn't look like the correct class here...
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?
This looks like Drupal 8 code which slipped into the Drupal 7 backport.
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.
Comment #158
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, 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?)
Comment #159
attiks CreditAttribution: attiks commented#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?
Comment #160
YesCT CreditAttribution: YesCT commentedthe 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.
Comment #161
attiks CreditAttribution: attiks commentedNew patch for D8, contains:
Comment #163
attiks CreditAttribution: attiks commentedgrrrr, typo
Comment #164
Gábor HojtsyAll 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.
Comment #165
catchMost of these changes look completely unrelated to the Chinese support, why are they all in here?
Comment #166
Gábor Hojtsy@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?!
Comment #167
catchComment #168
David_Rothstein CreditAttribution: David_Rothstein commentedThese aren't all minor cleanups, actually; they include an accessibility fix and fixes for some very non-standard UI patterns.
Reviewing #163:
$form_state['redirect']
in language_negotiation_configure_browser_form_submit() to fix the confusing redirect after submit.$form = array()
at the top of language_negotiation_configure_browser_form().'#default_value' => ''
on the 'new_mapping' select element at the bottom of language_negotiation_configure_browser_form().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.)
Missing t().
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>.
Comment #169
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, this issue hasn't been only about the Chinese support for a while, actually?
Comment #170
jair CreditAttribution: jair commentedNeeds reroll
Comment #171
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll.
Comment #172
Albert Volkman CreditAttribution: Albert Volkman commentedAddressing issues in #168.
Comment #173
Nitesh Sethia CreditAttribution: Nitesh Sethia commentedThis patch is working properly and there is no need for rerolling the stuff
Comment #174
mgifford#172: i365615-172.patch queued for re-testing.
Comment #175
mgifford@David_Rothstein In #168 I'm assuming your suggesting that the accessibility fix should be brought into a separate issue. I'm still not clear what that fix was.
Comment #176.0
(not verified) CreditAttribution: commentedUpdated issue summary with Steps to reproduce in Firefox, UP TO applying the patch in Comment 139; need to finish steps AFTER patch is applied
Comment #177
heddnRe-adding tags.
Comment #178
nikhilsukul CreditAttribution: nikhilsukul commentedComment #179
nikhilsukul CreditAttribution: nikhilsukul commentedComment #180
webwarrior CreditAttribution: webwarrior commentedComment #181
mgifford180: i365615-180.patch queued for re-testing.
Comment #183
mgiffordreroll
Comment #186
mgiffordThis is a pretty important issue!
Comment #187
alexpottThe rerolls from #180 on are missing the point. We need to be making changes to
NegotiationBrowserForm
. Adding language_negotiation_configure_browser_form(), language_negotiation_configure_browser_form_validate(), and language_negotiation_configure_browser_form_submit() back is incorrect.This also means that we're forgetting to manually test patches before uploading them.
Comment #188
Gábor Hojtsy@mgifford: yeah the rolls that include adding back the procedural functions instead of modifying where the code went are not good. We need to modify the actual code not add back one parallel copy.
Comment #189
mgiffordThanks @alexpott & Gábor Hojtsy
As usual I'm trying to nudge ahead issues between other priorities. I do really think that this needs to be resolved before the release of D8. Hopefully someone else can step up and take on modifying the relevant code.
Comment #190
Gábor HojtsyWell, just to make it clear, the fix itself is in Drupal 8, the cleanups suggested are not, but that does not make the D8 feature not work. Looks like this needs to be rolled again from #172.
Comment #191
mgiffordOk, let's see if this works...
Comment #193
mgiffordBloody ";"'s...
Comment #194
Gábor HojtsyComment #196
Gábor HojtsyIt was said multiple times above that zh-chs and zh-cht are so ancient and never actually used by browsers that we should remove the mapping. Note that the mapping does not hurt anyone, I mean we could map 'bacon' to 'en' for whatever we want. Looking for proof of the non-use of these two Chinese language codes, I found that Bing translator in fact documents they support Chinese exactly with these language codes. See http://msdn.microsoft.com/en-us/library/hh456380.aspx As of Feb 2014. So I would refrain from removing them in this patch (or not adding them to Drupal 7). They do not hurt for people who don't use it but they are definitely useful for people who do.
Comment #197
Gábor HojtsyRerolled the patch first.
Comment #198
Gábor HojtsyRolled without the removal of those mappings as per #196.
Comment #199
Gábor HojtsyAlso bring on the D8MI sprint.
Comment #200
YesCT CreditAttribution: YesCT commentedCan we reword this so the word Drupal is not in the UI?
still needs an issue summary update. (could be a bit time consuming, but should be done)
We should also explain in the summary why this is a bug report with no test addition or test changes.
Comment #201
YesCT CreditAttribution: YesCT commentedMaybe instead of
Drupal will make a best effort to determine
A best effort will be made to determine
Comment #202
Gábor HojtsyI think the intention there is to make clear who are the actors. The browser and Drupal. What about
Browsers use different language codes to refer to the same languages. This language mapping list allows to match those to the <a href="!configure-languages">languages used internally</a>.
As for the issue summary, it would be sad to loose the summary for the Drupal 7 change, which is still accurate. I think everyone hoped that the D8 port of the changes would be swiftly committed without much tossing around, especially not for a year. So it would be great to finally let this in D8 and continue backporting to D7.
Comment #203
YesCT CreditAttribution: YesCT commentedThanks. That little summary update helped me remember what was going on here.
I'm going to review this now.
Comment #204
YesCT CreditAttribution: YesCT commentedYeah, this was added by the patch from #96 that was committed in #99 (and then moved via #1480854: Convert operation links to '#type' => 'operations').
I agree with @David_Rothstein in #157 8, that this looks like the wrong class.
I can't find where that might have been copied from (to maybe confirm it might have been a copy and paste error) and I can't think of what it should be instead. I also cant figure out what it was intended to do. And it looks fine without it.
It is limited to this negotiation form: /admin/config/regional/language/detection/browser
So that change, removing that class, looks ok.
without patch, that button:
with patch, that button:
---
next, reviewing the other changes here.
Comment #205
YesCT CreditAttribution: YesCT commented"allows to make" is a little awkward.
and looking into this more,
the feedback that sparked this change from #157 was
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?
I think we lost what David was looking for.
in #168 @David_Rothstein suggested:
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 languages used by Drupal.
I've given it another try, using @Gábor's idea of referring to what drupal does as: internally, and also the new label in the table (site language instead of Drupal language).
Browsers use different language codes to refer to the same languages. Internally, a best effort is made to determine the correct language based on the code that the browser sends. You can add and edit additional mappings from browser language codes to <a href="!configure-languages">site languages</a>.
It might be odd to say ...add and edit additional... but was hoping to convey that these are not the only mappings and people dont have to add a mapping every time they add a language. (for example, I added Afrikaans. and there is no mapping here for a browser code, but it will get mapped by that best guess.)
and here it is in context.
Comment #206
YesCT CreditAttribution: YesCT commentedI tried deleting a mapping to make sure this message showed ok.
It does show.
But since i used the delete button and not the save configuration button, the message was a little weird.
I looked for where we have another similar situation, like a table of things with delete buttons on each and one save for the whole form.
There is one on the language admin page.
It says:
The Albanian (sq) language has been removed.
I think we should do something similar here.
looking in
core/modules/language/src/Form/LanguageDeleteForm.php
----
I'm going to make that change now.
----
... do we also want a log message?
Comment #207
YesCT CreditAttribution: YesCT commentedchanged the message, added a test for it.
also logged it.
ps. why is this test file called *unit* test?
attached a tests only patch.
Also, I was curious if the assert no after this would work since it might not be on the page listing the other browser codes.
So I have a file to check if that fails if the delete doesn't happen (or rather to see if it passes even if the delete doesn't happen).
[edit:]
this is the test I'm wondering fails
// Check that ch-zn no longer exists.
$this->assertNoField('edit-mappings-zh-cn-browser-langcode', 'Chinese browser language code no longer exists.');
Comment #211
YesCT CreditAttribution: YesCT commentedoh, good that fails test does catch the code not being deleted.
let's see if this fixes the test I wrote.
Comment #212
YesCT CreditAttribution: YesCT commentednice. the empty text shows up for an empty table.
Comment #213
YesCT CreditAttribution: YesCT commentedhm.
delete confirm goes back to
admin/config/regional/language/detection/browser
(the table of mappings)
but adding one,
or editing one and using the save configuration button,
goes back to
admin/config/regional/language/detection
(the negotiation and detection page)
that's why the test for adding looks like
it has to do the get to go back to the browser page.
maybe the test should check it goes back to the browser page and then check if the code is found.
in LanguageListTest it does
so changing the tests to assert they return to the place they should, based on the route.
(this will fail on the add and change test, but I'll fix that next)
Comment #215
YesCT CreditAttribution: YesCT commentedgreat that failed just right.
now the fix.
Comment #216
YesCT CreditAttribution: YesCT commentedI see these as labels when I inspect the html of the page. But they do not get read (when I turn on voice over on my Mac in my accessibility settings, in chrome).
So it looks ok to me, but we might want @mgifford or someone to check that the title is doing what we expect.
Comment #217
YesCT CreditAttribution: YesCT commentedreading #168 1. from @David_Rothstein
maybe we dont need to specify the redirect in the submit anyway... yep. worked locally. uploading taking out that redirect.
Comment #218
YesCT CreditAttribution: YesCT commentedOK.
I'm done (at least for now)
remaining:
aside from the small accessibility question about why we have a id on the table and #216 about the #title.
and a review of my changes.
Comment #219
mgifford#1 - Thanks for putting so much into this today! This is great and the screenshots are quite helpful.
I read through the patch. Looks good to me.
Installed with Chinese. Installed with English & then with Simplified Chinese Added.
The id's I think you are referring to are from #864006: Improve table semantics by adding scope or headers/id attributes
Deleting here seems to work as expected with the patch now admin/config/regional/language/detection/browser
Comment #220
YesCT CreditAttribution: YesCT commentedSigh, I know I said I was done, but I went back and read #168 1. again, and I think I found the last things from there.
Also attached an interdiff to 202 for @Gábor Hojtsy
Comment #221
mgiffordOh ya, it seems to work fine for me in VoiceOver & ChromeVox. I think the labels are set up properly.
Comment #222
YesCT CreditAttribution: YesCT commenteddid you mean to take off the d6 tag?
Comment #223
mgifford@YesCT - Ya. I'd love to see the backported, but it takes long enough to get things into d7 and really doesn't make sense to be investing much in d6 these days. I should have been specific about that though.
Comment #224
Gábor Hojtsy@mgifford: Good enough for RTBC?
Comment #225
mgiffordYes, I think so.
Comment #226
alexpottSome minor nits.
How about just $args since this is not only being used with $this->t()
Should all be
$this->t()
- in fact this patch is a regresses some previous conversions.Comment #227
Gábor HojtsyGood finds, indeed. Fixed those. @mgifford: want to do a once-over again? :)
Comment #228
Gábor HojtsyDuh, rolled 227 wrong, so it does not actually contain anything from the interdiff. Here is the good one. This should have been 227.
Comment #230
mgiffordGreat! I looked at the interdiff & compared that with @alexpott's suggestions. They look good to me.
I also installed it again & went through the steps to verify that it worked the same as before.
I'm happy to RTBC this again. Thanks Gábor!
Comment #231
alexpottWhilst I've stopped committing on issue follow-ups since they confuse everything (see the issue summary for an example of how) I'm going to commit this one because of the age and number of comments. This issue addresses a major bug and is allowed per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?. Committed 2095c89 and pushed to 8.0.x. Thanks!
Comment #233
Gábor HojtsyThanks all! I agree committing followups from an issue is confusing, but there was already one D8 followup committed here and the D7 maintainer pushed this back to D8 for followup work.
For the new D7 backport, I collected where were commits on this issue:
#365615-99: Language detection not working correctly for most Chinese readers (and add a user interface for all browser language mappings) (comment 99)
#365615-103: Language detection not working correctly for most Chinese readers (and add a user interface for all browser language mappings) (comment 103)
#365615-231: Language detection not working correctly for most Chinese readers (and add a user interface for all browser language mappings) (comment 231)
The merge of these should be backported to Drupal 7.
Comment #235
aniket_m CreditAttribution: aniket_m commentedHi,
Anybody has an idea on how to load css only for Chinese site in Internet Explorer?
I'm setting up a multi-folder Drupal 7 site for various languages, I'm testing it with various browsers, and they all seem to works fine, except for Internet Explorer for only chinese site.
I tried various solutions, but not succeed in that.
Thanks in advance.
Comment #236
aniket_m CreditAttribution: aniket_m commentedAnd finally I solve my problem for Internet Explorer for only chinese site.
Very strange issue. I disabled JS aggregation and i no longer got the js errors. I then disabled CSS aggregation and the page loads ok. When I aggregate the CSS the aggregated files are empty in IE, hence my site not displaying properly.