Bug 10466 - Lists: Use "size" as names/hash keys leads to an unexpected results when using Template::Toolkit (name of a virtual method there)
Summary: Lists: Use "size" as names/hash keys leads to an unexpected results when usin...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: David Cook
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-14 02:40 UTC by David Cook
Modified: 2015-06-04 23:23 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10466 - Lists: Use "size" as names/hash keys leads to an unexpected results when using Template::Toolkit (name of a virtual method there) (3.18 KB, patch)
2013-06-14 03:15 UTC, David Cook
Details | Diff | Splinter Review
Bug 10466 - Lists: Use "size" as names/hash keys leads to an unexpected results when using Template::Toolkit (name of a virtual method there) (3.35 KB, patch)
2013-06-14 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2013-06-14 02:40:13 UTC
This is an old bug that has been spotted again. This time, it's been noticed in the Lists/Virtual Shelves...

Basically, itemsloo.size produces different results depending on the type of data that is in the itemsloo hash (within the itemsloop array):

1) If "size" has a value, that value will be printed out.

2) If "size" is null/undef, a seemingly random number will print out. 

3) If there is no "size" key in the hash, a seemingly random number will print out.

(N.B. In fact, this "random" number is the number of keys in the itemsloo hash. "Size" is a virtual method in the template toolkit. http://www.template-toolkit.org/docs/manual/VMethods.html.)

--

Currently, elsewhere in Koha (bug 6374 which covers search.pl, addbooks.pl, opac-search.pl), we're checking to see if a "size" key exists, and if not, we're inserting a "size" key with a value of "". 

In this case, as long as the SQL query includes the size column, there will always be a "size" key (so scenario 3 is irrelevant). We need to check if "size" is "defined", and if it's not (scenario 2 from above), then we change it to a blank (e.g. "") value, so that it just prints out nothing. 

The fact that "size" does print a value is probably a bug in TT, so I don't think this is the best long-term solution. It would probably make more sense to rename the variable that we're passing to TT, because if TT gets "fixed" so that "size" always operates as a method rather than fetching the value of a hash, we are going to have to do this all over again.
Comment 1 David Cook 2013-06-14 03:15:32 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2013-06-14 08:07:10 UTC
I do remember this bug. Do not know where it was.
But I think there was code that forces the size value to be there.
Comment 3 Marcel de Rooy 2013-06-14 09:08:37 UTC
Wow. So much words for one line :)
Compliments for such a detailed test plan and even for finding such a small bug.
Comment 4 Marcel de Rooy 2013-06-14 09:13:25 UTC
Created attachment 18980 [details] [review]
Bug 10466 - Lists: Use "size" as names/hash keys leads to an unexpected results when using Template::Toolkit (name of a virtual method there)

This patch checks to see if "size" is undefined. If it is, we add a
blank (i.e. "") value to it in place of undef.

If we do not do this, calling "itemloo.size" will return the size
of the "itemloo" hash, rather than the value for the "size" key.

This is because "size" is a virtual method in Template Toolkit. It's
uncertain why the value is retrieved for the "size" key when there is
a defined value and why TT doesn't use the method instead, and that
it uses "size" as a method only if there is either no "size" key or
if the value tied to the "size" key is null/undef. This might be a
feature or it might be a bug in TT...

In the mean-time, we will check to see if it's undefined. If it is,
we'll give it a value.

This bug has been identified in the opac-search.pl, search.pl and
addbooks.pl pages before. To address it, we're currently checking
if there is a "size" key, and if not...we're adding one with a blank
value.

This patch takes up that same idea, although I think it might be better
to rename the variable before passing it to TT in case the behaviour
of TT changes in the future in regards to how it handles virtual
methods.

N.B. Obviously, this only affects users not using XSLTs.

--

Test Plan:

Before applying the patch:

0) Make sure you have opac search result XSLT turned off

1) Find bib records that do not have a 300$c (Dimensions) value.
2) Find bib records that do have a 300$c (Dimensions) value.

(N.B. These values should be stored in the `size` column of biblioitems).

3) Add items from both sets of records to a List
4) Note that records without a 300$c will display a number at the end
of the "Publication" description/string. It should be something like
33 or 34 in most cases.
5) Note that records with a 300$c don't display this number. They just
show the value from 300$c.

Apply the patch.

6) Clear your cache, refresh the page, etc.
7) Note that the number (e.g. 33 or 34) has disappeared from the end
of the "Publication" description/string.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Added a small comment at the end of this one line.
Comment 5 Marcel de Rooy 2013-06-14 09:22:37 UTC
QA Comment:
One line patch fixes a small problem.
Altough we could indeed solve this problem even better by replacing size with something else, this fix deserves to be pushed if it were only for the commit message and test plan :)
Additionally, there is at least one other place where this same problem is also solved in this same way (and was pushed before..)

Note that I took the liberty to amend with a small comment at the end of your line about the reason you explained so thoroughly in the commit message.

Passed QA
Comment 6 Galen Charlton 2013-06-14 23:31:20 UTC
I've pushed this to master.  I updated the commit message to clarify to normal users the problem being corrected.  Yes, I fully realize that this has made the commit message EVEN LONGER! :)

Thanks, David, especially for taking the time to fully describe your thinking on this.
Comment 7 Tomás Cohen Arazi 2013-07-02 18:04:37 UTC
This patch has been pushed to 3.12.x, will be in 3.12.2.

Thanks David!
Comment 8 Bernardo Gonzalez Kriegel 2013-07-13 17:49:36 UTC
Pushed to 3.10.x, will be in 3.10.8
Comment 9 Chris Hall 2013-07-18 09:01:00 UTC
Pushed to 3.8.x, will be in 3.8.15