Bug 6374 - Use "size" as names/hash keys leads to an unexpected results when using Template::Toolkit (name of a virtual method there)
Summary: Use "size" as names/hash keys leads to an unexpected results when using Templ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: 3.6
Hardware: All All
: P3 normal (vote)
Assignee: Adrien SAURAT
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks: 5917
  Show dependency treegraph
 
Reported: 2011-05-19 15:28 UTC by Janusz Kaczmarek
Modified: 2014-06-19 12:31 UTC (History)
6 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
A screenshot illustrating one of the cases (24.52 KB, image/png)
2011-05-19 15:37 UTC, Janusz Kaczmarek
Details
proposed patch (4.86 KB, patch)
2011-12-12 11:30 UTC, Adrien SAURAT
Details | Diff | Splinter Review
proposed patch #2 (21.02 KB, patch)
2012-01-02 16:01 UTC, Adrien SAURAT
Details | Diff | Splinter Review
Bug 6374: default value for Size in results (21.08 KB, patch)
2012-01-02 16:32 UTC, Gaetan Boisson
Details | Diff | Splinter Review
proposed patch #3 (21.66 KB, patch)
2012-01-03 14:32 UTC, Adrien SAURAT
Details | Diff | Splinter Review
[SIGNED-OFF] (21.75 KB, patch)
2012-01-03 14:43 UTC, Gaetan Boisson
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Janusz Kaczmarek 2011-05-19 15:28:46 UTC
In many places the name "size" is used as hash key when the hash is passed to the template system for display.  So, for example, since "size" is used as a name of a column of bibioitems table, it appears (or not, depending on if it was defined or not) in elements of SEARCH_RESULTS (in catalogue/search.pl or opac-search.pl) and is passed to the template results.tt/opac-results.tt.  Then, in [opac-]results.tt a test is being done: [% IF ( SEARCH_RESULT.size ) %]

Now: if "size" was defined, everything is OK and the size will be displayed (e.g. 21 cm), BUT if it was not defined, then a "random" number appears.  

This is because "size" is one of the virtual methods defined in Template::Toolkit for every data type (cf. http://search.cpan.org/~abw/Template-Toolkit-2.22/lib/Template/Manual/VMethods.pod).  So, if "size" was defined by Koha, it will overload the method, but if it was not, then 1) XXX.size exists anyway, 2) the virtual method size() is called -- and it returns the number of elements in the hash or loop etc., depending on what is referred. 

The number of suspicious similar spots is ca. 20:  git grep "IF.*\.size" called in koha-tmpl

The issue does not seem to me to be straightforward to resolve, so I am not proposing any patch at the time.

Since it seems that in most of the cases it rooted in biblioitems.size column, maybe it would be the simplest to rename that column to 'format' (and make appropriate changes in the code)?  Any other ideas?
Comment 1 Janusz Kaczmarek 2011-05-19 15:37:16 UTC
Created attachment 4191 [details]
A screenshot illustrating one of the cases
Comment 2 Owen Leonard 2011-05-24 15:48:50 UTC
I wonder if it's possible to globally rename the T:T variables which might conflict?
Comment 3 Adrien SAURAT 2011-12-12 11:30:33 UTC Comment hidden (obsolete)
Comment 4 Adrien SAURAT 2011-12-13 09:14:14 UTC
MT8364
Comment 5 Adrien SAURAT 2011-12-26 10:27:45 UTC
Doesn't work well but I can't see why yet. I'll send a new patch.
Comment 6 Adrien SAURAT 2012-01-02 16:01:49 UTC Comment hidden (obsolete)
Comment 7 Gaetan Boisson 2012-01-02 16:32:06 UTC Comment hidden (obsolete)
Comment 8 Gaetan Boisson 2012-01-03 13:40:29 UTC
I turns out there are actually two search forms on the admin side concerned with this issue. (Maybe more?)
From the home of the admin interface, there is a quick search field and a link to advanced search, both using the same search which is now fixed.
But in the cataloguing section it is a different search. The patch needs to fix this part too.
Comment 9 Adrien SAURAT 2012-01-03 14:32:41 UTC Comment hidden (obsolete)
Comment 10 Gaetan Boisson 2012-01-03 14:43:59 UTC
Created attachment 7028 [details] [review]
[SIGNED-OFF]

Works beautifully!

Bug 6374: default value for Size in result pages

When no size info is available, an empty string is
sent to the TT (if nothing is sent, the TT engine
will display another information, irrelevant for Koha).

Signed-off-by: Gaetan Boisson <gaetan.boisson@biblibre.com>
Comment 11 Ian Walls 2012-01-06 13:11:16 UTC
This patch is mostly indentation changes... only two sections of code changing are done.  Changes seems sane and secure.  Marking as Passed QA
Comment 12 Paul Poulain 2012-01-06 15:04:38 UTC
Patch pushed, please test

Gaetan, just a small comment: when you signoff a patch, be carefull : the 1st line is the header of the patch comment and is what appear on all lists.

you've added
"works beautifully !", which is not a good description ;-)

i've modified the comment to put your happyness after your signature !
Also note that your enthousiasm is nice, but it's not really usefull in the patch itself. Remember it's in the patch forever (really)
Comment 13 Chris Nighswonger 2012-01-21 05:17:17 UTC
FWIW... it looks like large portions of this patch are simply reformatting of code style. Please be sure to keep those sorts of changes to a minimum in bugfixes, and submit separate patches to handle reformatting only.