Opening the holds to pull list page can crash with the following logged error: SQL::Abstract::puke(): [SQL::Abstract::__ANON__] Fatal: SQL::Abstract before v1.75 used to generate incorrect SQL when the -NOT IN operator was given an undef-containing list: !!!AUDIT YOUR CODE AND DATA!!! (the upcoming Data::Query-based version of SQL::Abstract will emit the logically correct SQL instead of raising this exception) at /usr/share/koha/lib/Koha/Objects.pm line 145 I've traced this to Koha::Holds->get_items_that_can_fill(). Among other things, this routine fetches a list of item types that cannot fill holds, i.e., cases where the 'holdallowed' rule value is 'not_allowed'. However, if your default rule is to not allow holds, the @hold_not_allowed_itypes list will end up including an entry whose value is undef. When passed to the item query, doing the -not_in filter on itype will result in the crash listed above. A quick fix might be to filter out undef from that list. That will fix the crash, but could lead to incorrect results if the default policy is to deny holds for all item types except for ones that are going to be specifically permitted.
Created attachment 127859 [details] [review] Bug 29087: Prevent filter_by_for_hold to crash if default holdallowed is not_allowed
This seems to be the way to go, but I don't think it's ready for integration. We should have the logic moved to Koha::ItemTypes and add test coverage. Will try and come back to this later.
Galen, thanks for reporting this bug! Are you seeing this in master or stable?
Our library system is seeing this bug as well. Had a lot of staff reporting the error message to me today.
We have some libraries seeing this now as well, the rule mentioned here doesn't seem to be the culprit, but the error is the same.
Which version? Did you try the patch?
I've tried patching a customer site with this and it works OK. Happy to sign off (even though the bug isn't requesting it yet....)
Should we move this to NSO or SO even?
We see this occasionally on customer sites too.. we've even got it backported in a couple of cases I think.
(In reply to Katrin Fischer from comment #8) > Should we move this to NSO or SO even? No, see comment 2.