On bug 29844 we decided to remove wantarray from Koha::Objects->search. Reviewing the difference occurrences I found some unnecessary uses of ->as_list, where iterators should be used instead.
Created attachment 129346 [details] [review] Bug 29859: Use iterator instead of as_list On bug 29844 we decided to remove wantarray from Koha::Objects->search. Reviewing the difference occurrences I found some unnecessary uses of ->as_list, where iterators should be used instead. This patch only removes the obvious places, not the tricky ones.
There is a non-squashed version on https://gitlab.com/joubu/Koha/-/tree/bug_29859
Created attachment 129350 [details] [review] Bug 29859: Use iterator instead of as_list On bug 29844 we decided to remove wantarray from Koha::Objects->search. Reviewing the difference occurrences I found some unnecessary uses of ->as_list, where iterators should be used instead. This patch only removes the obvious places, not the tricky ones.
Created attachment 130102 [details] [review] Bug 29859: Use iterator instead of as_list On bug 29844 we decided to remove wantarray from Koha::Objects->search. Reviewing the difference occurrences I found some unnecessary uses of ->as_list, where iterators should be used instead. This patch only removes the obvious places, not the tricky ones. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment on attachment 130102 [details] [review] Bug 29859: Use iterator instead of as_list Review of attachment 130102 [details] [review]: ----------------------------------------------------------------- We introduce a few .count calls in templates here.. which will add a DB hit per call unless I'm mistaken? Now, I don't think a count call has much impact on performance, but it may be worth taking into consideration at some point.. for the pattern [% IF whatevers.count %]boiler[% FOREACH whatever IN whatever %]list of stuff[% END %][% END %] we could perhaps just add the boiler in the first iteration of the loop using the 'first' or 'index' methods of the TT iterator? instead of testing for count explicitly? ::: Koha/Holds.pm @@ +122,5 @@ > + { > + columns => ['itemnumber'], > + collapse => 1, > + } > + )->get_columns('itemnumber'); get_column vs get_columns
Created attachment 130148 [details] [review] Bug 29859: Use iterator instead of as_list On bug 29844 we decided to remove wantarray from Koha::Objects->search. Reviewing the difference occurrences I found some unnecessary uses of ->as_list, where iterators should be used instead. This patch only removes the obvious places, not the tricky ones. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 130149 [details] [review] Bug 29859: (QA follow-up) Minor typo correction We introduced a tiny bug with a typo in the patchset, the accessor method is called 'get_column' not 'get_columns'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
A nice improvement.. I fixed the minor typo with a followup and moved the discussion around .count vs loop variables to a new bug. QA scripts are happy and I couldn't find any errors in the UI. Passing QA
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Created attachment 130480 [details] [review] Bug 29859: (follow-up) Use iterator instead of as_list
Created attachment 130483 [details] [review] Bug 29859: Fix currencies display Oops!
> Bug 29859: (follow-up) Use iterator instead of as_list > Bug 29859: Fix currencies display Pushed to master
Created attachment 132972 [details] [review] Bug 29859: Fix club/enroll
> Bug 29859: Fix club/enroll Pushed to master
Created attachment 132983 [details] [review] Bug 29859: Fix BatchUpdateAuthority Use of inherited AUTOLOAD for non-method Koha::BackgroundJob::BatchUpdateAuthority::ModAuthority() is no longer allowed
> Bug 29859: Fix BatchUpdateAuthority Pushed to master
(In reply to Fridolin Somers from comment #16) > > Bug 29859: Fix BatchUpdateAuthority > Pushed to master I obsoleted this.(In reply to Fridolin Somers from comment #16) > > Bug 29859: Fix BatchUpdateAuthority > Pushed to master Hum? This patch was marked as obsolete, it shouldn't have been pushed. It was a mistake to attach it here. See bug 30461.
Comment on attachment 132983 [details] [review] Bug 29859: Fix BatchUpdateAuthority Ah ok, reverted from master