[Koha-devel] Resultsets vs. list context

[Tomas Cohen Arazi]

Hi, let me tell you a short and (technical but) interesting story. Most of
you might know about this, but I found it could be useful to know and/or to
refresh.

DBIC has a nice property that our Koha::Objects inherited (obviously): you
can chain calls like this:

    Koha::Patrons->search( $filter_1 )->search( $filter_2 )->search(
$filter_3 )->...;

We recently agreed (not sure there is a coding guideline) that if we wanted
to  provide high-level methods to encapsulate such filters, we would call
those methods 'filter_by_*' unless the context suggests a better name. So
we could, in theory do something like:

    my $cool_old_books = Koha::Biblios->search( $some_initial_filter )

 ->filter_by_older_than({ years => 30 })

 ->filter_by_coolness();

So far, so good :-D

But then, we have another (cool) method in Koha::Objects: _new_from_dbic.
This method allows us to, given a DBIC relationship, convert the linked
resultset into the proper Koha::Object(s)-based class. For example:

    my $patron_checkouts = $patron->checkouts;

in this case, the actual code is pretty simple thanks to _new_from_dbic:

sub checkouts {
    my ($self) = @_;
    my $checkouts = $self->_result->issues;
    return Koha::Checkouts->_new_from_dbic( $checkouts );
}

Methods like ->search behave consistently to what DBIC does, and as such,
they honor list context. This is:

    my $scalar = Koha::Patrons->search(filter);
    my @list = Koha::Patrons->search($filter);

will do what you think: $scalar will be an object, representing a
resultset, and @list will be the actually fetched list of results from the
search, as a list.

But there's a catch. And we need to be aware of that and there is an
ongoing discussion about it: _new_from_dbic doesn't honor list context [1].
This means when you code, you need to go look if the method you're calling
is _new_from_dbic-based, or ->search based [2]. This is not cool.

    @checkouts = $patron->checkouts; # doesn't do what you expect!
    @checkouts = $patron->checkouts->filter_by_overdue; # this does if
->search based.

Our codebase is full of places in which we either call ->as_list in our
resultsets (because something exploded if we didn't, for example):

    my @checkouts = $patron->checkouts->as_list;

or in which we explicitly call `scalar the_thing'  so we are sure things
are evaluated in scalar context because we want the resultset instead (for
example, when passing params to a template):

    $template->param( checkouts => scalar $patron->checkouts );

I proposed we make ->_new_from_dbic and ->empty honor list context and
hence making our methods consistent (can be seen by the referenced bugs).
But we found a nasty problem: calling methods in Template::Toolkit
internally assigns them to a global stash which is implemented as a hash
(a.k.a. list context :-D). So, you will find that 'some' things are
calculated on the controller (.pl) scripts and then passed to the templates
(to overcome this), and  some others are calculated directly in the
templates. Now you know the reason we've introduced those things over the
years in the codebase.

So, making _new_from_dbic honor list context, opens a big can of worms in
the template front. We explored the idea of using a different
Template::Toolkit stash,  that honors scalar context, but the author has
marked it as experimental, and it would still require changing many things
in the templates. The tradeoff doesn't lean towards that path.

The conclusion so far is that we should drop support for list context.
Hence, my suggestion as developer and QA team member is:

TL;DR

Make use of ->as_list when you have a resultset really need list context
like in:

foreach my $item ( @{$items->as_list} ) { ... }
grep { filter($_) } @{@libraries->as_list}
my @biblios = $biblios->as_list;

and so on. This will save you some headaches (in some cases) and will make
your code survive this upcoming change. We should start enforcing this in
the QA step as well, so adjust your code for this. Once we remove
wantarray, things will explode on their own.

Best regards

[1] https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883
[2] There is also Koha::Objects->empty, actually. That presents the same
problem as _new_from_dbic.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871

--
Tomás Cohen Arazi
Theke Solutions (http://theke.io)
✆ +54 9351 3513384
GPG: B2F3C15F
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.koha-community.org/pipermail/koha-devel/attachments/20220106/5d5a5dd6/attachment.htm>