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

[bugzilla-daemon at bugs.koha-community.org]

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

Marcel de Rooy <m.de.rooy at rijksmuseum.nl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Signed Off                  |Failed QA

--- Comment #13 from Marcel de Rooy <m.de.rooy at 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.