|
Description
Mark Tompsett
2012-07-03 12:18:11 UTC
*** Bug 8349 has been marked as a duplicate of this bug. *** Created attachment 10624 [details] [review] Fixed string substitution bug (8349) and better SQL building Fixes bug 8349 and 8350 as the if statement does affect the cases encountered by the substitution which was moved into it. Patch looks ok, but I'm not able to reproduce a bug. git grep tells me that the unique place where BreedingSearch is used is cataloguing/addbooks.pl. In cataloguing/addbooks.pl, if I search '0' or 0, BreedingSearch is not called, because of "if($query)" in script. If you want to allow searching for string '0', I think you should modify addbooks.pl as well. Sadly, I don't know how to reproduce it either. Though, perhaps: "HTTP 500 error from koha server while importing Z39.50records" on the mailing list will give clues? I was able to trigger this by: - login - "Cataloguing" (goes to addbooks.pl) - Do a cataloguing search for "98765432310" (shouldn't exist in the library) - check error logs for my 3.6.3: [Thu Aug 30 12:03:05 2012] [error] [client 192.168.100.2] [Thu Aug 30 12:03:05 2012] addbooks.pl: Use of uninitialized value $search in substitution (s///) at /usr/share/koha/lib/C4/Breeding.pm line 188., referer: https://.../cgi-bin/koha/cataloguing/addbooks.pl Put in patching equivalent to my 3.6.3 system. No errors! However, fixing Breeding.pm may not be the only problem. I noticed, while trying to think through the possibilities that there are issues in addbooks.pl The BreedingSearch call has either title=undef or isbn=undef. Is this a reasonable expectation? Additionally, BreedingSearch has three parameters: search, isbn, z3950random. By calling with only two parameters, z3950 is always undef. This means that the "else" always runs. And as Julian Maurice pointed out, there is only one program currently using the BreedingSearch. Perhaps an optimization of the function needs to be made too? I was thinking of a substitution for a different function, but I'm not so sure of that. Someone else more familiar with the addbooks.pl processing would need to make that call. Created attachment 11888 [details] [review] Prevents incorrect SQL statement generation in C4::Breeding resulting from an error on an undefined string substitution. I generated a 3.6.x, 3.8.x, and master patch. Login -> Cataloguing -> 9876543210 (or some other 10 or 13 digit number not in your library's catalog) -- (catalogue search) Without the patch, there should be an error in C4::Breeding. Created attachment 11889 [details] [review] (3.8.x) Prevents incorrect SQL statement generation in C4::Breeding resulting from an error on an undefined string substitution. I generated a 3.6.x, 3.8.x, and master patch. Login -> Cataloguing -> 9876543210 (or some other 10 or 13 digit number not in your library's catalog) -- (catalogue search) Without the patch, there should be an error in C4::Breeding. Created attachment 11890 [details] [review] (master) Prevents incorrect SQL statement generation in C4::Breeding resulting from an error on an undefined string substitution. I generated a 3.6.x, 3.8.x, and master patch. Login -> Cataloguing -> 9876543210 (or some other 10 or 13 digit number not in your library's catalog) -- (catalogue search) Without the patch, there should be an error in C4::Breeding. Created attachment 11891 [details] [review] Bug 8350: warning in logs when searching for nonexistent ISBN Searching for a 10 or 13 digit numeric string that does not exist in one's catalog will fail to affect SQL statement correctly in C4::Breeding. Moved string substitution, which was triggering error when search was undefined, and fixed if statements accordingly. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> QA comment: * small patch, tiny side effect risk * passes koha-qa.pl fixes the error & cataloguing still work after patch passed QA Patch pushed to master Pushed to 3.8.x will be in 3.8.6 |