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
Confirmed in master.
Under 3.8.1 is just below the analytics section that was added with tabs, instead of spaces. Line 260.
Created attachment 10566 [details] [review] Undefined $items->{'materials'} fails a string check, so drop it. Dropping the "ne ''" was more elegant.
Created attachment 10577 [details] [review] 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>
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.
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.
Created attachment 10620 [details] [review] 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.
Created attachment 11496 [details] [review] 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>
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.")
(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'...
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.
Created attachment 11937 [details] [review] 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.
Created attachment 11956 [details] [review] 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>
This bug still exists in 3.6.x, 3.8.x, and master. That's why I changed the version to unspecified.
QA comment * tiny patch, widely and wisely explained passed QA
Patch pushed to master
I cant seem to find this in master. Has it not been pushed yet?
(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
Pushed to 3.8.x, will be in 3.8.6