When in list context, it assigns an array with an interator inside. To test: my @patrons = Koha::Patrons->new->empty; Dumper(@patrons); => FAIL: prints [ Koha::Patrons... ]
Created attachment 123876 [details] [review] Bug 28871: Regression test
Created attachment 123877 [details] [review] Bug 28871: Make Koha::Objects->empty work on list context This patch makes Koha::Objects->empty work on list context, by making it return the $self->search call. To test: 1. Apply the regression test patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Objects.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Note: this patch has the side effect of making Koha::Objects->new->empty to perform a search on the DB. I'm not sure how to solve this because the cache needs to be set after the resultset is generated. It feels like a cleaner approach (i.e. not using the set_cache method).
Created attachment 123878 [details] [review] Bug 28871: (follow-up) Remove caching This patch removes the use of set_cache, as it gets ineffective with this patchset. To test: 1. Without any patches applied do: $ docker exec -ti koha_db_1 bash m$ mysql -ppassword > SET GLOBAL general_log=1; > SET GLOBAL general_log_file='mariadb.log'; > \q m$ tail -f /var/lib/mysql/mariadb.log 2. On a separate terminal run: $ kshell k$ prove t/db_dependent/Koha/Objects.t => Notice that before the DELETE tests are run, there are some queries about borrowers and 0 = 1 3. Apply this patches 4. Repeat 2 => Notice there's an extra query This is only to highlight the side-effect of this change. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 123879 [details] [review] Bug 28871: Make Koha::Objects->empty work on list context This patch makes Koha::Objects->empty work on list context, by making it return the $self->search call. To test: 1. Apply the regression test patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Objects.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D
Created attachment 123925 [details] [review] Bug 28871: Regression test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123926 [details] [review] Bug 28871: Make Koha::Objects->empty work on list context This patch makes Koha::Objects->empty work on list context, by making it return the $self->search call. To test: 1. Apply the regression test patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Objects.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
No obvious regressions found, nice simple patch. Signing off.
QAing
BTW It seems to me that we do not even need to do a search, just create a resultset with new only. But this is merely a theoretical difference. There will be no db read since we set the cache. # $self = $self->search(\'0 = 1'); $self->_resultset()->set_cache([]);
Created attachment 123987 [details] [review] Bug 28871: Regression test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 123988 [details] [review] Bug 28871: Make Koha::Objects->empty work on list context This patch makes Koha::Objects->empty work on list context, by making it return the $self->search call. To test: 1. Apply the regression test patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Objects.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #9) > BTW It seems to me that we do not even need to do a search, just create a > resultset with new only. But this is merely a theoretical difference. There > will be no db read since we set the cache. > > # $self = $self->search(\'0 = 1'); > $self->_resultset()->set_cache([]); That ->search call will not trigger a db search. That's correct. It was added so further chaining always returns ah empty set.
Blocked by the discussion on bug 28883. IF we decide to remove wantarray it does not make sense to have this patch.
This fix is too simple and is correct regarding the current codebase.
(In reply to Tomás Cohen Arazi from comment #0) > To test: > my @patrons = Koha::Patrons->new->empty; > Dumper(@patrons); What is the practical use of doing so? I would prefer my @patrons; Was there some intention to use empty with chaining?
(In reply to Marcel de Rooy from comment #15) > (In reply to Tomás Cohen Arazi from comment #0) > > To test: > > my @patrons = Koha::Patrons->new->empty; > > Dumper(@patrons); > > What is the practical use of doing so? You should git grep '\->empty' The existence of ->empty is meant for consistency in results, and also to avoid querying the DB for empty results when we know in advance that we need an empty resultset. > I would prefer my @patrons; Right, but my example was too simple. Sometimes ->empty is called implicitly somewhere else, far from the code you're looking at. > Was there some intention to use empty with chaining? empty is there so you don't need to do things like: if ( $result->count > 0 ) { $result = $result->filter_by_something_else; } you just chain your queries and know it will work...
(In reply to Tomás Cohen Arazi from comment #16) > You should git grep '\->empty' Just say it :) Koha/Acquisition/Order.pm: return Koha::Holds->new->empty; => It returns a I<Koha::Holds> resultset. So that should be no longer the case in LIST context ? Koha/Biblio.pm: return Koha::Items->new->empty * return Koha::Items->new->empty unless C4::Context->preference('EasyAnalyticalRecords'); * return Koha::Items->search( { itemnumber => { -in => \@itemnumbers } } ); => Far fetched, but we might have empty list or empty result set ? Koha/Item.pm: return Koha::Libraries->new()->empty if $branchitemrule->{holdallowed} eq 'from_local_hold_group' && !$self->home_branch->validate_hold_sibling( {branchcode => $patron->branchcode} ); Koha/Item.pm: return Koha::Libraries->new()->empty if $branchitemrule->{holdallowed} eq 'from_home_library' && $self->home_branch->branchcode ne $patron->branchcode; => So pickup locations will return () or result set that could be empty ? Looks a bit inconsistent. Or do I miss something ;) > you just chain your queries and know it will work... Chain is scalar context. So no difference between current state and your patch?
In other words, please show me a better example where it does not work now in list context.
(In reply to Marcel de Rooy from comment #18) > In other words, please show me a better example where it does not work now > in list context. This sub: sub items_fetch { return Koha::Items->search(...); } will respect list context and do the appropriate thing i.e. my @items = items_fetch(); will perform the DB query, and have @items be assigned a list of Koha::Item objects. As ->empty is expected to be returned sometimes, and (why not) chained with other queries, it needs to behave the same as ->search. This is the ->empty counterpart of what I raised on bug 28883. Look at t/db_dependendent/Koha/Biblio.t:419 the addition of ->as_list there, is a consequence of the possibility Koha::Libraries->new->empty is returned in some scenarios. So Koha::Biblio->pickup_locations can honour list context sometimes, and sometimes not. With this patch the programmer can be sure ->pickup_locations has a predictable behavior and avoid coding for exceptional cases.
(In reply to Tomás Cohen Arazi from comment #19) > So Koha::Biblio->pickup_locations can honour list context sometimes, and > sometimes not. With this patch the programmer can be sure ->pickup_locations > has a predictable behavior and avoid coding for exceptional cases. That said, removing the use of wantarray entirely (with which I agree after the debate, and because of the tradeoffs, not for the behavior), would make this patch unnecessary. But that doesn't look like it is gonna happen soon.
Found in production: [2021/12/09 10:20:29] [ERROR] PUT /api/v1/holds/499056/pickup_location: unhandled exception (Koha::Exceptions::Object::MethodNotCoveredByTests)<<The method Ko ha::Libraries->branchcode is not covered by tests! Please, allow this patches in, until we decide about wantarray. It is weird not to fix an obvious bug because there's a long term refactoring being discussed...
(In reply to Tomás Cohen Arazi from comment #21) > Found in production: > > [2021/12/09 10:20:29] [ERROR] PUT /api/v1/holds/499056/pickup_location: > unhandled exception (Koha::Exceptions::Object::MethodNotCoveredByTests)<<The > method Ko > ha::Libraries->branchcode is not covered by tests! > > Please, allow this patches in, until we decide about wantarray. It is weird > not to fix an obvious bug because there's a long term refactoring being > discussed... I filed 29804 to avoid delays for the releases.
Let's close this one, and focus on removing the use of wantarray in Koha::Objects instead.