A problem was issue after patch for bug 6629 (cookie language sanitization), for language selection. Before the patch for 6629: in C4::Templates::themelanguage, language is taken from: if cookie: lang = C4::Templates::getlanguagecookie else - HTTP_ACCEPT_LANGUAGE (parsed and interpreted) checked against sysprefs 'en' else After the patch: in C4::Templates::themelanguage, language is taken from: lang = C4::Templates::getlanguagecookie if not lang: - HTTP_ACCEPT_LANGUAGE (parsed and interpreted) checked against sysprefs 'en' else But in C4::Templates::getlanguagecookie, language is taken from: - cookie (if exist) - else from: - HTTP_ACCEPT_LANGUAGE (not parsed, so lang could be set to "fr,fr-fr;q=0.8,en-us;q=0.5,en;q=0.3") (which almost always exists) - result is sanitized (so if not cookie: my language is something like "frfr-frq08enusq05enq03", but not installed in koha :-) )
Created attachment 6486 [details] [review] Bug 7282: invalid language selection This patch restore the original behavior. Perhaps getlanguagecookie should be improve to parse and interprete HTTP_ACCEPT_LANGUAGE too ? But duplicate code (in themelanguage) should be stripped. And his name is getlanguagecookie ... so should be only the language from cookie ? if yes, discard HTTP_ACCEPT_LANGUAGE from getlanguagecookie is a solution... So, a more elaborate patch could be done.
Ah yep, good catch, ill test a little more and sign off
Created attachment 6487 [details] [review] Bug 7282: invalid language selection - restore the original behavior: call getlanguagecookie only if the cookie is present Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
> Perhaps getlanguagecookie should be improve to parse and interprete > HTTP_ACCEPT_LANGUAGE too ? But duplicate code (in themelanguage) should be > stripped. > And his name is getlanguagecookie ... so should be only the language from > cookie ? if yes, discard HTTP_ACCEPT_LANGUAGE from getlanguagecookie is a > solution... Yes, there seems to have something wrong here. And for me it doesn't work as it should. This doesn't work: - Clear your browser cookies - Select fr, fr-fr, en languages - Load OPAC main page We should have fr templates selected, based on browser preference. We have English template.
Frederic, FR is activated in the system preferences eh?
(In reply to comment #5) > Frederic, FR is activated in the system preferences eh? Yes.
Created attachment 6488 [details] [review] Proposed patch Bug 7282 Invalid language selection Frère Sébastien Marie refactored patch to ensure that without cookie, language selection is based on browser preferences. To reproduce the bug: - Activate en and fr-FR for OPAC - Clear your browser cookies - Select your language preference in your browser: fr, fr-fr, en - Load OPAC main page - OPAC is displayed in English, rather than French as asked by browser preferences Apply the patch and test: - Clear your browser cookies - Load OPAC main page - Pages are displayed in French
The attachment 6488 [details] [review] will break search pages, as getlanguagecookie is called by catalogue/search.pl and opac/opac-search.pl for obtain the locale for stems (option enable with syspref).
Created attachment 6489 [details] [review] Proposed patch, fixing comment 8 Bug 7282 Invalid language selection Frère Sébastien Marie refactored patch to ensure that without cookie, language selection is based on browser preferences. To reproduce the bug: - Activate en and fr-FR for OPAC - Clear your browser cookies - Select your language preference in your browser: fr, fr-fr, en - Load OPAC main page - OPAC is displayed in English, rather than French as asked by browser preferences Apply the patch and test: - Clear your browser cookies - Load OPAC main page - Pages are displayed in French
Created attachment 6490 [details] [review] Bug 7282 - invalid language selection I don't known exactly how to test stem as it is biblioitems dependent. For language selection, same test as before.
I prefer the patch from Frère Sébastien Marie, otherwise we are introducing 2 new places where user input is not sanitised. Also, having the regexp in one place means fixing it, by adding g for example is a lot easier.
(In reply to comment #9) > Created attachment 6489 [details] [review] > Proposed patch, fixing comment 8 oh ! you have corrected same time as me... so two patchs are proposed... your has some drawbacks: - re-introduce uncheck user input (I don't known for impact of user controlled string in Lingua::Stem::Snowball). - without cookie, HTTP_ACCEPT_LANG is not used for stem how to deal for the two patch proposed ?
(In reply to comment #11) > I prefer the patch from Frère Sébastien Marie, So do I. For the new getlanguage function, wouldn't it be better to explicitly specify the calling interface (opac/intra)? And call for example: my $lang = C4::Templates::getlanguage($cgi, 'opac');
Created attachment 6491 [details] [review] Bug 7282 - invalid language selection I agreed with Frederic, explicit parameter is better. I have corrected the patch accordingly, and correct a minor problem (the intranet search, as not specify the interface in getlanguage call, was using the 'opac' language... explicit argument is definitively better :-) )
Created attachment 6495 [details] [review] Signed-off patch
Could bug 7282 make a difference here? It's waiting for QA.
QA comment : valid code, fixes buggy behaviour perlcritic C4/Templates.pm C4/Templates.pm source OK perlcritic catalogue/ catalogue/search.pl source OK passed QA patch pushed, please test
Works on master, waiting to close this bug until patch is in 3.6.x too.
Chris N. - can you pick that for 3.6.x? It's really an annoying I18N bug, affects OPAC language detection and is confusing patrons.
+1
Thanks