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
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
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>
QA: Looking here
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.
(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.
... 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
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>
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
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>
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.
(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.
(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 ?
(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
(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)
(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.)
(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.
(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
(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.
(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.
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.
(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
(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!
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>
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)
> 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.
Forgot to change the status. Back to "signed off"
Personally, I'm pretty onboard with this.. moving us closer to DBIx::Class seems reasonable and productive.
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>
Created attachment 97701 [details] [review] Bug 19809: Remove some new occurrences Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Nice work everyone! Pushed to master for 20.05
Enhancement not backported to 19.11.x
(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.
Rebased and pushed to 19.11.x for 19.11.03 (thanks Nick!)
Does not cleanly apply to 19.05, will not backport