Bug 19809 - Koha::Objects::find calls do not need to be forbidden in list context
Summary: Koha::Objects::find calls do not need to be forbidden in list context
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 18539
Blocks:
  Show dependency treegraph
 
Reported: 2017-12-14 08:18 UTC by Julian Maurice
Modified: 2020-03-09 17:29 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.03


Attachments
Bug 19809: Re-allow to call Koha::Objects::find in list context (9.49 KB, patch)
2017-12-14 08:22 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19809: Re-allow to call Koha::Objects::find in list context (9.47 KB, patch)
2018-03-15 16:18 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 19809: Re-allow to call Koha::Objects::find in list context (11.99 KB, patch)
2019-07-04 11:12 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19809: Re-allow to call Koha::Objects::find in list context (10.71 KB, patch)
2019-09-16 06:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19809: Re-allow to call Koha::Objects::find in list context (11.12 KB, patch)
2019-11-06 16:31 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19809: Re-allow to call Koha::Objects::find in list context (11.27 KB, patch)
2020-01-22 10:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19809: Remove some new occurrences (1.99 KB, patch)
2020-01-22 10:51 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2017-12-14 08:18:38 UTC
Bug 18539 forbade to call Koha::Objects::find in list context.
This is not necessary, we can fix the subroutine so that it can be called in scalar and list contexts without problems.

See also discussion on ML: http://lists.koha-community.org/pipermail/koha-devel/2017-December/044213.html
Comment 1 Julian Maurice 2017-12-14 08:22:42 UTC
Created attachment 69790 [details] [review]
Bug 19809: Re-allow to call Koha::Objects::find in list context

and remove 'scalar' keyword in calls where it's not needed.
Also, fix Koha::Patron::guarantor which had the same problem as find
Comment 2 Brendan Gallagher 2018-03-15 16:18:12 UTC
Created attachment 72977 [details] [review]
Bug 19809: Re-allow to call Koha::Objects::find in list context

and remove 'scalar' keyword in calls where it's not needed.
Also, fix Koha::Patron::guarantor which had the same problem as find

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 3 Marcel de Rooy 2018-04-27 08:01:42 UTC
QA: Looking here
Comment 4 Marcel de Rooy 2018-04-27 08:26:49 UTC
My first feeling is that if we allow list context, we should return an empty list instead of undef. It would fix your if( @a ) test since  @a==0. But what would pop up again? Constructions like the one in Objects.t:
my $patrons = {
        foo => Koha::Patrons->find('foo'),
        bar => 'baz',
};
This would generate "Odd number of elements in anonymous hash" warnings and mixup of hash keys and values. So we should have to add scalars again in the parameter hashes.
So the question actually becomes: Where do we want to add scalars?
Imo the current situation helps us to identify problems immediately. If you want to add a "push @a, Koha::Objects->find($b)" you will crash rightaway and see that you must add a scalar. The other way around, the empty list in the anonymous hash will not popup right away, so the problem may go unnoticed. For me, the balance goes to keeping the forbid-list-context.
Moving to FQA but could have been ID too.
Comment 5 Julian Maurice 2018-06-08 07:55:47 UTC
(In reply to Marcel de Rooy from comment #4)
> My first feeling is that if we allow list context, we should return an empty
> list instead of undef. It would fix your if( @a ) test since  @a==0.
This doesn't need to be fixed. Empty list and undef both evaluate to a false value in this context.

> But what would pop up again? Constructions like the one in Objects.t:
> my $patrons = {
>         foo => Koha::Patrons->find('foo'),
>         bar => 'baz',
> };
> This would generate "Odd number of elements in anonymous hash" warnings and
> mixup of hash keys and values. So we should have to add scalars again in the
> parameter hashes.
I believe that is the main reason why calls in list context were forbidden in the first place. But this problem disappear if 'find' returns undef instead of an empty list. 

> So the question actually becomes: Where do we want to add scalars?
Nowhere! Because we don't need to :-)

IMO 'find' is a method that should return one, only one, and always one result. And this should not depend on the calling context.
Comment 6 Julian Maurice 2018-06-08 07:58:05 UTC
... and also DBIx::Class::ResultSet::find returns undef in list context. I believe Koha::Objects::find should have the same behaviour.

https://metacpan.org/pod/DBIx::Class::ResultSet#find
Comment 7 Julian Maurice 2019-07-04 11:12:54 UTC
Created attachment 91287 [details] [review]
Bug 19809: Re-allow to call Koha::Objects::find in list context

and remove 'scalar' keyword in calls where it's not needed.
Also, fix Koha::Patron::guarantor which had the same problem as find

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 8 Bouzid Fergani 2019-09-13 19:05:46 UTC
Hi Julian,
Thanks to rebase this patch.

Bug 19809 - Koha::Objects::find calls do not need to be forbidden in list context

91287 - Bug 19809: Re-allow to call Koha::Objects::find in list context

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 19809: Re-allow to call Koha::Objects::find in list context
Using index info to reconstruct a base tree...
M	C4/Reserves.pm
M	Koha/Patron.pm
M	cataloguing/moveitem.pl
M	circ/overdue.pl
M	members/memberentry.pl
M	members/pay.pl
M	opac/opac-memberentry.pl
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-memberentry.pl
CONFLICT (content): Merge conflict in opac/opac-memberentry.pl
Auto-merging members/pay.pl
Auto-merging members/memberentry.pl
Auto-merging circ/overdue.pl
Auto-merging cataloguing/moveitem.pl
Auto-merging Koha/Patron.pm
CONFLICT (content): Merge conflict in Koha/Patron.pm
Auto-merging C4/Reserves.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 19809: Re-allow to call Koha::Objects::find in list context
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-19809-Re-allow-to-call-KohaObjectsfind-in-list-_0gg5g.patch
Comment 9 Julian Maurice 2019-09-16 06:08:38 UTC
Created attachment 92825 [details] [review]
Bug 19809: Re-allow to call Koha::Objects::find in list context

and remove 'scalar' keyword in calls where it's not needed.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 10 Martin Renvoize 2019-10-29 11:55:05 UTC
This seems sane to me and I like to follow the example set by DBIx::Class to be honest.. that project has been around for a long time and has made allot of good decisions for good reasons.
Comment 11 Marcel de Rooy 2019-11-01 11:06:16 UTC
(In reply to Martin Renvoize from comment #10)
> This seems sane to me and I like to follow the example set by DBIx::Class to
> be honest.. that project has been around for a long time and has made allot
> of good decisions for good reasons.

I would agree with you if we were writing Koha::Objects->find at this moment. But in the meantime they go back to bug 13019, pushed 02/2015. How many find calls do we have now, and associated if conditions etc.?
In view of that volume and the risk of breaking a lot of code, I would not recommend it now.
Comment 12 Julian Maurice 2019-11-04 08:18:49 UTC
(In reply to Marcel de Rooy from comment #11)
> (In reply to Martin Renvoize from comment #10)
> > This seems sane to me and I like to follow the example set by DBIx::Class to
> > be honest.. that project has been around for a long time and has made allot
> > of good decisions for good reasons.
> 
> I would agree with you if we were writing Koha::Objects->find at this
> moment. But in the meantime they go back to bug 13019, pushed 02/2015. How
> many find calls do we have now, and associated if conditions etc.?
> In view of that volume and the risk of breaking a lot of code, I would not
> recommend it now.

Currently, calls to `find` are always in scalar context (because otherwise Koha croaks), and this patch doesn't change the behaviour of `find` in scalar context. Why do you think there's a risk of breaking code ?
Comment 13 Marcel de Rooy 2019-11-05 15:11:32 UTC
(In reply to Julian Maurice from comment #12)
> (In reply to Marcel de Rooy from comment #11)
> > (In reply to Martin Renvoize from comment #10)
> > > This seems sane to me and I like to follow the example set by DBIx::Class to
> > > be honest.. that project has been around for a long time and has made allot
> > > of good decisions for good reasons.
> > 
> > I would agree with you if we were writing Koha::Objects->find at this
> > moment. But in the meantime they go back to bug 13019, pushed 02/2015. How
> > many find calls do we have now, and associated if conditions etc.?
> > In view of that volume and the risk of breaking a lot of code, I would not
> > recommend it now.
> 
> Currently, calls to `find` are always in scalar context (because otherwise
> Koha croaks), and this patch doesn't change the behaviour of `find` in
> scalar context. Why do you think there's a risk of breaking code ?

Hi Julian,
This might have been a bit overcautious, you are right.
But looking at the code, I still see some problems:

+    @pars = grep { defined } @pars;
This is not the same as the earlier check:
-    return if !@pars || none { defined($_) } @pars;
Btw note that this statement would return an empty list (with the croak removed).

     return $object;
This is no longer good. Since you are returning 'undef' here literally while you want to return empty list if it is list context.
You should do:
    if (@pars) {
        my $result = $self->_resultset()->find(@pars);
        if ($result) {
            $object = $self->object_class()->_new_from_dbic($result);
            return $object;
        }
    }
    return;
The last return gives empty list in list context and undef in scalar context. 

I would like to see that tested too in the Objects.t test.
And we need a rebase on members/pay.pl
Comment 14 Julian Maurice 2019-11-05 15:42:11 UTC
(In reply to Marcel de Rooy from comment #13)
> Hi Julian,
> This might have been a bit overcautious, you are right.
> But looking at the code, I still see some problems:
> 
> +    @pars = grep { defined } @pars;
> This is not the same as the earlier check:
> -    return if !@pars || none { defined($_) } @pars;
> Btw note that this statement would return an empty list (with the croak
> removed).

Yes, but the croak is there, so if we reach this line it means we're in scalar context, so it would return undef.
Also, I am failing to see what the differences are between the two checks. In both cases, if @pars is empty or contain only undefined values, the rest of the subroutine is not executed. Can you give a value of @pars for which the behavior differ ?

> 
>      return $object;
> This is no longer good. Since you are returning 'undef' here literally while
> you want to return empty list if it is list context.

I do not want to return an empty list. I want Koha::Objects::find to behave like DBIx::Class::ResultSet::find, which returns undef if no results were found (even if in list context)
Comment 15 Marcel de Rooy 2019-11-05 15:57:49 UTC
(In reply to Julian Maurice from comment #14)
> (In reply to Marcel de Rooy from comment #13)
> > Hi Julian,
> > This might have been a bit overcautious, you are right.
> > But looking at the code, I still see some problems:
> > 
> > +    @pars = grep { defined } @pars;
> > This is not the same as the earlier check:
> > -    return if !@pars || none { defined($_) } @pars;
> > Btw note that this statement would return an empty list (with the croak
> > removed).
> 

> Also, I am failing to see what the differences are between the two checks.
> In both cases, if @pars is empty or contain only undefined values, the rest
> of the subroutine is not executed. Can you give a value of @pars for which
> the behavior differ ?

Yes, this is very theoretical.
You strip the @pars array now. Before we passed the original array unless it only consisted of undefined values.
So suppose I pass 51, undef, 13. You are now passing 51, 13. Different thing!
Note that find responds to multiple column PKs and unique constraint stuff. (We are probably not using in Koha.)
Comment 16 Marcel de Rooy 2019-11-05 16:00:57 UTC
(In reply to Julian Maurice from comment #14)
> >      return $object;
> > This is no longer good. Since you are returning 'undef' here literally while
> > you want to return empty list if it is list context.
> 
> I do not want to return an empty list. I want Koha::Objects::find to behave
> like DBIx::Class::ResultSet::find, which returns undef if no results were
> found (even if in list context)

OK. I didnt get that right away. It is quite unperlish to return ( undef ).
But yes, I checked and DBIx does it here..
So, should we stick to perl or to DBIx ? Since Koha::Objects relies on DBIx, I wouldnt mind doing the latter here.
Comment 17 Julian Maurice 2019-11-05 17:41:24 UTC
(In reply to Marcel de Rooy from comment #15)
> Yes, this is very theoretical.
> You strip the @pars array now. Before we passed the original array unless it
> only consisted of undefined values.
> So suppose I pass 51, undef, 13. You are now passing 51, 13. Different thing!
> Note that find responds to multiple column PKs and unique constraint stuff.
> (We are probably not using in Koha.)

Ah yes, you are right. Even if we are not using it in Koha it may be used by external tools or plugins, so it needs to be fixed

(In reply to Marcel de Rooy from comment #16)
> OK. I didnt get that right away. It is quite unperlish to return ( undef ).
> But yes, I checked and DBIx does it here..
> So, should we stick to perl or to DBIx ? Since Koha::Objects relies on DBIx,
> I wouldnt mind doing the latter here.

I don't know if it's perlish or not. To me it just feels natural to always get a value (even if that value is undef) when I call find. And when you think about it, that's what we get actually by being forced to put scalar in front of `find` calls.

Also, it's not that uncommon in other Perl modules. Cache::Memcached does the same thing for instance

% perl -MCache::Memcached -MData::Dumper -E '$memd = new Cache::Memcached; say Dumper [$memd->get("mykey")];'

$VAR1 = [
          undef
        ];

It would be great to have Jonathan's opinion as he wrote the patch for bug 18539
Comment 18 Marcel de Rooy 2019-11-06 12:22:19 UTC
(In reply to Julian Maurice from comment #17)
> I don't know if it's perlish or not. To me it just feels natural to always
> get a value (even if that value is undef) when I call find. And when you
> think about it, that's what we get actually by being forced to put scalar in
> front of `find` calls.

https://metacpan.org/pod/Perl::Critic::Policy::Subroutines::ProhibitExplicitReturnUndef
Inspired by Damian's best practices.

Note that my $object; ... return $object; is almost the same.

But this is not an argument to not allow an exception from the rule ;)
The only thing we should not do is:
@array = Koha::Objects->find($x);
if( @array ) {
etc.
Because that condition would always be true. Similarly, foreach( ..->find ) would not work.

if( Koha::Objects->find($x) ) would be just fine however and probably what we need most.
Comment 19 Marcel de Rooy 2019-11-06 12:30:18 UTC
(In reply to Julian Maurice from comment #17)
> It would be great to have Jonathan's opinion as he wrote the patch for bug
> 18539

Looking back, I think that the biggest problem was using ->find in parameter  hashes like { var1 => KO->find, var2 => etc }. Here we have list context and if there was no result, we were returning an empty list causing a parameter/value shift.
This comes from:
    return unless $result;

Since your patch does not return empty list, we would not suffer from that.
Comment 20 Jonathan Druart 2019-11-06 13:24:20 UTC
From 18539:
"""
Reading https://perlmaven.com/how-to-return-undef-from-a-function
this sound like the more correct behaviour.
"""

That is why I picked this solution.
I am not against another one, but what would be the gain?

Behave as DBIx::Class::ResultSet::find? Ok I am good with that.
Comment 21 Julian Maurice 2019-11-06 15:39:53 UTC
(In reply to Jonathan Druart from comment #20)
> From 18539:
> """
> Reading https://perlmaven.com/how-to-return-undef-from-a-function
> this sound like the more correct behaviour.
> """

All the pages I read about this topic talk about returning undef to indicate a failure (it is compared to `die` and `croak`)
But this is not a failure here. Not finding a database entry is not a failure. "No results" is a valid result.

https://perlmaven.com/how-to-return-undef-from-a-function uses this example

my @y_results = div(42, 0);
if (@y_results) {
    say "Success! We can divide 42 by 0";
} else {
    say "Failure!";
}

In my opinion this is a bad example. Nobody should write code like that. The `div` subroutine must be clear about its return value. It is then the caller responsibility to use it correctly.
And there is nothing wrong in calling `div` in a list context. For example:

my @results = map { div(42, $_) } (0..9);

Forbidding a perfectly valid use case makes no sense to me.

> I am not against another one, but what would be the gain?

It just feels like the sane thing to do. I expect Koha::Objects->find to return a scalar value, not a list. And adding `scalar` in front of `->find` calls seems superfluous.
If I'm the only one feeling that way, please mark it as "wontfix", I will gladly give up on this patch
Comment 22 Marcel de Rooy 2019-11-06 15:55:41 UTC
(In reply to Julian Maurice from comment #21)
> (In reply to Jonathan Druart from comment #20)
> It just feels like the sane thing to do. I expect Koha::Objects->find to
> return a scalar value, not a list. And adding `scalar` in front of `->find`
> calls seems superfluous.
> If I'm the only one feeling that way, please mark it as "wontfix", I will
> gladly give up on this patch

I am having the impression that he did not want you to give up at all. So please continue (ungladly) and make it work again!
Comment 23 Julian Maurice 2019-11-06 16:31:57 UTC
Created attachment 95115 [details] [review]
Bug 19809: Re-allow to call Koha::Objects::find in list context

and remove 'scalar' keyword in calls where it's not needed.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 24 Julian Maurice 2019-11-06 16:34:32 UTC
Patch rebased + some changes:

- Explicitly say in POD that `find` returns undef when there is no results.
- Fix the `if` condition so that the behaviour is unchanged (see comment 15)
Comment 25 Julian Maurice 2019-11-06 16:43:21 UTC
> I am having the impression that he did not want you to give up at all. So
> please continue (ungladly) and make it work again!

I know, but at this point, it just feel like I'm pushing something that is a matter of personal preferences rather than an objective improvement. And it's not important enough to waste time on it.
Comment 26 Julian Maurice 2020-01-17 15:39:19 UTC
Forgot to change the status. Back to "signed off"
Comment 27 Martin Renvoize 2020-01-21 16:40:49 UTC
Personally, I'm pretty onboard with this.. moving us closer to DBIx::Class seems reasonable and productive.
Comment 28 Jonathan Druart 2020-01-22 10:51:31 UTC
Created attachment 97700 [details] [review]
Bug 19809: Re-allow to call Koha::Objects::find in list context

and remove 'scalar' keyword in calls where it's not needed.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2020-01-22 10:51:35 UTC
Created attachment 97701 [details] [review]
Bug 19809: Remove some new occurrences

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Martin Renvoize 2020-01-23 10:35:40 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 31 Joy Nelson 2020-02-06 23:21:47 UTC
Enhancement not backported to 19.11.x
Comment 32 Jonathan Druart 2020-02-11 10:46:49 UTC
(In reply to Joy Nelson from comment #31)
> Enhancement not backported to 19.11.x

I think this change needs to be backported. Or you will have to be meticulous to not backport a ->find call in a list context.
Comment 33 Joy Nelson 2020-02-11 20:12:36 UTC
Rebased and pushed to 19.11.x for 19.11.03

(thanks Nick!)
Comment 34 Lucas Gass 2020-03-09 17:29:30 UTC
Does not cleanly apply to 19.05, will not backport