[Bug 8175] New: Missing defined check in catalogue/details.pl
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Priority: P5 - low Change sponsored?: --- Bug ID: 8175 Assignee: gmcharlt@gmail.com Summary: Missing defined check in catalogue/details.pl Severity: enhancement Classification: Unclassified OS: All Reporter: mtompset@hotmail.com Hardware: All Status: NEW Version: rel_3_6 Component: Cataloging Product: Koha SchemaSpy says items.materials can be null C4::Items seems to just pull the value from items.* Error log was generating: [Fri Jun 01 07:14:16 2012] [error] [client 192.168.100.2] [Fri Jun 1 07:14:16 2 012] detail.pl: Use of uninitialized value in string ne at /usr/share/koha/intra net/cgi-bin/catalogue/detail.pl line 255. I believe 255 should read: if (defined($item->{'materials'}) && $item->{'materials'} ne ''){ It currently reads: if ($item->{'materials'} ne ''){ I was looking at 3.6.3 source I didn't check my 3.8.1 version -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|rel_3_6 |master --- Comment #1 from Owen Leonard <oleonard@myacpl.org> --- Confirmed in master. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 --- Comment #2 from M. Tompsett <mtompset@hotmail.com> --- Under 3.8.1 is just below the analytics section that was added with tabs, instead of spaces. Line 260. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 --- Comment #3 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 10566 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10566&action=edit Undefined $items->{'materials'} fails a string check, so drop it. Dropping the "ne ''" was more elegant. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10566|0 |1 is obsolete| | --- Comment #4 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 10577 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10577&action=edit Bug 8175 - Missing defined check in catalogue/details.pl Actually, just dropping the "ne ''" was better. In some cases $item->{'materials'} isn't defined. Any value is true, so no need for comparison. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |chris@bigballofwax.co.nz -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Ian Walls <koha.sekjal@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |koha.sekjal@gmail.com --- Comment #5 from Ian Walls <koha.sekjal@gmail.com> --- what about the case where $items->materials = 0? Wouldn't that assess to false, and thus not display? Zero could be a valid entry, especially if the items.materials field is linked to an authorised value list in the framework. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 --- Comment #6 from M. Tompsett <mtompset@hotmail.com> --- I see the validity of your point, Ian. What is desired is: NULL produce false trim()='' produce false everything else true (0,"0", and everything non-zero in string or number nature). I'll do a patch up later, if someone else doesn't beat me to it first. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Missing defined check in |Check for something in |catalogue/details.pl |materials field fails in | |catalogue/details.pl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10577|0 |1 is obsolete| | --- Comment #7 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 10620 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10620&action=edit Fixed the undefined issue and the intent of the ne '' Addressed Ian's concerns for the 0 or '0' case while correcting the NULL problem that triggered my looking at this code, and cleaning up the intent of the "ne ''" which is clearly only to generate a materials column in the output if there is something printable. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 --- Comment #8 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 11496 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11496&action=edit Bug 8175 - Check for something in materials field fails in catalogue/details.pl The intent was clearly to know if the materials column should be displayed or not. As such, a defined check handles the NULL, 0, and '0' cases. The string match for /\S/ handles the intent of only displaying the column if is there is text in the materials field. Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10620|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |julian.maurice@biblibre.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m --- Comment #9 from Jonathan Druart <jonathan.druart@biblibre.com> --- Hi Mark, Could you resubmit your patch after updating your commit message please ? (clarify "As such, a defined check handles the NULL, 0, and '0' cases.") -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion CC| |mtj@kohaaloha.com --- Comment #10 from Mason James <mtj@kohaaloha.com> --- (In reply to comment #9)
Hi Mark,
Could you resubmit your patch after updating your commit message please ? (clarify "As such, a defined check handles the NULL, 0, and '0' cases.")
status modded to 'in discussion'... -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 --- Comment #11 from M. Tompsett <mtompset@hotmail.com> --- Okay, I finally have time to get back to this. I first found this because my materials fields were all NULL. mysql -u {kohauser} -p
use {koha database}; select distinct materials from items; +-----------+ | materials | +-----------+ | NULL | +-----------+ 1 row in set (0.17 sec)
So, I fixed the bug, and then Ian piped up about 0. So, I decided to break it back to normal and test BEFORE the patch, and AFTER the patch. TESTING ====== 1) Log into Staff Client 2) Do a search to find an item 3) Click an item, and we should be at the detail.pl page for a biblionumber (eg. 19158). CASE NULL ========= 4) Go back to mysql and: update items SET materials=NULL where biblionumber=19158; 5) Go back to browser and refresh: There is no "Materials Specified" column displayed. 6) Check koha-error_log [Sun Sep 02 16:08:38 2012] [error] [client 192.168.100.2] [Sun Sep 2 16:08:38 2012] detail.pl: Use of uninitialized value in string ne at /usr/share/koha/intranet/cgi-bin/catalogue/detail.pl line 255., referer: https://.../cgi-bin/koha/catalogue/search.pl?q=the CASE 0 ====== 7) Go back to mysql and: update items SET materials='0' where biblionumber=19158; 8) Go back to browser and refresh: There is a "Materials Specified" column with 0 displayed in it. 9) Check error log again. Nothing new. CASE '' ======= 10) Go back to mysql and: update items SET materials='' where biblionumber=19158; 11) Go back to browser and refresh: There is no "Materials Specified" column displayed. 12) Check error log again. Nothing new. CASE ' ' ======== 13) Go back to mysql and: update items SET materials='0' where biblionumber=19158; 14) Go back to browser and refresh: There is a "Materials Specified" column with nothing visible in it. 15) Check error log again. Nothing new. CASE 'blah' ====== 7) Go back to mysql and: update items SET materials='blah' where biblionumber=19158; 8) Go back to browser and refresh: There is a "Materials Specified" column with blah displayed in it. 9) Check error log again. Nothing new. There are two things wrong: 1) the generated error, which caused me to find this bug, and 2) showing a column for no apparent reason. | CASE | Defined | =~ /\S/ | $var | ne '' | Should Display? | Display? | +========+=========+=========+======+=======+=================+==========+ | NULL | 0 | Error | 0 | Error | 0 | 0 | | '' | 1 | 0 | 0 | 0 | 0 | 0 | | '0' | 1 | 1 | 0 | 1 | 1 | 1 | | ' ' | 1 | 0 | 1 | 1 | 0 | 1 | | 'blah' | 1 | 1 | 1 | 1 | 1 | 1 | +========+=========+=========+======+=======+=================+==========+ The above table shows 1/0 values for potential replacement code or existing code options, and whether or not it displays correctly. Patch and repeat the test cases. Once you are finished, you may wish to set your materials value back to what it was. Seeing as materials is a 'text' type, the 0 vs. '0' difference is irrelevant. But I hope this table and test plan explains what I was thinking when I wrote those comments. I'll have patches up shortly. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Check for something in |Checking the materials |materials field fails in |field generates error or |catalogue/details.pl |displays output incorrectly | |in catalogue/details.pl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Checking the materials |items.materials check logs |field generates error or |error or displays |displays output incorrectly |incorrectly in details.pl |in catalogue/details.pl | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #11496|0 |1 is obsolete| | --- Comment #12 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 11937 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11937&action=edit items.materials=NULL triggers error, =' ' displays empty column The patches for 3.6.x, 3.8.x, and master seem identical with some line shifts. So if it fails to apply to one of them let me know. This was generated off master. I tested the change on our 3.6.3 system. I have put a detailed discussion with test cases in a previous comment. Sorry this took so long to get back to. I hope everything is much clearer. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |mtompset@hotmail.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #11937|0 |1 is obsolete| | --- Comment #13 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 11956 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11956&action=edit Bug 8175 - check logs error or displays incorrectly in details.pl Changed "$item->{'materials'} ne ''" to "defined($item->{'materials'}) && $item->{'materials'} =~ /\S/" in if condition to prevent error when it is NULL, and to properly capture the intent of printing if there is something visible. Cases tested include NULL, '0', '', ' ', and 'blah' by using UPDATE items SET materials=NULL where biblionumber=19158; where the biblionumber was chosen randomly, because only NULL was in the items.materials field. The NULL case triggers an error, but it does display correctly. The ' ' case displays an apparently empty column, which does not seem to be the intent of the flag that is being set. This is why a simple $var check is not sufficient. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|master |unspecified --- Comment #14 from M. Tompsett <mtompset@hotmail.com> --- This bug still exists in 3.6.x, 3.8.x, and master. That's why I changed the version to unspecified. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |paul.poulain@biblibre.com --- Comment #15 from Paul Poulain <paul.poulain@biblibre.com> --- QA comment * tiny patch, widely and wisely explained passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master Version|unspecified |rel_3_8 --- Comment #16 from Paul Poulain <paul.poulain@biblibre.com> --- Patch pushed to master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 --- Comment #17 from Chris Cormack <chris@bigballofwax.co.nz> --- I cant seem to find this in master. Has it not been pushed yet? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 --- Comment #18 from Paul Poulain <paul.poulain@biblibre.com> --- (In reply to comment #17)
I cant seem to find this in master. Has it not been pushed yet?
forgot to push to kc.org repo before leaving. pushed now -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8175 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable --- Comment #19 from Chris Cormack <chris@bigballofwax.co.nz> --- Pushed to 3.8.x, will be in 3.8.6 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org