Bug 28883 - Koha::Objects->_new_from_dbic doesn't work correctly in list context
Summary: Koha::Objects->_new_from_dbic doesn't work correctly in list context
Status: CLOSED WORKSFORME
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 28886
  Show dependency treegraph
 
Reported: 2021-08-19 13:35 UTC by Tomás Cohen Arazi
Modified: 2022-12-12 21:24 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 28883: Regression tests (1.81 KB, patch)
2021-08-19 14:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28883: Make Koha::Objects->_new_from_dbic honour list context (1.50 KB, patch)
2021-08-19 14:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28883: Regression tests (1.85 KB, patch)
2021-08-19 19:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 28883: Make Koha::Objects->_new_from_dbic honour list context (1.54 KB, patch)
2021-08-19 19:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 28883: Regression tests (1.95 KB, patch)
2021-08-20 08:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28883: Make Koha::Objects->_new_from_dbic honour list context (1.64 KB, patch)
2021-08-20 08:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28883: (QA follow-up) Improve test description (1.27 KB, patch)
2021-08-20 08:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28883: [OPTIONAL] (follow-up) Should we force scalar context within map? (954 bytes, patch)
2021-08-20 08:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28883: Fix wrong list context calls (799 bytes, patch)
2021-08-27 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28883: (QA follow-up) Update use of Scalar (2.51 KB, patch)
2021-09-02 08:57 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2021-08-19 13:35:18 UTC
It will always return a resultset, it should return a list in list context.
Comment 1 Tomás Cohen Arazi 2021-08-19 14:46:32 UTC
Created attachment 123976 [details] [review]
Bug 28883: Regression tests
Comment 2 Tomás Cohen Arazi 2021-08-19 14:46:46 UTC
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
Comment 3 David Nind 2021-08-19 19:18:39 UTC
Created attachment 123980 [details] [review]
Bug 28883: Regression tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2021-08-19 19:18:43 UTC
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>
Comment 5 Marcel de Rooy 2021-08-20 07:10:31 UTC
QAing
Comment 6 Marcel de Rooy 2021-08-20 07:39:41 UTC
    is( $items_scalar->count, 2, 'No items in resultset' );

Hmm that description ;)
Comment 7 Marcel de Rooy 2021-08-20 08:47:39 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2021-08-20 08:48:14 UTC
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>
Comment 9 Marcel de Rooy 2021-08-20 08:48:18 UTC
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>
Comment 10 Marcel de Rooy 2021-08-20 08:48:21 UTC
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>
Comment 11 Marcel de Rooy 2021-08-20 08:48:25 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2021-08-20 08:49:34 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2021-08-20 08:52:07 UTC
(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
Comment 14 Tomás Cohen Arazi 2021-08-26 12:14:19 UTC
(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
Comment 15 Jonathan Druart 2021-08-27 12:55:23 UTC
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
}
Comment 16 Tomás Cohen Arazi 2021-08-27 12:58:13 UTC
(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.
Comment 17 Jonathan Druart 2021-08-27 13:05:02 UTC
I don't know, did you check the hundreds occurrences we have in the code?
Comment 18 Jonathan Druart 2021-08-27 13:13:57 UTC
Created attachment 124181 [details] [review]
Bug 28883: Fix wrong list context calls
Comment 19 Tomás Cohen Arazi 2021-08-27 13:14:15 UTC
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.
Comment 20 Jonathan Druart 2021-08-27 13:22:37 UTC
I have found only one problematic occurrence but I'd like another QA stamp with that in mind.
Comment 21 Jonathan Druart 2021-08-27 13:25:27 UTC
(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 :)
Comment 22 Jonathan Druart 2021-08-27 13:27:09 UTC
If we need consistency, then should we adjust all the other methods?

Koha::UploadedFiles->getCategories?
Comment 23 Martin Renvoize 2021-09-01 11:47:16 UTC
(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.
Comment 24 Tomás Cohen Arazi 2021-09-01 12:14:31 UTC
(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).
Comment 25 Martin Renvoize 2021-09-02 07:57:46 UTC
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
Comment 26 Martin Renvoize 2021-09-02 08:57:20 UTC
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.
Comment 27 Martin Renvoize 2021-09-02 08:58:32 UTC
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.
Comment 28 Martin Renvoize 2021-09-02 09:18:32 UTC
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 :(.
Comment 29 Jonathan Druart 2021-09-02 10:05:54 UTC
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).
Comment 30 Tomás Cohen Arazi 2021-09-02 13:30:05 UTC
I think we should discuss what we want. And draw a plan.
Comment 31 Martin Renvoize 2021-09-03 15:27:26 UTC
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.
Comment 32 Tomás Cohen Arazi 2021-09-15 16:40:42 UTC
@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.
Comment 33 Tomás Cohen Arazi 2021-09-15 16:46:37 UTC
After reading the comments again, it seems that:
- Keep wantarray, fix places: ashimema, tcohen
- Get rid of wantarray: Joubu

what do you think, Marcel?
Comment 34 Marcel de Rooy 2021-09-16 05:27:20 UTC
(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 ;)
Comment 35 Marcel de Rooy 2021-09-24 05:59:16 UTC
Looking here now.
Comment 36 Marcel de Rooy 2021-09-24 06:12:08 UTC
+    is( $items_scalar->count, 2, 'No items in resultset' );
+    is( scalar @items_list, 2, 'Empty list in list context' );
Comment 37 Marcel de Rooy 2021-09-24 06:35:45 UTC
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 ?
Comment 38 Marcel de Rooy 2021-09-24 06:43:31 UTC
(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? ;)
Comment 39 Marcel de Rooy 2021-09-24 06:48:06 UTC
(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).
Comment 40 Marcel de Rooy 2021-09-24 06:51:35 UTC
[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.
Comment 41 Marcel de Rooy 2021-09-24 06:55:16 UTC
(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.
Comment 42 Marcel de Rooy 2021-09-24 07:05:00 UTC
$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 ;)
Comment 43 Marcel de Rooy 2021-09-24 07:44:09 UTC
(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..
Comment 44 Marcel de Rooy 2021-09-24 08:01:45 UTC
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.
Comment 45 Marcel de Rooy 2021-09-24 08:17:32 UTC
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.
Comment 46 Marcel de Rooy 2021-09-24 08:34:17 UTC
(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.
Comment 47 Marcel de Rooy 2021-09-24 10:01:14 UTC
(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?
Comment 48 Tomás Cohen Arazi 2021-09-24 12:13:33 UTC
(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.
Comment 49 Marcel de Rooy 2021-09-27 06:52:59 UTC
(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.
Comment 50 Marcel de Rooy 2021-09-27 06:55:42 UTC
(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 51 Marcel de Rooy 2021-09-27 06:57:01 UTC
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.
Comment 52 Martin Renvoize 2021-09-27 07:39:46 UTC
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'.
Comment 53 Martin Renvoize 2021-09-27 07:52:21 UTC
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.
Comment 54 Jonathan Druart 2021-09-27 08:10:22 UTC
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.
Comment 55 Marcel de Rooy 2021-09-27 08:21:08 UTC
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.
Comment 56 Tomás Cohen Arazi 2022-01-07 15:09:58 UTC
Let's close this one, and focus on removing the use of wantarray in Koha::Objects instead.