Bug 6755

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.

(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.

(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?

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?

> 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.

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.

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!

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

(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?

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.

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.

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

"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.

(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?

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.

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.

> 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.

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.

> 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.

(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'.

> 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...

Easy change to read, just uses different source for subroutine.  Marking as Passed QA

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)

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.

Is there another bug number for this error? Otherwise, could you create one and assign it to me. Thanks.

Pushed, please test again

Follow up patch pushed, please test

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.

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

> 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.

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.

Could bug 7282 make a difference here? It's waiting for QA.

(In reply to comment #44)
> Could bug 7282 make a difference here? It's waiting for QA.

Yes, it does. You're fantastic Katrin!

Frédéric, does it mean that your patch (attachment 6831 [details] [review]) is now irrelevent because it fixes the same thing as 7282 ?

> 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 on attachment 6831 [details] [review]
Proposed patch

Not required anymore

Langauge detection and switching of languages seems ok now. Tested on current master.

This still exists on 3.4.8, reopening and marking as rel_3_4. Applying (manually) c01fcbf9 fixes the problem.

back to "patch pushed", so this bug comes back to chris_n eyes, (hopefully)

There have been no further reports of problems so I am marking this bug resolved.