https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #69790|0 |1 is obsolete| | --- Comment #2 from Brendan Gallagher <brendan@bywatersolutions.com> --- Created attachment 72977 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=72977&action=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 <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 --- Comment #5 from Julian Maurice <julian.maurice@biblibre.com> --- (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.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #72977|0 |1 is obsolete| | --- Comment #7 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 91287 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91287&action=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 <brendan@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Bouzid Fergani <bouzid.fergani@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply CC| |bouzid.fergani@inlibro.com --- Comment #8 from Bouzid Fergani <bouzid.fergani@inlibro.com> --- 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.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 --- Comment #17 from Julian Maurice <julian.maurice@biblibre.com> --- (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.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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::ProhibitExplicit... 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.

From 18539: """ Reading https://perlmaven.com/how-to-return-undef-from-a-function

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 --- Comment #24 from Julian Maurice <julian.maurice@biblibre.com> --- 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.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #26 from Julian Maurice <julian.maurice@biblibre.com> --- Forgot to change the status. Back to "signed off" -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 --- Comment #27 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #95115|0 |1 is obsolete| | --- Comment #28 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 97700 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=97700&action=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 <brendan@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|m.de.rooy@rijksmuseum.nl |jonathan.druart@bugs.koha-c | |ommunity.org -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Martin Renvoize <martin.renvoize@ptfs-europe.com> 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.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable CC| |joy@bywatersolutions.com --- Comment #31 from Joy Nelson <joy@bywatersolutions.com> --- Enhancement not backported to 19.11.x -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|20.05.00 |20.05.00, 19.11.03 released in| | --- Comment #33 from Joy Nelson <joy@bywatersolutions.com> --- 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.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Koha::Objects::find calls |Koha::Objects::find no |do not need to be forbidden |longer need to be forbidden |in list context |in list context Severity|enhancement |minor -- You are receiving this mail because: You are watching all bug changes.