If to_api is requested to embed the count on a relation that might be undef (for example, if the relation is not just a FK on another table but linking table: sub current_item_level_holds { my ($self) = @_; my $items_rs = $self->_result->aqorders_items; my @item_numbers = $items_rs->get_column('itemnumber')->all; return unless @item_numbers; my $biblio = $self->biblio; return unless $biblio; return $biblio->current_holds->search( { itemnumber => { -in => \@item_numbers } } ); } it is possible to get exceptions for calling ->count on an undefined object. We should catch this situation and return zero.
Created attachment 101560 [details] [review] Bug 24965: Regression tests To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object.t => FAIL: Tests fail with "Can't call method "count" on an undefined value..." Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 101561 [details] [review] Bug 24965: Make Koha::Object->to_api handle undef counts In some cases where we write a class method that is not directly tied to a DBIC relationship (maybe it should) we are following the following pattern: my $related_object = Koha::RelatedObjects->find($params,$attributes); return unless $related_object; return $related_object->some_iterator_on_other_class; While this is not highly consistent (design-wise, because it would be great if we could return an empty iterator instead of undef) we use this broadly and consistently. The to_api method +count handling is not failsafe to those undef cases. And we should make it return 0 instead of trying to get ->count on undefined objects. This patch makes to_api fallback to 0 when it is requested to embed a count. To test: 1. Apply the regression test patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object.t => FAIL: Test fails with an error about ->count on undefined variable. 3. Apply this patch 4. Repeat 2 => SUCCESS: The patch adds the simple fallback to 0 on counts 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 102154 [details] [review] Bug 24965: Regression tests To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object.t => FAIL: Tests fail with "Can't call method "count" on an undefined value..." Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 102155 [details] [review] Bug 24965: Make Koha::Object->to_api handle undef counts In some cases where we write a class method that is not directly tied to a DBIC relationship (maybe it should) we are following the following pattern: my $related_object = Koha::RelatedObjects->find($params,$attributes); return unless $related_object; return $related_object->some_iterator_on_other_class; While this is not highly consistent (design-wise, because it would be great if we could return an empty iterator instead of undef) we use this broadly and consistently. The to_api method +count handling is not failsafe to those undef cases. And we should make it return 0 instead of trying to get ->count on undefined objects. This patch makes to_api fallback to 0 when it is requested to embed a count. To test: 1. Apply the regression test patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object.t => FAIL: Test fails with an error about ->count on undefined variable. 3. Apply this patch 4. Repeat 2 => SUCCESS: The patch adds the simple fallback to 0 on counts 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
What is the problematic situation in current_item_level_holds? no @item_numbers, or no $biblio? The 'return unless @item_numbers' can be removed, you will get an empty iterator in that case. Do you have other examples?
Waiting for an answer.
(In reply to Jonathan Druart from comment #6) > Waiting for an answer. In my opinion it feels dirty. We should have a way to effectively build an 'empty iterator' instead of actually performing a search to the DB, knowing it needs to return 0 results. Damn you make me work extra all the time
*** This bug has been marked as a duplicate of bug 25297 ***