This is only possible under 21.11 (before bug Bug 29844 - Remove uses of wantarray in Koha::Objects) in templates where constructions like items.count etc are used on passed objects. Template Toolkit does evaluate these expressions in LIST context while we probably expected scalar context as when chaining in Perl. This particular crash comes from acqui/orderreceive.pl (and template): [% IF ( order.items.count ) %] In this case we should either use the Scalar plugin or - simply - use size here from TT itself. Note that this could be applied to master too since it is probably cheaper to stay in TT than call Koha::Objects/DBI here.
(In reply to Marcel de Rooy from comment #0) > Note that this could be applied to master too since it is probably cheaper > to stay in TT than call Koha::Objects/DBI here. No, it cannot ! The resultset is one. So always true..
Quite interesting to see how many occurrences are really affected. If you pass items with say $items = Koha::Items->search, you will be fine since you passed items as a scalar to the template. So a lot of [plurals].count are just fine. But what about order.items.count? Generally this comes from passing order as a scalar but the next part "items" is evaluated in list context in TT. So those might be wrong.. A git grep like git grep -E "\w+\.\w+\.count " will give such candidates. (Note that you could also call other methods than count..] This gives me just a bunch for opac templates in 21.11. The ones I checked seem to be fine: item.ratings.count comes from $issue->{ratings} so scalar context. But quite a bit more for intranet, but most are in a specific context: club enrollments, return claims, credit type library limits, course reserves, stock rotation quota. Just mentioning a few: includes/cat-toolbar.inc: [% ELSIF ( biblio.subscriptions.count ) %] includes/clubs-table.inc: [% c.club_enrollments.count | html %] modules/acqui/showorder.tt: [% order.claims.count | html %] modules/cataloguing/moveitem.tt: [% IF from_biblio.items.count == 0 && CAN_user_editcatalogue_edit_catalogue %]modules/members/discharge.tt: [% IF patron.holds.count %] etc
> includes/cat-toolbar.inc: [% ELSIF ( > biblio.subscriptions.count ) %] This one is fine. return $self->{_subscriptions} (scalar) > modules/cataloguing/moveitem.tt: [% IF from_biblio.items.count == > 0 && CAN_user_editcatalogue_edit_catalogue %] It is a scalar too: my $items_rs = $self->_result->items; club enrollments too: return scalar Koha::Club::Enrollments->search( { club_id => $self->id(), date_canceled => undef } ); return_claims: my $return_claims = $self->_result->return_claims_borrowernumbers; (SCALAR) FINE claims FINE my $claims_rs = $self->_result->aqorders_claims; So it is not so bad as it looked. But sub items in Koha/Acquisition/Order.pm is a bad example: with return Koha::Items->search({ itemnumber => \@itemnumbers }); So we could git grep on git grep -E "return Koha::\w+->search" in the Koha directory. See next comment.
git grep -E "return Koha::\w+->search" Acquisition/Bookseller.pm: return Koha::Subscriptions->search( { aqbooksellerid => $self->id } ); => sub subscriptions This could be LIST context. So needs adjustment in 21.11 ? Acquisition/Order.pm: return Koha::Items->search({ itemnumber => \@itemnumbers }); => see previous comment WRONG ArticleRequests.pm: return Koha::ArticleRequests->search_limited($params); ArticleRequests.pm: return Koha::ArticleRequests->search_limited( $params ); ArticleRequests.pm: return Koha::ArticleRequests->search_limited( $params ); ArticleRequests.pm: return Koha::ArticleRequests->search_limited( $params ); ArticleRequests.pm: return Koha::ArticleRequests->search_limited( $params ); => sub requested, pending, processing, completed, canceled => article-requests.tt: Pending (<span id="ar_pending_count">[% article_requests_pending.count | html %]</span>) => article-requests.tt: Processing (<span id="ar_processing_count">[% article_requests_processing.count | html %]</span>) => Are passed with scalar to template. FINE OK Biblio.pm: return Koha::Libraries->search( => sub pickup_locations looks WRONG but no direct template calls? OK Biblio.pm: return Koha::Checkouts->search( { "item.biblionumber" => $self->id }, => sub current_checkouts and ALSO old_checkouts NO template calls OK Biblio.pm: return Koha::Items->search( { itemnumber => { -in => \@itemnumbers } } ); => host_items looks wrong NO template_calls OK Biblios.pm: return Koha::Libraries->search( => pickup_locations SEE above OK Holds.pm: return Koha::Items->search( => get_items_that_can_fill, waiting, unfilled are candidates / No template calls found TODO Illrequest.pm: return Koha::AuthorisedValues->search( OK ItemType.pm: return Koha::ItemTypes->search_with_localization({ parent_type => $self->itemtype }); => children_with_localization OK Library.pm: return Koha::Libraries->search({ branchcode => { '-in' => [ keys %seen ] } }); => get_hold_libraries, validate_hold_sibling TODO Library/Group.pm: return Koha::Libraries->search( TODO SMS/Provider.pm: return Koha::Patrons->search( { sms_provider_id => $self->id } )->count(); TODO SMTP/Server.pm: return Koha::Libraries->search( { branchcode => { -in => \@library_ids } } ); => libraries OK Subscription.pm: return Koha::Patrons->search({ borrowernumber => {-in => \@borrowernumbers } }); * subscribers OK Template/Plugin/AuthorisedValues.pm: return Koha::AuthorisedValues->search_with_library_limits( * GetAuthValueDropbox, GetDescriptionsByKohaField TODO Template/Plugin/Desks.pm: return Koha::Desks->search(); OK Template/Plugin/ItemTypes.pm: return Koha::ItemTypes->search_with_localization->unblessed; TODO Util/StockRotation.pm: return Koha::Libraries->search( OK Virtualshelves.pm: return Koha::Virtualshelves->search( * get_shelves_containing_record
Preliminary conclusion: Luckily only found a few occurrences that really seem to be wrong in 21.11. The combination of weird TT list context evaluation of chained expressions, passing objects to templates and potentially returning lists instead of a result set (before bug 29844) based on wantarray is a minefield.
Acquisition/Bookseller.pm: return Koha::Subscriptions->search( { aqbooksellerid => $self->id } ); => sub subscriptions This could be LIST context. So needs adjustment in 21.11 ? This one is also fine, since we do not call it in templates. We call biblio.subscriptions but not on a bookseller.
Created attachment 144609 [details] [review] Bug 32472: Fix wrong POD for bookseller->subscriptions You should not call this in list context anymore after bug 29844. Test plan: Check pod of Bookseller. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 144610 [details] [review] Bug 32472: [21.11] Force scalar context on acqorder->items This resolves a crash like The method Koha::Item->count is not covered by tests! on the template construction [% IF ( order.items.count ) %] in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt. Test plan (21.11): Receive an order line under this condition: quantityreceived>0 with items attached to the order line. Note: Should be possible, comes from production logs. Look for list context on ->items called from acqorder. The result of git grep "\->items" is too wide but we could theoretically miss an occurence with git grep "order.*\->items". With order.* we do catch $new_order_object->items.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 144860 [details] [review] Bug 32472: Fix wrong POD for bookseller->subscriptions You should not call this in list context anymore after bug 29844. Test plan: Check pod of Bookseller. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Created attachment 144861 [details] [review] Bug 32472: [21.11] Force scalar context on acqorder->items This resolves a crash like The method Koha::Item->count is not covered by tests! on the template construction [% IF ( order.items.count ) %] in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt. Test plan (21.11): Receive an order line under this condition: quantityreceived>0 with items attached to the order line. Note: Should be possible, comes from production logs. Look for list context on ->items called from acqorder. The result of git grep "\->items" is too wide but we could theoretically miss an occurence with git grep "order.*\->items". With order.* we do catch $new_order_object->items.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Look at both patches, they are extremely trivial. There is a signoff from Emmi (thx for that!). Moving to the attention of the RM. The second patch is not needed on master or 22.x.x. Marked for 21.11 :)
Trivial POD fix pushed to master.
applied to 21.11.x for 21.11.18
POD fix pushed to stable
*** Bug 30321 has been marked as a duplicate of this bug. ***