From 63c67b811e8e0dc413d62fdceb2dd5dc69a9f828 Mon Sep 17 00:00:00 2001 From: Julian Maurice Date: Thu, 14 Dec 2017 09:19:22 +0100 Subject: [PATCH] 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 --- C4/Reserves.pm | 2 +- C4/SIP/ILS/Transaction/Hold.pm | 2 +- Koha/Club.pm | 4 ++-- Koha/Club/Enrollment.pm | 4 ++-- Koha/Club/Field.pm | 2 +- Koha/Objects.pm | 17 +++++++++-------- Koha/Subscription.pm | 4 ++-- acqui/booksellers.pl | 2 +- acqui/uncertainprice.pl | 2 +- cataloguing/moveitem.pl | 2 +- circ/overdue.pl | 2 +- members/memberentry.pl | 2 +- members/pay.pl | 4 ++-- t/db_dependent/Koha/Objects.t | 13 ++++++++++--- 14 files changed, 35 insertions(+), 27 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index cf6ee50169..813322c68a 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -797,7 +797,7 @@ sub CheckReserves { next if (($branchitemrule->{'holdallowed'} == 1) && ($branch ne $patron->branchcode)); my $hold_fulfillment_policy = $branchitemrule->{hold_fulfillment_policy}; next if ( ($branchitemrule->{hold_fulfillment_policy} ne 'any') && ($res->{branchcode} ne $item->$hold_fulfillment_policy) ); - next unless $item->can_be_transferred( { to => scalar Koha::Libraries->find( $res->{branchcode} ) } ); + next unless $item->can_be_transferred( { to => Koha::Libraries->find( $res->{branchcode} ) } ); $priority = $res->{'priority'}; $highest = $res; last if $local_hold_match; diff --git a/C4/SIP/ILS/Transaction/Hold.pm b/C4/SIP/ILS/Transaction/Hold.pm index 825e89b9db..536d10d910 100644 --- a/C4/SIP/ILS/Transaction/Hold.pm +++ b/C4/SIP/ILS/Transaction/Hold.pm @@ -60,7 +60,7 @@ sub do_hold { $self->ok(0); return $self; } - unless ( $item->can_be_transferred( { to => scalar Koha::Libraries->find( $branch ) } ) ) { + unless ( $item->can_be_transferred( { to => Koha::Libraries->find( $branch ) } ) ) { $self->screen_msg('Item cannot be transferred.'); $self->ok(0); return $self; diff --git a/Koha/Club.pm b/Koha/Club.pm index 09a510b64d..f55bfb54a8 100644 --- a/Koha/Club.pm +++ b/Koha/Club.pm @@ -48,7 +48,7 @@ sub club_template { return unless $self->club_template_id(); - return scalar Koha::Club::Templates->find( $self->club_template_id() ); + return Koha::Club::Templates->find( $self->club_template_id() ); } =head3 club_fields @@ -84,7 +84,7 @@ sub branch { return unless $self->branchcode(); - return scalar Koha::Libraries->find( $self->branchcode() ); + return Koha::Libraries->find( $self->branchcode() ); } =head3 type diff --git a/Koha/Club/Enrollment.pm b/Koha/Club/Enrollment.pm index 5b42a0a686..5b6be0a7a1 100644 --- a/Koha/Club/Enrollment.pm +++ b/Koha/Club/Enrollment.pm @@ -61,7 +61,7 @@ sub cancel { sub club { my ( $self ) = @_; - return scalar Koha::Clubs->find( $self->club_id() ); + return Koha::Clubs->find( $self->club_id() ); } =head3 patron @@ -70,7 +70,7 @@ sub club { sub patron { my ( $self ) = @_; - return scalar Koha::Patrons->find( $self->borrowernumber() ); + return Koha::Patrons->find( $self->borrowernumber() ); } =head3 is_canceled diff --git a/Koha/Club/Field.pm b/Koha/Club/Field.pm index 56f5aa03e8..6f92d83f31 100644 --- a/Koha/Club/Field.pm +++ b/Koha/Club/Field.pm @@ -46,7 +46,7 @@ Represents the value set at creation time for a Koha::Club::Template::Field sub club_template_field { my ( $self ) = @_; - return scalar Koha::Club::Template::Fields->find( $self->club_template_field_id ); + return Koha::Club::Template::Fields->find( $self->club_template_field_id ); } =head3 type diff --git a/Koha/Objects.pm b/Koha/Objects.pm index 31387e1442..fb537581c8 100644 --- a/Koha/Objects.pm +++ b/Koha/Objects.pm @@ -76,6 +76,8 @@ Similar to DBIx::Class::ResultSet->find this method accepts: Strictly speaking, columns_values should only refer to columns under an unique constraint. +It returns undef if no results were found + my $object = Koha::Objects->find( { col1 => $val1, col2 => $val2 } ); my $object = Koha::Objects->find( $id ); my $object = Koha::Objects->find( $idpart1, $idpart2, $attrs ); # composite PK @@ -85,15 +87,14 @@ my $object = Koha::Objects->find( $idpart1, $idpart2, $attrs ); # composite PK sub find { my ( $self, @pars ) = @_; - croak 'Cannot use "->find" in list context' if wantarray; - - return if !@pars || none { defined($_) } @pars; + my $object; - my $result = $self->_resultset()->find( @pars ); - - return unless $result; - - my $object = $self->object_class()->_new_from_dbic( $result ); + unless (!@pars || none { defined($_) } @pars) { + my $result = $self->_resultset()->find(@pars); + if ($result) { + $object = $self->object_class()->_new_from_dbic($result); + } + } return $object; } diff --git a/Koha/Subscription.pm b/Koha/Subscription.pm index 833f674ad4..8213818fa5 100644 --- a/Koha/Subscription.pm +++ b/Koha/Subscription.pm @@ -51,7 +51,7 @@ Returns the biblio linked to this subscription as a Koha::Biblio object sub biblio { my ($self) = @_; - return scalar Koha::Biblios->find($self->biblionumber); + return Koha::Biblios->find($self->biblionumber); } =head3 vendor @@ -62,7 +62,7 @@ Returns the vendor/supplier linked to this subscription as a Koha::Acquisition:: sub vendor { my ($self) = @_; - return scalar Koha::Acquisition::Booksellers->find($self->aqbooksellerid); + return Koha::Acquisition::Booksellers->find($self->aqbooksellerid); } =head3 subscribers diff --git a/acqui/booksellers.pl b/acqui/booksellers.pl index 99da78cb9f..0b69feb497 100755 --- a/acqui/booksellers.pl +++ b/acqui/booksellers.pl @@ -82,7 +82,7 @@ my $allbaskets= $query->param('allbaskets')||0; my @suppliers; if ($booksellerid) { - push @suppliers, scalar Koha::Acquisition::Booksellers->find( $booksellerid ); + push @suppliers, Koha::Acquisition::Booksellers->find( $booksellerid ); } else { @suppliers = Koha::Acquisition::Booksellers->search( { name => { -like => "%$supplier%" } }, diff --git a/acqui/uncertainprice.pl b/acqui/uncertainprice.pl index 4a559e3691..5ecf41d83c 100755 --- a/acqui/uncertainprice.pl +++ b/acqui/uncertainprice.pl @@ -72,7 +72,7 @@ my $op = $input->param('op'); my $owner = $input->param('owner') || 0 ; # flag to see only "my" orders, or everyone orders my $bookseller = Koha::Acquisition::Booksellers->find( $booksellerid ); -$template->param( basket => scalar Koha::Acquisition::Baskets->find($basketno) ); +$template->param( basket => Koha::Acquisition::Baskets->find($basketno) ); #show all orders that have uncertain price for the bookseller my $pendingorders = SearchOrders({ diff --git a/cataloguing/moveitem.pl b/cataloguing/moveitem.pl index 1e664a911e..18474ac343 100755 --- a/cataloguing/moveitem.pl +++ b/cataloguing/moveitem.pl @@ -79,7 +79,7 @@ if ( $barcode && $biblionumber ) { if ($moveresult) { $template->param( success => 1, - from_biblio => scalar Koha::Biblios->find($frombiblionumber), + from_biblio => Koha::Biblios->find($frombiblionumber), ); } else { diff --git a/circ/overdue.pl b/circ/overdue.pl index 0e558dce1e..a1c8bb748e 100755 --- a/circ/overdue.pl +++ b/circ/overdue.pl @@ -309,7 +309,7 @@ if ($noreport) { } push @overduedata, { - patron => scalar Koha::Patrons->find( $data->{borrowernumber} ), + patron => Koha::Patrons->find( $data->{borrowernumber} ), duedate => $data->{date_due}, borrowernumber => $data->{borrowernumber}, cardnumber => $data->{cardnumber}, diff --git a/members/memberentry.pl b/members/memberentry.pl index fa5cbe427e..64f99645c4 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -791,7 +791,7 @@ $template->param( csrf_token => # HouseboundModule data $template->param( - housebound_role => scalar Koha::Patron::HouseboundRoles->find($borrowernumber), + housebound_role => Koha::Patron::HouseboundRoles->find($borrowernumber), ); if(defined($data{'flags'})){ diff --git a/members/pay.pl b/members/pay.pl index 32c8c36dec..0c2e1fc0e5 100755 --- a/members/pay.pl +++ b/members/pay.pl @@ -115,7 +115,7 @@ elsif ( $input->param('confirm_writeoff') ) { Koha::Account->new( { patron_id => $borrowernumber } )->pay( { amount => $amount, - lines => [ scalar Koha::Account::Lines->find($accountlines_id) ], + lines => [ Koha::Account::Lines->find($accountlines_id) ], type => 'WRITEOFF', note => $payment_note, interface => C4::Context->interface, @@ -222,7 +222,7 @@ sub writeoff_all { Koha::Account->new( { patron_id => $borrowernumber } )->pay( { amount => $amount, - lines => [ scalar Koha::Account::Lines->find($accountlines_id) ], + lines => [ Koha::Account::Lines->find($accountlines_id) ], type => 'WRITEOFF', note => $payment_note, interface => C4::Context->interface, diff --git a/t/db_dependent/Koha/Objects.t b/t/db_dependent/Koha/Objects.t index 592ab40866..900efcf480 100644 --- a/t/db_dependent/Koha/Objects.t +++ b/t/db_dependent/Koha/Objects.t @@ -46,13 +46,20 @@ my $borrowernumber_exists = grep { /^borrowernumber$/ } @columns; is( $borrowernumber_exists, 1, 'Koha::Objects->columns should return the table columns' ); subtest 'find' => sub { - plan tests => 4; + plan tests => 6; my $patron = $builder->build({source => 'Borrower'}); my $patron_object = Koha::Patrons->find( $patron->{borrowernumber} ); is( $patron_object->borrowernumber, $patron->{borrowernumber}, '->find should return the correct object' ); - eval { my @patrons = Koha::Patrons->find( $patron->{borrowernumber} ); }; - like( $@, qr|^Cannot use "->find" in list context|, "->find should not be called in list context to avoid side-effects" ); + my @patrons = Koha::Patrons->find( $patron->{borrowernumber} ); + is(scalar @patrons, 1, '->find in list context returns a value'); + is($patrons[0]->borrowernumber, $patron->{borrowernumber}, '->find in list context returns the same value as in scalar context'); + + my $patrons = { + foo => Koha::Patrons->find('foo'), + bar => 'baz', + }; + is ($patrons->{foo}, undef, '->find in list context returns undef when no record is found'); # Test sending undef to find; should not generate a warning warning_is { $patron = Koha::Patrons->find( undef ); } -- 2.20.1