Bug 32472

Summary: [21.11 CRASH] The method Koha::Item->count is not covered by tests
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: arthur.suzuki, emmi.takkinen, jacob.omara, tomascohen
Version: 21.11   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29844
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.18,22.11.04
Attachments: Bug 32472: Fix wrong POD for bookseller->subscriptions
Bug 32472: [21.11] Force scalar context on acqorder->items
Bug 32472: Fix wrong POD for bookseller->subscriptions
Bug 32472: [21.11] Force scalar context on acqorder->items

Description Marcel de Rooy 2022-12-15 07:11:08 UTC
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.
Comment 1 Marcel de Rooy 2022-12-15 07:23:26 UTC
(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..
Comment 2 Marcel de Rooy 2022-12-15 07:56:55 UTC
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
Comment 3 Marcel de Rooy 2022-12-15 08:46:50 UTC
> 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.
Comment 4 Marcel de Rooy 2022-12-15 09:39:17 UTC
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
Comment 5 Marcel de Rooy 2022-12-15 09:42:13 UTC
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.
Comment 6 Marcel de Rooy 2022-12-15 09:47:47 UTC
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.
Comment 7 Marcel de Rooy 2022-12-15 10:36:57 UTC
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>
Comment 8 Marcel de Rooy 2022-12-15 10:37:01 UTC
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>
Comment 9 Emmi Takkinen 2022-12-28 11:16:56 UTC
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>
Comment 10 Emmi Takkinen 2022-12-28 11:17:31 UTC
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>
Comment 11 Marcel de Rooy 2023-02-16 08:57:35 UTC
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 :)
Comment 12 Tomás Cohen Arazi 2023-03-06 14:06:55 UTC
Trivial POD fix pushed to master.
Comment 13 Arthur Suzuki 2023-03-06 14:45:12 UTC
applied to 21.11.x for 21.11.18
Comment 14 Jacob O'Mara 2023-03-07 18:33:56 UTC
POD fix pushed to stable
Comment 15 Emmi Takkinen 2023-06-16 06:09:32 UTC
*** Bug 30321 has been marked as a duplicate of this bug. ***