[Koha-devel] Resultsets vs. list context

[dcook at prosentient.com.au]

That was really interesting. Sounds like a good call to me. I’ve never really been a fan of ‘wantarray’ black magic anyway. 

 

David Cook

Senior Software Engineer

Prosentient Systems

Suite 7.03

6a Glen St

Milsons Point NSW 2061

Australia

 

Office: 02 9212 0899

Online: 02 8005 0595

 

From: Koha-devel <koha-devel-bounces at lists.koha-community.org> On Behalf Of Tomas Cohen Arazi
Sent: Friday, 7 January 2022 3:21 AM
To: koha-devel <koha-devel at lists.koha-community.org>
Subject: [Koha-devel] Resultsets vs. list context

 

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/20220110/b4cc5d4e/attachment.htm>