Bug 6755 - Problems with switching languages
Summary: Problems with switching languages
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: 3.4
Hardware: All All
: P3 major (vote)
Assignee: Marcel de Rooy
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks: 6563
  Show dependency treegraph
 
Reported: 2011-08-19 18:47 UTC by Katrin Fischer
Modified: 2013-12-05 20:05 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch (1.82 KB, patch)
2011-08-25 13:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Revised patch (3.32 KB, patch)
2011-09-09 14:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Signed-off patch (3.51 KB, patch)
2011-09-09 15:35 UTC, Frédéric Demians
Details | Diff | Splinter Review
Revised patch (25.18 KB, patch)
2011-09-12 15:27 UTC, Frédéric Demians
Details | Diff | Splinter Review
Follow up patch (928 bytes, patch)
2011-09-22 09:11 UTC, Alex Arnaud
Details | Diff | Splinter Review
[SIGNED-OFF] Problems with switching languages (25.20 KB, patch)
2011-09-22 13:37 UTC, Alex Arnaud
Details | Diff | Splinter Review
Signed-off follow-up patch (1.06 KB, patch)
2011-09-22 14:05 UTC, Frédéric Demians
Details | Diff | Splinter Review
Consolidate language selection for templates (9.65 KB, patch)
2011-09-23 13:44 UTC, Frédéric Demians
Details | Diff | Splinter Review
Follow up patch (1.50 KB, patch)
2011-09-26 18:44 UTC, Frédéric Demians
Details | Diff | Splinter Review
[3.4.x] First patch missing (1.51 KB, patch)
2011-10-08 21:59 UTC, Frédéric Demians
Details | Diff | Splinter Review
Second missing patch (9.67 KB, patch)
2011-10-08 22:00 UTC, Frédéric Demians
Details | Diff | Splinter Review
[3.4.x] Second missing patch (9.67 KB, patch)
2011-10-08 22:01 UTC, Frédéric Demians
Details | Diff | Splinter Review
Proposed patch (1.30 KB, patch)
2011-12-16 08:59 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 Katrin Fischer 2011-08-19 18:47:38 UTC
On current master and 3.4.3 there is something that happens quite often:

I open Koha and see english strings but the language chooser shows that German is selected. After I change to English and back to German, all is normal.

In my browser German is the preferred langauge. The only activated languages for staff and OPAC are German and English. Those are also the only 2 languages available.
Comment 1 Magnus Enger 2011-08-22 06:24:23 UTC
I can confirm this bug - I'm seeing the same things for installs with English and Norwegian. My gut feeling tells me it mostly happens when I have been away from an OPAC long enough for the language cookie to expire, but I have not tested that properly.
Comment 2 Marcel de Rooy 2011-08-25 13:10:41 UTC Comment hidden (obsolete)
Comment 3 Frédéric Demians 2011-09-09 11:20:11 UTC
(In reply to comment #0)
> On current master and 3.4.3 there is something that happens quite
> often:
> I open Koha and see english strings but the language chooser shows
> that German is selected. After I change to English and back to German,
> all is normal.  In my browser German is the preferred langauge. The
> only activated languages for staff and OPAC are German and English.
> Those are also the only 2 languages available.

Same issue with English and French available in the OPAC. M. de Rooy
patch doesn't solve this bug. We may habe to open a separate bug for it.
Comment 4 Marcel de Rooy 2011-09-09 11:23:52 UTC
(In reply to comment #3)
> (In reply to comment #0)
> > On current master and 3.4.3 there is something that happens quite
> > often:
> > I open Koha and see english strings but the language chooser shows
> > that German is selected. After I change to English and back to German,
> > all is normal.  In my browser German is the preferred langauge. The
> > only activated languages for staff and OPAC are German and English.
> > Those are also the only 2 languages available.
> 
> Same issue with English and French available in the OPAC. M. de Rooy
> patch doesn't solve this bug. We may habe to open a separate bug for it.

Could you provide more details why the patch does not resolve it. I tested with English and Dutch and it resolved this phenomenon. Did I overlook something?
Comment 5 Katrin Fischer 2011-09-09 11:31:39 UTC
Hi Marcel, your patch says it resolves a problem with the news. What I see is not related to the news but to the 'normal' templates. Perhaps this is the wrong patch?
Comment 6 Frédéric Demians 2011-09-09 11:36:57 UTC
> Could you provide more details why the patch does not resolve it. I
> tested with English and Dutch and it resolved this phenomenon. Did I
> overlook something?

I confirm I have this bug (or a very close one) with ou without your
patch. Here is exactly what I have:

- In FF, I have set language preferances order to: fr, fr-fr, en, en-us.
- In FF, I clear history with cookies.
- I open an OPAC. 
- In the bottom of the page, I can see the language selection tab:
  French (Français) is selected ; but the OPAC language is not French
  but English.
- In the language selection tab, I click on English.
- The OAPC is reloaded.
- In the language selection tab, English is selected. The page is
  displayed in English.
- In the language selection tab, I click on French.
- The OAPC is reloaded.
- In the language selection tab, French is selected. The page is
  displayed in French.
Comment 7 Marcel de Rooy 2011-09-09 12:49:59 UTC
Thanks Frederic, I see what you mean now.
To be even more specific, I have the following:
Search bar is English and also top link Log in to your account.
News is Dutch (with the patch I submitted).
Login to your account (next to news) is also Dutch.
Selected language is also Dutch in the language bar.
In summary, top of the screen is English, everything below is Dutch.
Will take a look again, resetting status in the meantime.
Comment 8 Marcel de Rooy 2011-09-09 14:40:08 UTC
Sending a revised patch. I found two other problems:

1) The call in Output/gettemplate did not pass the CGI object to C4::Templates->new. This routine called themelanguage but WITHOUT cgi object! So it ignored the cookie. My patch now optionally adds the query parameter to Templates->new and corrects the themelanguage call. 

2) There were two routines themelanguage. One in Templates.pm and one in Output.pm.

This should resolve it now hopefully. Please test!
Comment 9 Marcel de Rooy 2011-09-09 14:40:41 UTC Comment hidden (obsolete)
Comment 10 Frédéric Demians 2011-09-09 15:35:17 UTC Comment hidden (obsolete)
Comment 11 Chris Cormack 2011-09-11 22:09:58 UTC
The one in C4/Templates is the one to use, not the the one in C4/Output

We have now created a circular dependency. Please redo the patch to use themelanguage in C4::Templates
Comment 12 Frédéric Demians 2011-09-12 04:49:35 UTC
(In reply to comment #11)
> The one in C4/Templates is the one to use, not the the one in
> C4/Output

> We have now created a circular dependency. Please redo the patch to
> use themelanguage in C4::Templates

I don't understand. I can see that there are two themelanguage
functions, one in C4::Template and one in C4::Output, which is bad! But
I don't see how it create a circular dependency. Could you explain?
Shouldn't this patch choose one function or the other, and so delete one
of them to avoid this mistake in the future?
Comment 13 Chris Cormack 2011-09-12 05:13:04 UTC
This patch changes to using the one in C4/Output, instead of using the one in C4/Template, like it used to.

The one in C4/Output only exists as part of the transition away from HTML::Template::Pro

So with this patch we have C4::Output calling C4::Template which calls a function in C4::Output.

Id accept the patch if it used the themelanguage subroutine in C4::Template, i'd like it even more if it got rid of the one in C4::Output.
Comment 14 Marcel de Rooy 2011-09-12 09:53:45 UTC
I understand the issues raised, but do/did not have the time to send the theoretically best solution. So, it is a pragmatic patch to solve an annoying issue for non-English users. 
Failed QA is fine with me, but the current code should have failed QA too!
I can send another patch, but it will take some weeks. No problem if someone else does the job now. The problem is clear.
Comment 15 Chris Cormack 2011-09-12 10:23:33 UTC
I'll refactor the patch to use themelanguage in C4::Template. Unless someone beats me to do that, I'm currently reinstalling debian so you have a bit of a head start :)

It shouldn't be a big job, just changing the calls to use the local themelanguage instead of the one in C4::Output and testing it still works
Comment 16 Frédéric Demians 2011-09-12 15:27:59 UTC Comment hidden (obsolete)
Comment 17 Chris Cormack 2011-09-12 18:19:39 UTC
"1) The call in Output/gettemplate did not pass the CGI object to
C4::Templates->new. This routine called themelanguage but WITHOUT cgi object!
So it ignored the cookie. My patch now optionally adds the query parameter to
Templates->new and corrects the themelanguage call. "

I think this original contention is not correct

Looking at C4::Templates themelanguage

   my ( $htdocs, $tmpl, $interface ) = @_;                                                                                  
    my $query = new CGI;                                                                                                     
                                                                                                                             
    # Set some defaults for language and theme                                                                               
    # First, check the user's preferences                                                                                    
    my $lang;                                                                                                                
                                                                                                                             
    # But, if there's a cookie set, obey it                                                                                  
    $lang = $query->cookie('KohaOpacLanguage')                                                                               
      if ( defined $query and $query->cookie('KohaOpacLanguage') );


Its true $query is not passed, but its initiated there, and the cookie is checked.

So I think the bug is something different, the new patch (and the old one) reintroduce the behaviour of using the browsers language setting if no cookie is set. Which was removed because it was causing problems.
Comment 18 Frédéric Demians 2011-09-12 18:57:20 UTC
(In reply to comment #17)
> "1) The call in Output/gettemplate did not pass the CGI object to
> C4::Templates->new. This routine called themelanguage but WITHOUT cgi
> object!  So it ignored the cookie. My patch now optionally adds the
> query parameter to Templates->new and corrects the themelanguage call.
> "

Thanks a lot for taking care of all those subtleties. Have you apply my
last patch? gettemplate isn't anymore in Output.pm. For my myopic eyes,
it seems correct. The above comment may not say exactly what is the
code...

> So I think the bug is something different, the new patch (and the old
> one) reintroduce the behaviour of using the browsers language setting
> if no cookie is set. Which was removed because it was causing
> problems.

? Isn't it a desirable behavior?
Comment 19 Katrin Fischer 2011-09-12 19:08:32 UTC
One of the problems with detecting the language from the browser settings was, that not only the selected languages from the system preferences were taken into account, but all existing templates. Often libraries had installed all templates because the big tarball was used and customized only one language in a way, that broke the layout for other languages that were not activated. Language chooser is often deactivated so you end up trapped in a broken layout, broken templates (becaues they were not updated when the system was) and/or broken/incorrect/incomplete translations.
Comment 20 Chris Cormack 2011-09-12 19:45:40 UTC
Hi Frédéric

I think you may have misunderstood, I was quoting marcel, then I pointed out the existing code is checking the cookie. So I dont think that is actually the problem. The new patch changes a lot of things including reintroducing a behaviour that was removed. It may also have the effect of making the language picker work, but I dont think it has anything to do with the cgi object not being passed. I would rather we understand what the problem is, and just change the themelanguage sub in C4::Template.

But if someone wants to sign off and then it makes it through QA I will revisit.
Comment 21 Frédéric Demians 2011-09-13 04:54:18 UTC
> But if someone wants to sign off and then it makes it through QA I
> will revisit.

Ok, thanks. So I see two options: (1) As you said someone can confirm
that my patch, which has the advantage of cleaning code, solves this bug
and hasn't side effect ; (2) Marcel amends its initial patch, taking
into account your comment.
Comment 22 Katrin Fischer 2011-09-16 21:36:28 UTC
I am not sure I understand everything that was talked about here - but I really think we need a fix for this problem and it would be very good to have a solution in 3.4.5.
Comment 23 Alex Arnaud 2011-09-22 09:11:53 UTC Comment hidden (obsolete)
Comment 24 Frédéric Demians 2011-09-22 09:33:41 UTC
> I've applied the patch and got an error when searching for a system
> preference.  This is because preferences.pl still uses
> C4::Output::_get_template_file (which no longer exists). So this
> follow up patch makes preferences.pl uses
> C4::Templates::_get_template_file.

Merci, thanks for testing and catching this omission... I can confirm
your patch works and is required. If you could confirm that with my
patch and yours there isn't any other page which doesn't load or load
with the false language, you could sign it of. Even if, in last resort,
Chris has to decide based on his criteria.
Comment 25 Alex Arnaud 2011-09-22 13:07:19 UTC

(In reply to comment #24)
> > I've applied the patch and got an error when searching for a system
> > preference.  This is because preferences.pl still uses
> > C4::Output::_get_template_file (which no longer exists). So this
> > follow up patch makes preferences.pl uses
> > C4::Templates::_get_template_file.
> 
> Merci, thanks for testing and catching this omission... I can confirm
> your patch works and is required. If you could confirm that with my
> patch and yours there isn't any other page which doesn't load or load
> with the false language, you could sign it of. Even if, in last resort,
> Chris has to decide based on his criteria.

Frédéric,
 
I've signed off and sent the first patch. I let you do the same for the follow up and change the patch status to 'Signed off'.
Comment 26 Frédéric Demians 2011-09-22 13:30:05 UTC
> I've signed off and sent the first patch. I let you do the same for the follow
> up and change the patch status to 'Signed off'.

Please, attach signed-off patch to this bug, replacing the original one. I will sign-off yours but a second sign-off may be necessary...
Comment 27 Alex Arnaud 2011-09-22 13:37:25 UTC Comment hidden (obsolete)
Comment 28 Ian Walls 2011-09-22 13:47:21 UTC
Easy change to read, just uses different source for subroutine.  Marking as Passed QA
Comment 29 Frédéric Demians 2011-09-22 14:05:57 UTC Comment hidden (obsolete)
Comment 30 Chris Cormack 2011-09-22 21:49:34 UTC
Please test very well. This does reintroduce the behaviour of overriding whatever settings the library has specified for their opac, with the language chosen by the users browser.

This may cause problem, so please test that scenario before marking this resolved. I think it is a bad idea, but the bug itself is bad enough that I will push it and hopefully if it really is a bad idea someone will follow up with a patch which makes the language picker respect what has been chosen in the system prefences. (Like the cookie does)
Comment 31 Magnus Enger 2011-09-23 07:22:51 UTC
I'm afraid I don't have the time to look properly at this now, but does it reintroduce the situation where e.g.:

- english, swedish and norwegian are all *installed*
- english and swedish is *activated* for the opac
- i visit with my browser preference set to norwegian and i'm shown the norwegian templates, even if they are not activated

If this behaviour is re-introduced i'm giving it a "thumbs down" - we should only show templates that are activated.
Comment 32 Frédéric Demians 2011-09-23 09:10:40 UTC
Is there another bug number for this error? Otherwise, could you create one and assign it to me. Thanks.
Comment 33 Frédéric Demians 2011-09-23 13:44:42 UTC Comment hidden (obsolete)
Comment 34 Chris Cormack 2011-09-26 10:27:50 UTC
Pushed, please test again
Comment 35 Frédéric Demians 2011-09-26 18:44:06 UTC Comment hidden (obsolete)
Comment 36 Chris Cormack 2011-09-26 19:23:00 UTC
Follow up patch pushed, please test
Comment 37 Chris Nighswonger 2011-10-08 21:32:02 UTC
This patch series does not apply cleanly to 3.4.x. If it should, please rebase against that branch and resubmit with [3.4.x] in the subject line.
Comment 38 Frédéric Demians 2011-10-08 21:59:34 UTC Comment hidden (obsolete)
Comment 39 Frédéric Demians 2011-10-08 22:00:12 UTC Comment hidden (obsolete)
Comment 40 Frédéric Demians 2011-10-08 22:01:17 UTC Comment hidden (obsolete)
Comment 41 Magnus Enger 2011-10-18 13:38:31 UTC
On 3.4.5, whenever I try to move from step 2 to step 3 in the guided reports wizzard, I get this error: 

Undefined subroutine &C4::Reports::Guided::themelanguage called at /home/magnus/kohanor32/C4/Reports/Guided.pm line 714.

Line 714 is this: 
my ($theme, $lang) = themelanguage($htdocs, $columns_def_file, $section,$cgi);

This does not happen on current master. Looks to me like these two patches from master should be applied to 3.4.x too: 

http://git.koha-community.org/gitweb/?p=koha.git;a=commitdiff;h=3bb33fd666d4d0eea26114b373874189edc82459

http://git.koha-community.org/gitweb/?p=koha.git;a=commitdiff;h=c01fcbf97306d6fdcea001dbceaa4dc79c63239a
Comment 42 Frédéric Demians 2011-10-18 13:42:27 UTC
> This does not happen on current master. Looks to me like these two patches from
> master should be applied to 3.4.x too: 
> 
> http://git.koha-community.org/gitweb/?p=koha.git;a=commitdiff;h=3bb33fd666d4d0eea26114b373874189edc82459
> 
> http://git.koha-community.org/gitweb/?p=koha.git;a=commitdiff;h=c01fcbf97306d6fdcea001dbceaa4dc79c63239a

I confirm your assumption. I had to do it today on two live 3.4.5 sites.
Comment 43 Frédéric Demians 2011-12-16 08:55:57 UTC
As a side effect of patches for bug 6629, this bug has been reintroduced
in HEAD and 3.6.x. 

To reproduce it:

- Define two languages for OPAC: en and fr-FR
- Open Firefox (for example)
- Menu Edit > Preferences, tab Option: select Language fr, en
- Clear your cookies
- Open the OPAC
- The OPAC is displayed in English. This is wrong. The fr templates
  should have been used based on browser languages preferences.
Comment 44 Katrin Fischer 2011-12-16 08:58:48 UTC
Could bug 7282 make a difference here? It's waiting for QA.
Comment 45 Frédéric Demians 2011-12-16 08:59:40 UTC Comment hidden (obsolete)
Comment 46 Frédéric Demians 2011-12-16 09:07:09 UTC
(In reply to comment #44)
> Could bug 7282 make a difference here? It's waiting for QA.

Yes, it does. You're fantastic Katrin!
Comment 47 Paul Poulain 2011-12-16 12:33:25 UTC
Frédéric, does it mean that your patch (attachment 6831 [details] [review]) is now irrelevent because it fixes the same thing as 7282 ?
Comment 48 Frédéric Demians 2011-12-16 12:40:33 UTC
> Frédéric, does it mean that your patch (attachment 6831 [details] [review]) is
> now irrelevent because it fixes the same thing as 7282 ?

Yes, Paul. 7282 has the effect to solve 6755. I don't know how to mark
the proposed patch as obsolete without attaching a new one...
Comment 49 Frédéric Demians 2011-12-16 17:26:06 UTC
Comment on attachment 6831 [details] [review]
Proposed patch

Not required anymore
Comment 50 Katrin Fischer 2011-12-18 20:55:33 UTC
Langauge detection and switching of languages seems ok now. Tested on current master.
Comment 51 Yuval Hager 2012-02-14 18:43:48 UTC
This still exists on 3.4.8, reopening and marking as rel_3_4. Applying (manually) c01fcbf9 fixes the problem.
Comment 52 Paul Poulain 2012-02-20 10:44:17 UTC
back to "patch pushed", so this bug comes back to chris_n eyes, (hopefully)
Comment 53 Jared Camins-Esakov 2012-12-31 00:39:20 UTC
There have been no further reports of problems so I am marking this bug resolved.