Created attachment 59511 [details] [review] Bug 17982: Fix the use of uniq in sub themelanguage Doing uniq( \@themes ) is useless. It will just return to you the only reference you gave it. List::MoreUtils::uniq requires a list instead of an arrayref. So it is a trivial fix that makes sub themelanguage return one theme instead of three themes like [ 'prog', 'prog', 'prog' ]. Note that Template->new inserts one or two include paths to TT for each of these three identical themes. Added a unit to be somewhat overcomplete :) Test plan: [1] Run t/db_dependent/Templates.t [2] Open a page on OPAC or intranet. (Did you restart Plack?) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59537 [details] [review] Bug 17982: Fix the use of uniq in sub themelanguage Doing uniq( \@themes ) is useless. It will just return to you the only reference you gave it. List::MoreUtils::uniq requires a list instead of an arrayref. So it is a trivial fix that makes sub themelanguage return one theme instead of three themes like [ 'prog', 'prog', 'prog' ]. Note that Template->new inserts one or two include paths to TT for each of these three identical themes. Added a unit test to be somewhat overcomplete :) Test plan: [1] Run t/db_dependent/Templates.t [2] Open a page on OPAC or intranet. (Did you restart Plack?) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59630 [details] [review] Bug 17982: Fix the use of uniq in sub themelanguage Doing uniq( \@themes ) is useless. It will just return to you the only reference you gave it. List::MoreUtils::uniq requires a list instead of an arrayref. So it is a trivial fix that makes sub themelanguage return one theme instead of three themes like [ 'prog', 'prog', 'prog' ]. Note that Template->new inserts one or two include paths to TT for each of these three identical themes. Note: Removed a overcomplete unit test that basically tested uniq with uniq in order to remove a dependency for a more important bug report. Test plan: [1] Run t/db_dependent/Auth.t (triggering themelanguage) [2] Open a page on OPAC or intranet. (Did you restart Plack?) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment on attachment 59630 [details] [review] Bug 17982: Fix the use of uniq in sub themelanguage Review of attachment 59630 [details] [review]: ----------------------------------------------------------------- ::: C4/Templates.pm @@ +256,4 @@ > my $where = $tmpl =~ /xsl$/ ? 'xslt' : 'modules'; > for my $theme (@themes) { > if ( -e "$htdocs/$theme/$lang/$where/$tmpl" ) { > + return ( $theme, $lang, [ uniq(@themes) ] ); I'm not sure the parenthesis for uniq are necessary.
(In reply to M. Tompsett from comment #4) > Comment on attachment 59630 [details] [review] [review] > Bug 17982: Fix the use of uniq in sub themelanguage > > Review of attachment 59630 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: C4/Templates.pm > @@ +256,4 @@ > > my $where = $tmpl =~ /xsl$/ ? 'xslt' : 'modules'; > > for my $theme (@themes) { > > if ( -e "$htdocs/$theme/$lang/$where/$tmpl" ) { > > + return ( $theme, $lang, [ uniq(@themes) ] ); > > I'm not sure the parenthesis for uniq are necessary. Seems to be a more a matter of taste. My personal preference is parentheses, only omitting them with builtins like open or print.
Created attachment 59747 [details] [review] Bug 17982: Improve test coverage This test triggers the three cases that were modified. prove -v t/db_dependent/Templates.t
Created attachment 59748 [details] [review] Bug 17982: Improve test coverage This test triggers the three cases that were modified. prove -v t/db_dependent/Templates.t
Created attachment 59749 [details] [review] Bug 17982: Fix the use of uniq in sub themelanguage Doing uniq( \@themes ) is useless. It will just return to you the only reference you gave it. List::MoreUtils::uniq requires a list instead of an arrayref. So it is a trivial fix that makes sub themelanguage return one theme instead of three themes like [ 'prog', 'prog', 'prog' ]. Note that Template->new inserts one or two include paths to TT for each of these three identical themes. Note: Removed a overcomplete unit test that basically tested uniq with uniq in order to remove a dependency for a more important bug report. Test plan: [1] Run t/db_dependent/Auth.t (triggering themelanguage) [2] Open a page on OPAC or intranet. (Did you restart Plack?) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 59750 [details] [review] Bug 17982: Improve test coverage This test triggers the three cases that were modified. prove -v t/db_dependent/Templates.t
Mark, Thx for signoff but please explain what the second test has to do with the first patch? You are testing language, but the change is about not duplicating themes by proper use of uniq. Marcel
Apart from that, it fails too: # Subtest: Test C4::Templates::themelanguage 1..4 not ok 1 - Expected language: non-en # Failed test 'Expected language: non-en' # at t/db_dependent/Templates.t line 68. # got: 'en' # expected: 'fr-FR' ok 2 - Expected language: default en ok 3 - Expected language: unset ok 4 - Expected language: not coded for # Looks like you failed 1 test of 4.
Do not assume that fr-FR exists. I dont have it :)
Mock::FileSystem ?
(In reply to Marcel de Rooy from comment #10) > Thx for signoff but please explain what the second test has to do with the > first patch? You are testing language, but the change is about not > duplicating themes by proper use of uniq. You changed 3 lines of code, this test triggers those three lines of code. However, you rightly pointed out fr-FR might not exist. Shoot. :( If it does exist, it triggers the three cases you changed.
(In reply to Marcel de Rooy from comment #13) > Mock::FileSystem ? That looks promising. I'll try something in about 2 hours.
Comment on attachment 59750 [details] [review] Bug 17982: Improve test coverage Because there is no easy way to mock the file system. I'll just forget this, and sign off.
Created attachment 59765 [details] [review] Bug 17982: Expose wrong use of uniq This test should fail without the patch fixing the uniq calls. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59766 [details] [review] Bug 17982: Fix the use of uniq in sub themelanguage Doing uniq( \@themes ) is useless. It will just return to you the only reference you gave it. List::MoreUtils::uniq requires a list instead of an arrayref. So it is a trivial fix that makes sub themelanguage return one theme instead of three themes like [ 'prog', 'prog', 'prog' ]. Note that Template->new inserts one or two include paths to TT for each of these three identical themes. Test plan: [1] Run t/db_dependent/Templates.t (should no longer fail) [2] Run t/db_dependent/Auth.t (triggering themelanguage) [3] Open a page on OPAC or intranet. (Did you restart Plack?) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> EDIT (Marcel): Amended test plan for additional unit test.
Created attachment 59790 [details] [review] Bug 17982: Expose wrong use of uniq This test should fail without the patch fixing the uniq calls. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 59791 [details] [review] Bug 17982: Fix the use of uniq in sub themelanguage Doing uniq( \@themes ) is useless. It will just return to you the only reference you gave it. List::MoreUtils::uniq requires a list instead of an arrayref. So it is a trivial fix that makes sub themelanguage return one theme instead of three themes like [ 'prog', 'prog', 'prog' ]. Note that Template->new inserts one or two include paths to TT for each of these three identical themes. Test plan: [1] Run t/db_dependent/Templates.t (should no longer fail) [2] Run t/db_dependent/Auth.t (triggering themelanguage) [3] Open a page on OPAC or intranet. (Did you restart Plack?) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> EDIT (Marcel): Amended test plan for additional unit test. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 17.05, thanks Marcel!
Should this be in 16.11.x? Not sure if it fixes a bug atm.
(In reply to Katrin Fischer from comment #22) > Should this be in 16.11.x? Not sure if it fixes a bug atm. You could call it a tiny bug, but no real need to backport it.
Ok, will leave it out for now then. If it's needed for something later on, we can come back to this.