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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Lucas Gass  changed:

   What|Removed |Added

 CC||lu...@bywatersolutions.com

--- Comment #34 from Lucas Gass  ---
Does not cleanly apply to 19.05, will not backport

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Joy Nelson  changed:

   What|Removed |Added

 Version(s)|20.05.00|20.05.00, 19.11.03
released in||

--- Comment #33 from Joy Nelson  ---
Rebased and pushed to 19.11.x for 19.11.03

(thanks Nick!)

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #32 from Jonathan Druart  
---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Joy Nelson  changed:

   What|Removed |Added

 Status|Pushed to master|Pushed to stable
 CC||j...@bywatersolutions.com

--- Comment #31 from Joy Nelson  ---
Enhancement not backported to 19.11.x

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #30 from Martin Renvoize  ---
Nice work everyone!

Pushed to master for 20.05

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Martin Renvoize  changed:

   What|Removed |Added

 Version(s)||20.05.00
released in||
 Status|Passed QA   |Pushed to master

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Martin Renvoize  changed:

   What|Removed |Added

   Keywords|rel_20_05_target|

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Jonathan Druart  changed:

   What|Removed |Added

 QA Contact|m.de.r...@rijksmuseum.nl|jonathan.dru...@bugs.koha-c
   ||ommunity.org

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #29 from Jonathan Druart  
---
Created attachment 97701
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=97701=edit
Bug 19809: Remove some new occurrences

Signed-off-by: Jonathan Druart 

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Jonathan Druart  changed:

   What|Removed |Added

 Status|Signed Off  |Passed QA

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Jonathan Druart  changed:

   What|Removed |Added

  Attachment #95115|0   |1
is obsolete||

--- Comment #28 from Jonathan Druart  
---
Created attachment 97700
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=97700=edit
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 

Signed-off-by: Jonathan Druart 

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #27 from Martin Renvoize  ---
Personally, I'm pretty onboard with this.. moving us closer to DBIx::Class
seems reasonable and productive.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Martin Renvoize  changed:

   What|Removed |Added

   Keywords||rel_20_05_target

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Julian Maurice  changed:

   What|Removed |Added

 Status|Failed QA   |Signed Off

--- Comment #26 from Julian Maurice  ---
Forgot to change the status. Back to "signed off"

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #25 from Julian Maurice  ---
> 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.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #24 from Julian Maurice  ---
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)

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Julian Maurice  changed:

   What|Removed |Added

  Attachment #92825|0   |1
is obsolete||

--- Comment #23 from Julian Maurice  ---
Created attachment 95115
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=95115=edit
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 

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #22 from Marcel de Rooy  ---
(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!

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #21 from Julian Maurice  ---
(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

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #20 from Jonathan Druart  
---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #19 from Marcel de Rooy  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #18 from Marcel de Rooy  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Marcel de Rooy  changed:

   What|Removed |Added

 CC||jonathan.dru...@bugs.koha-c
   ||ommunity.org

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #17 from Julian Maurice  ---
(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

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #16 from Marcel de Rooy  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #15 from Marcel de Rooy  ---
(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.)

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #14 from Julian Maurice  ---
(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)

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Marcel de Rooy  changed:

   What|Removed |Added

 Status|Signed Off  |Failed QA

--- Comment #13 from Marcel de Rooy  ---
(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

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #12 from Julian Maurice  ---
(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 ?

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #11 from Marcel de Rooy  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Martin Renvoize  changed:

   What|Removed |Added

 CC||martin.renvoize@ptfs-europe
   ||.com

--- Comment #10 from Martin Renvoize  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Julian Maurice  changed:

   What|Removed |Added

 Status|Patch doesn't apply |Signed Off

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Julian Maurice  changed:

   What|Removed |Added

  Attachment #91287|0   |1
is obsolete||

--- Comment #9 from Julian Maurice  ---
Created attachment 92825
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=92825=edit
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 

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Bouzid Fergani  changed:

   What|Removed |Added

 Status|Signed Off  |Patch doesn't apply
 CC||bouzid.ferg...@inlibro.com

--- Comment #8 from Bouzid Fergani  ---
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

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Julian Maurice  changed:

   What|Removed |Added

 Status|Patch doesn't apply |Signed Off

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Julian Maurice  changed:

   What|Removed |Added

  Attachment #72977|0   |1
is obsolete||

--- Comment #7 from Julian Maurice  ---
Created attachment 91287
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91287=edit
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 

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Marcel de Rooy  changed:

   What|Removed |Added

 Status|Signed Off  |Patch doesn't apply

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Julian Maurice  changed:

   What|Removed |Added

 Status|Failed QA   |Signed Off

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #6 from Julian Maurice  ---
... 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

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #5 from Julian Maurice  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Marcel de Rooy  changed:

   What|Removed |Added

 Status|BLOCKED |Failed QA

--- Comment #4 from Marcel de Rooy  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Marcel de Rooy  changed:

   What|Removed |Added

   Patch complexity|--- |Small patch
 QA Contact|testo...@bugs.koha-communit |m.de.r...@rijksmuseum.nl
   |y.org   |

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Marcel de Rooy  changed:

   What|Removed |Added

 CC||m.de.r...@rijksmuseum.nl
 Status|Signed Off  |BLOCKED

--- Comment #3 from Marcel de Rooy  ---
QA: Looking here

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Brendan Gallagher  changed:

   What|Removed |Added

 Status|Needs Signoff   |Signed Off

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Brendan Gallagher  changed:

   What|Removed |Added

  Attachment #69790|0   |1
is obsolete||

--- Comment #2 from Brendan Gallagher  ---
Created attachment 72977
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=72977=edit
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 

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

Julian Maurice  changed:

   What|Removed |Added

 Status|ASSIGNED|Needs Signoff

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

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

[bugzilla-daemon]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809

--- Comment #1 from Julian Maurice  ---
Created attachment 69790
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69790=edit
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

-- 
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/