Bug 7282 - invalid language selection
Summary: invalid language selection
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: 3.6
Hardware: All All
: P3 major (vote)
Assignee: Sébastien Marie
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-01 15:56 UTC by Sébastien Marie
Modified: 2012-10-25 23:10 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7282: invalid language selection (981 bytes, patch)
2011-12-01 16:04 UTC, Sébastien Marie
Details | Diff | Splinter Review
Bug 7282: invalid language selection (1.01 KB, patch)
2011-12-01 16:17 UTC, Chris Cormack
Details | Diff | Splinter Review
Proposed patch (1.95 KB, patch)
2011-12-01 17:04 UTC, Frédéric Demians
Details | Diff | Splinter Review
Proposed patch, fixing comment 8 (3.36 KB, patch)
2011-12-01 18:08 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 7282 - invalid language selection (4.18 KB, patch)
2011-12-01 18:09 UTC, Sébastien Marie
Details | Diff | Splinter Review
Bug 7282 - invalid language selection (4.18 KB, patch)
2011-12-01 18:48 UTC, Sébastien Marie
Details | Diff | Splinter Review
Signed-off patch (4.72 KB, patch)
2011-12-01 20:01 UTC, Frédéric Demians
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Sébastien Marie 2011-12-01 15:56:37 UTC
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 :-) )
Comment 1 Sébastien Marie 2011-12-01 16:04:14 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2011-12-01 16:10:08 UTC
Ah yep, good catch, ill test a little more and sign off
Comment 3 Chris Cormack 2011-12-01 16:17:51 UTC Comment hidden (obsolete)
Comment 4 Frédéric Demians 2011-12-01 16:47:57 UTC
> 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.
Comment 5 Chris Cormack 2011-12-01 16:54:01 UTC
Frederic, FR is activated in the system preferences eh?
Comment 6 Frédéric Demians 2011-12-01 16:59:41 UTC
(In reply to comment #5)
> Frederic, FR is activated in the system preferences eh?

Yes.
Comment 7 Frédéric Demians 2011-12-01 17:04:50 UTC Comment hidden (obsolete)
Comment 8 Sébastien Marie 2011-12-01 17:53:55 UTC
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).
Comment 9 Frédéric Demians 2011-12-01 18:08:28 UTC Comment hidden (obsolete)
Comment 10 Sébastien Marie 2011-12-01 18:09:45 UTC Comment hidden (obsolete)
Comment 11 Chris Cormack 2011-12-01 18:13:11 UTC
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.
Comment 12 Sébastien Marie 2011-12-01 18:17:17 UTC
(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 ?
Comment 13 Frédéric Demians 2011-12-01 18:38:34 UTC
(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');
Comment 14 Sébastien Marie 2011-12-01 18:48:00 UTC Comment hidden (obsolete)
Comment 15 Frédéric Demians 2011-12-01 20:01:30 UTC
Created attachment 6495 [details] [review]
Signed-off patch
Comment 16 Katrin Fischer 2011-12-16 08:57:39 UTC
Could bug 7282 make a difference here? It's waiting for QA.
Comment 17 Paul Poulain 2011-12-16 13:32:41 UTC
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
Comment 18 Katrin Fischer 2011-12-18 20:57:00 UTC
Works on master, waiting to close this bug until patch is in 3.6.x too.
Comment 19 Katrin Fischer 2011-12-23 13:34:45 UTC
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.
Comment 20 Magnus Enger 2011-12-23 14:01:11 UTC
+1
Comment 21 Sébastien Marie 2012-02-07 15:06:51 UTC
Thanks