https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28871 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 123976 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123976&action=edit Bug 28883: Regression tests -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com, | |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123977|0 |1 is obsolete| | --- Comment #4 from David Nind <david@davidnind.com> --- Created attachment 123981 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123981&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- is( $items_scalar->count, 2, 'No items in resultset' ); Hmm that description ;) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|BLOCKED |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123981|0 |1 is obsolete| | --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 123993 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123993&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 123995 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123995&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #15 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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 } -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #17 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I don't know, did you check the hundreds occurrences we have in the code? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #19 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #21 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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 :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #23 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #27 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #29 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|24857 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24857 [Bug 24857] Add ability to group items for records -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #32 from Tomás Cohen Arazi <tomascohen@gmail.com> --- @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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #34 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 ;) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #36 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- + is( $items_scalar->count, 2, 'No items in resultset' ); + is( scalar @items_list, 2, 'Empty list in list context' ); -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #38 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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? ;) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #40 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- [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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #42 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- $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 ;) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #44 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #46 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #48 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #50 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #52 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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'. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 --- Comment #54 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29139 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28883 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.