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?
Created attachment 4191 [details] A screenshot illustrating one of the cases
I wonder if it's possible to globally rename the T:T variables which might conflict?
Created attachment 6721 [details] [review] proposed patch I don't know the database enough to judge if a name change would be the best solution, but I attach here a patch which would at least define a default value for size (empty space), preventing conflict with TT. A lot of lines appear modified because I corrected some indentation/space tabs, but only three lines of code were really added.
MT8364
Doesn't work well but I can't see why yet. I'll send a new patch.
Created attachment 7014 [details] [review] proposed patch #2 New patch, correcting staff+OPAC results. Corrects also a lot of indentation.
Created attachment 7016 [details] [review] Bug 6374: default value for Size in results In order to avoid the TT to display random numbers when the size information is not provided, a default empty string is sent if nothing else is available. Signed-off-by: Gaetan Boisson <gaetan.boisson@biblibre.com>
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.
Created attachment 7027 [details] [review] proposed patch #3 Modifies also the "addbooks" result page.
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>
This patch is mostly indentation changes... only two sections of code changing are done. Changes seems sane and secure. Marking as Passed QA
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)
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.