Description
Tomás Cohen Arazi
2021-08-19 13:35:18 UTC
Created attachment 123976 [details] [review] Bug 28883: Regression tests Created attachment 123977 [details] [review] Bug 28883: Make Koha::Objects->_new_from_dbic honour list context This patch makes the _new_from_dbic method return a list in list context. It does so by leveraging on the existing Koha::Objects->_wrap internal method that _does the right thing_. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Objects.t => FAIL: Boo! Tests fail! The list is not empty, and it contains... a resultset intead of empty or individual items. 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! Things work as expected! 5. Run: k$ probe t/db_dependent/Koha/Biblio.t => SUCCESS: Tests pass! It uses the chaining: $biblio->items->as_list and doesn't break. It implies that the change won't break any Koha place in which ->as_list is being used on a resultset created by _new_from_dbic 6. Sign off :-D Created attachment 123980 [details] [review] Bug 28883: Regression tests Signed-off-by: David Nind <david@davidnind.com> Created attachment 123981 [details] [review] Bug 28883: Make Koha::Objects->_new_from_dbic honour list context This patch makes the _new_from_dbic method return a list in list context. It does so by leveraging on the existing Koha::Objects->_wrap internal method that _does the right thing_. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Objects.t => FAIL: Boo! Tests fail! The list is not empty, and it contains... a resultset intead of empty or individual items. 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! Things work as expected! 5. Run: k$ probe t/db_dependent/Koha/Biblio.t => SUCCESS: Tests pass! It uses the chaining: $biblio->items->as_list and doesn't break. It implies that the change won't break any Koha place in which ->as_list is being used on a resultset created by _new_from_dbic 6. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> QAing is( $items_scalar->count, 2, 'No items in resultset' ); Hmm that description ;) Seems to work as it is, but I am just wondering if we should force scalar context within the map statement of _wrap ? To prevent perl one day (another version etc) guessing list context and creating an nice endless loop. So making it: my @objects = map { scalar $self->object_class->_new_from_dbic( $_ ) } etc Instead of: sub _new_from_dbic { my ( $class, $resultset ) = @_; + if ( wantarray ) { + return $class->_wrap( $resultset->all() ); + } my $self = { _resultset => $resultset }; bless( $self, $class ); [..] sub _wrap { my ( $self, @dbic_rows ) = @_; my @objects = map { $self->object_class->_new_from_dbic( $_ ) } @dbic_rows; return @objects; } Adding an optional follow-up for it too. Please let me know what you think. Created attachment 123992 [details] [review] Bug 28883: Regression tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 123993 [details] [review] Bug 28883: Make Koha::Objects->_new_from_dbic honour list context This patch makes the _new_from_dbic method return a list in list context. It does so by leveraging on the existing Koha::Objects->_wrap internal method that _does the right thing_. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Objects.t => FAIL: Boo! Tests fail! The list is not empty, and it contains... a resultset intead of empty or individual items. 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! Things work as expected! 5. Run: k$ probe t/db_dependent/Koha/Biblio.t => SUCCESS: Tests pass! It uses the chaining: $biblio->items->as_list and doesn't break. It implies that the change won't break any Koha place in which ->as_list is being used on a resultset created by _new_from_dbic 6. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 123994 [details] [review] Bug 28883: (QA follow-up) Improve test description Copy-pasted descriptions. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 123995 [details] [review] Bug 28883: [OPTIONAL] (follow-up) Should we force scalar context within map? Currently, this does not seem necessary. Somehow it feels better imo. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Hmm wait (In reply to Marcel de Rooy from comment #7) > Seems to work as it is, but I am just wondering if we should force scalar > context within the map statement of _wrap ? This is nonsense. We are going to the singular class (In reply to Marcel de Rooy from comment #13) > (In reply to Marcel de Rooy from comment #7) > > Seems to work as it is, but I am just wondering if we should force scalar > > context within the map statement of _wrap ? > > This is nonsense. We are going to the singular class Been there, heh Wow, no! This is not as trivial as it looks like. We must force scalar context actually. I am expecting lot of side-effects with this change. Try with: use Koha::Items; my $item = Koha::Items->find(1); my $h1 = { holds => $item->holds, something => 'else' }; use Data::Printer colored => 1; warn p $h1 Add 1 hold => holds is a Koha::Hold when we expect a set Add another hold Outch, you get: { holds => Koha::Hold, Koha::Hold=Hash => "something", else => undef } (In reply to Jonathan Druart from comment #15) > Wow, no! This is not as trivial as it looks like. > > We must force scalar context actually. I am expecting lot of side-effects > with this change. I get that, but isn't it the case already? We already force scalar context when passing things to the templates. I don't know, did you check the hundreds occurrences we have in the code? Created attachment 124181 [details] [review] Bug 28883: Fix wrong list context calls I get it, but consistency will always be a good thing! Right now we need: $template->param( outstanding_debits => scalar $patron->account->outstanding_debits, items => $biblio->items, ); Because we are not consistent (i.e. outstanding_debits does a return $smth->search) I'd prefer $template->param( outstanding_debits => scalar $patron->account->outstanding_debits, items => scalar $biblio->items, ); The motivation for this bug was to find consensus on a predictable way of coding to lower amount of bugs. I have found only one problematic occurrence but I'd like another QA stamp with that in mind. (In reply to Tomás Cohen Arazi from comment #19) > $template->param( > outstanding_debits => scalar $patron->account->outstanding_debits, > items => scalar $biblio->items, > ); I'd personally prefer to not have to forget the "scalar" (and almost always use iterator) and explicitly ask for the list when I need it (->as_list). Not blocking, just expressing my opinion :) If we need consistency, then should we adjust all the other methods? Koha::UploadedFiles->getCategories? (In reply to Jonathan Druart from comment #21) > (In reply to Tomás Cohen Arazi from comment #19) > > $template->param( > > outstanding_debits => scalar $patron->account->outstanding_debits, > > items => scalar $biblio->items, > > ); > > I'd personally prefer to not have to forget the "scalar" (and almost always > use iterator) and explicitly ask for the list when I need it (->as_list). > > Not blocking, just expressing my opinion :) I'm of two minds.. in some ways perls contextual returns are lovely and DWIM.. but in other cases I'd really rather it required me to be specific. DBIC actually gets around this by having a context ignorant accessor.. adding _rs to accessor methods (search, relationships) will force list context and give you back an iterator rather than getting clever with wantarray. (In reply to Martin Renvoize from comment #23) > (In reply to Jonathan Druart from comment #21) > > (In reply to Tomás Cohen Arazi from comment #19) > > > $template->param( > > > outstanding_debits => scalar $patron->account->outstanding_debits, > > > items => scalar $biblio->items, > > > ); > > > > I'd personally prefer to not have to forget the "scalar" (and almost always > > use iterator) and explicitly ask for the list when I need it (->as_list). > > > > Not blocking, just expressing my opinion :) > > I'm of two minds.. in some ways perls contextual returns are lovely and > DWIM.. but in other cases I'd really rather it required me to be specific. > > DBIC actually gets around this by having a context ignorant accessor.. > adding _rs to accessor methods (search, relationships) will force list > context and give you back an iterator rather than getting clever with > wantarray. I understand and agree Koha::Objects being too clever might be counter productive. What I found was an inconsistent behavior. Getting rid of the use of wantarray should be discussed broadly, as a big behavior change (think plugins, etc). This messes with calls to Koha::Template::Plugin::Scalar.. I feel like ::Scalar was introduced as a work around for the problems this attempts to resolve at a lower level perhaps. Anyway.. I'm seeing the following error in the logs when trying to look at the record details page for various bibs with this patch applied: [Thu Sep 02 08:55:44.032397 2021] [cgi:error] [pid 14881] [client 84.71.180.109:37036] AH01215: Template process failed: undef error - Can't call method "filter_by_for_hold" on unblessed reference at /home/martin/kohaclone/Koha/Template/Plugin/Context.pm line 50.: /home/martin/kohaclone/catalogue/detail.pl, referer: http://mrdev-zebra-staff.koha-ptfs.co.uk/cgi-bin/koha/catalogue/search.pl?idx=kw&q=spin Created attachment 124420 [details] [review] Bug 28883: (QA follow-up) Update use of Scalar This patch updates the use of the TT Scalar plugin and clarifies resultset objects vs arrays in the variable naming. That patch seems to resolve it.. the general approach for this in the dbic world is to use _rs to force scalar context at the object level.. but out Scalar plugin does pretty much the same thing. Hmm, the more I delve into this the more scary I find it actually.. :( I'm coming to a similar conclusion to Jonathan, that we should always return the object iterator and rely on as_list to get an array or arrayref when required. As I try to QA 24857 I keep running into new issues in TT where we'd need to start calling the Scalar plugin to get back the object... I'm kinda torn here.. DBIC does the magic wantarray handling and then adds an _rs version of methods to force return of a scalar ignoring context.. we're doing the opposite here by having 'as_list' which forces return of an array (or arrayref) but again doing magic in the core return. Perhaps we should keep this but replace our as_list function with _rs or as_object or something along those lines? We loose some magic because we always have to re-add the relationships back in manually when we write out objects.. so adding an _rs equivalent for every relationship accessor could be a bit of a pain :(. It's not something we can/should do in one patch (either way we pick). But we can start moving some, bit by bit, and add a deprecation notice (like: we won't support wantarray in 2 major versions anymore). I think we should discuss what we want. And draw a plan. After playing with this in a few ways I think I like the plan of doing as dbic does with the standard returns.. i.e. using wantarray and taking note of context. I don't hugely like the use of Context.Scalar in templates.. it feels cumbersome somehow, but Tomas showed me a core tt plugin that allows thingy.scalar.relationship.. i.e adding scalar into the chain. That seems to work OK in my testing. @ashimema, @Joubu what you think needs to be done to unlock this? I could chase the places in which scalar is used in a chained fashion on templates and make them use the Scalar TT plugin as suggested. After reading the comments again, it seems that: - Keep wantarray, fix places: ashimema, tcohen - Get rid of wantarray: Joubu what do you think, Marcel? (In reply to Tomás Cohen Arazi from comment #33) > After reading the comments again, it seems that: > - Keep wantarray, fix places: ashimema, tcohen > - Get rid of wantarray: Joubu > > what do you think, Marcel? I am close to revisiting this bug. On my list ;) Looking here now. + is( $items_scalar->count, 2, 'No items in resultset' ); + is( scalar @items_list, 2, 'Empty list in list context' ); Curious code segment Koha/Patron: sub get_club_enrollments { my ( $self, $return_scalar ) = @_; my $e = Koha::Club::Enrollments->search( { borrowernumber => $self->borrowernumber(), date_canceled => undef } ); return $e if $return_scalar; return wantarray ? $e->as_list : $e; } $return_scalar ? (In reply to Jonathan Druart from comment #22) > If we need consistency, then should we adjust all the other methods? > > Koha::UploadedFiles->getCategories? Whats wrong with that one? Hahaha, it is the most consistent routine in Koha. Always returning an arrayref.. Who wrote it? ;) (In reply to Tomás Cohen Arazi from comment #16) > (In reply to Jonathan Druart from comment #15) > > Wow, no! This is not as trivial as it looks like. > > > > We must force scalar context actually. I am expecting lot of side-effects > > with this change. > > I get that, but isn't it the case already? We already force scalar context > when passing things to the templates. It is not the case already. There is no need to force scalar now on $item->holds or similar constructions since you get a single "plural Koha object" back (if you know what I mean). [PATCH 4/5] Bug 28883: Fix wrong list context calls - my $images = $biblio->cover_images; - $template->param( localimages => $biblio->cover_images ); + $template->param( localimages => scalar $biblio->cover_images ); Why didnt you pass $images instead ? The original code was a bit weird. But yes, it serves as a good example of why we cannot do this without immediately fixing a lot of calls. (In reply to Jonathan Druart from comment #20) > I have found only one problematic occurrence but I'd like another QA stamp > with that in mind. I have my doubts here if that was really the only one. $biblio->items->filter_by_visible_in_opac( { patron => $patron } )->as_list => This filter sub in Koha/Items always returns a scalar (a plural object). => Unfortunately, we have been quite inconsistent in this area. E.g. directly following it: sub filter_out_lost { my ($self) = @_; my $params = { itemlost => 0 }; return $self->search( $params ); } ->search is context specific (contains wantarray), so depends on caller and thus filter_out_lost is too. I am lost ;) (In reply to Marcel de Rooy from comment #41) > (In reply to Jonathan Druart from comment #20) > > I have found only one problematic occurrence but I'd like another QA stamp > > with that in mind. > > I have my doubts here if that was really the only one. Searching a bit, hmm. The first weak(!) example I found in a test: t/db_dependent/Illrequests.t: isa_ok($patron->checkouts, 'Koha::Checkouts'); In this specific test at that time there is only 1 item. But if there were more, we would have a problem. Gaining bit more confidence.. OK Cant find bad calls that easy ;) WHich doesnt say there are none! What I did: === cd Koha; git grep -E "return.*s\->_new_from_dbic" => Find routines that directly return a new_from_dbic with a plural object DONE Account/Line.pm DONE Acquisition/Basket.pm DONE Acquisition/Bookseller.pm DONE Acquisition/Order.pm DONE Biblio.pm Cash/Register.pm Checkouts/ReturnClaims.pm DONE Clubs.pm DONE Course/Item.pm Illrequest.pm DONE Item.pm DONE Library.pm Object.pm Object/Mixin/AdditionalFields.pm Objects.pm DONE Patron.pm Patrons.pm StockRotationRota.pm Within that module, I looked for the specific subs that do it: Biblio: items, orders, suggestions, cover_images Patron: holds,checkouts, pending_checkouts, old_checkouts, get_routing_lists, old_holds, return_claims, extended_attributes Item: holds, cover_images,orders, get_transfers, current_holds, tracked_links Account/Line: credit_offsets, debit_offsets, credits, debits Acquisition/Bookseller: baskets, contacts Acquisition/Order: claims Course/Item.pm: course_reserves Library.pm: stockrotationstages, outgoing_transfers, inbound_transfers, library_groups, cash_registers For each of these subs, I grepped something like: git grep -E "\->\s*debits(\s*,|$)" Which should find occurrences where $blabla-> debits, might be in a list or function call etc. So far, so good. git grep Context.Scalar on master koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc: [% IF Context.Scalar(Context.Scalar(items, "filter_by_for_hold"), "count") %] koha-tmpl/intranet-tmpl/prog/en/includes/catalog-strings.inc: [% SET current = Context.Scalar(orders, "filter_by_current") %] koha-tmpl/intranet-tmpl/prog/en/includes/catalog-strings.inc: [% SET cancelled = Context.Scalar(orders, "filter_by_cancelled") %] I think we should not do these things in the templates. Doesnt look good to me. The double Scalar in the first line is horrible. (In reply to Tomás Cohen Arazi from comment #33) > After reading the comments again, it seems that: > - Keep wantarray, fix places: ashimema, tcohen > - Get rid of wantarray: Joubu > > what do you think, Marcel? Well, it looks like Martin changed opinion between comment28 and comment31. And I do not read explicitly that Joubu wants to get rid of wantarray btw. I would like to go the wantarray approach like this dev proposes. More generally, it is the perl way of things. (The number of hits on wantarray is surprisingly low btw in the codebase.) We created ->search as context sensitive, so making new_from_dbic now context sensitive too and therewith ->items, ->holds etc, etc too is a logical followup. As Jonathan and I have been looking for bad calls, it feels that we are on the safe side when going further. Maybe we should push this quickly if we want it in the next release. Allowing to test and find remaining problems. But this is something else: > I could chase the places in which scalar is used in a chained fashion on > templates and make them use the Scalar TT plugin as suggested. Personally, I dont like to go that road. I already had some reservations about passing Koha objects into the templates, but okay we do. But adding these scalar calls here and there in templates is imo kind of pollution. Solve in the controller, in perl. Already we may miss things when git grepping the codebase, since we forget that we are using a different notation when calling objects in the templates. Which might be dangerous in the long run? So I would opt for adding a patch here that removes these three calls somehow instead of adjusting them. (In reply to Marcel de Rooy from comment #45) > git grep Context.Scalar on master > > koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc: [% IF > Context.Scalar(Context.Scalar(items, "filter_by_for_hold"), "count") %] > koha-tmpl/intranet-tmpl/prog/en/includes/catalog-strings.inc: [% SET > current = Context.Scalar(orders, "filter_by_current") %] > koha-tmpl/intranet-tmpl/prog/en/includes/catalog-strings.inc: [% SET > cancelled = Context.Scalar(orders, "filter_by_cancelled") %] > > I think we should not do these things in the templates. Doesnt look good to > me. > The double Scalar in the first line is horrible. Additional note: If TT was doing things a bit smarter, this should work like we chain in perl: var countorders = [% biblio.orders.filter_by_current.count || 0 | html %]; (In Perl the chain brings it in scalar context.) But it fails. TT calls the orders method in LIST context. Explaining the horrible scalar stuff introduced. See also https://www.perlmonks.org/bare/?node_id=413941 Question remains then: Should we workaround that in the script or template? (In reply to Marcel de Rooy from comment #47) > (In reply to Marcel de Rooy from comment #45) > > git grep Context.Scalar on master > > > > koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc: [% IF > > Context.Scalar(Context.Scalar(items, "filter_by_for_hold"), "count") %] > > koha-tmpl/intranet-tmpl/prog/en/includes/catalog-strings.inc: [% SET > > current = Context.Scalar(orders, "filter_by_current") %] > > koha-tmpl/intranet-tmpl/prog/en/includes/catalog-strings.inc: [% SET > > cancelled = Context.Scalar(orders, "filter_by_cancelled") %] > > > > I think we should not do these things in the templates. Doesnt look good to > > me. > > The double Scalar in the first line is horrible. > > Additional note: > If TT was doing things a bit smarter, this should work like we chain in perl: > var countorders = [% biblio.orders.filter_by_current.count || 0 | html %]; > (In Perl the chain brings it in scalar context.) > > But it fails. TT calls the orders method in LIST context. Explaining the > horrible scalar stuff introduced. > See also https://www.perlmonks.org/bare/?node_id=413941 We should be able to use the TT scalar plugin and do things like: [% USE scalar %] [% SET var = scalar.biblio.items %] but I haven't checked it. > Question remains then: Should we workaround that in the script or template? I feel like we should do this stuffs in the controller and leave logic out of TT, as much as possible. (In reply to Marcel de Rooy from comment #47) > But it fails. TT calls the orders method in LIST context. Explaining the > horrible scalar stuff introduced. > See also https://www.perlmonks.org/bare/?node_id=413941 This last observations makes things a bit worse. Because we need to check all method calls in the templates now too. (In reply to Tomás Cohen Arazi from comment #48) > We should be able to use the TT scalar plugin and do things like: > > [% USE scalar %] > [% SET var = scalar.biblio.items %] > > but I haven't checked it. I tried: + [% IF biblio.scalar.items.scalar.filter_by_for_hold.count %] + var countorders = [% biblio.scalar.orders.scalar.filter_by_current.count || 0 | html %]; + var countdeletedorders = [% biblio.scalar.orders.scalar.filter_by_cancelled.count || 0 | html %]; with [% USE scalar %] and [% USE Scalar %], but couldnt get them to work. Template process failed: undef error - The method Koha::Biblio->scalar is not covered by tests! Comment on attachment 124420 [details] [review] Bug 28883: (QA follow-up) Update use of Scalar I think we better remove this patch here. It contains a regression too. Hmm, for the .scalar stuff to work do we not need to also update C4::Templates to use the "experimental" Template::Stash::Context module for it's stash as aposed to the default Template::Stash? Must admin, I'm a little confused by our use of TT now I look into it.. we have some fun code in C4::Templates to 'ade transition'. Humm.. now I'm more confused.. the TT docs mention the Scalar plugin but that perlmonks question mentioned ::Stash::Context.. I'm not sure which route we're talking about now. Just saying, I though we were all aware of these problems. I have been all down this road long time ago and it's why I introduced the use of scalar and the weird lines in the include file (better there than duplicated in the controllers IMO). It was not the sexier approach but it worked, and I didn't find another solution at that time. Then it's also why I suggested earlier in this thread to be explicit when we needed an array. Just failed another bug because this does not work (TT list context): [% IF patron.article_requests_current.count %] And about that, I have been saying longer that I just dont like all these object methods in templates :) I think this is a serious drawback when using TT. Let's close this one, and focus on removing the use of wantarray in Koha::Objects instead. |