Bug 32472

(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. ***