From 54787dee7cb480c600af07ff5058d822263b716c 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. Also, fix Koha::Patron::guarantor which had the same problem as find 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 | 18 +++++++++--------- Koha/Patron.pm | 7 +++++-- 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 ++-- opac/opac-memberentry.pl | 2 +- t/db_dependent/Koha/Objects.t | 13 ++++++++++--- 16 files changed, 41 insertions(+), 31 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 097a5c09d6..93f1022c63 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -800,7 +800,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 63e1a06dbb..cf27e8098e 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 168f55bca6..788f956984 100644 --- a/Koha/Club/Enrollment.pm +++ b/Koha/Club/Enrollment.pm @@ -59,7 +59,7 @@ sub cancel { sub club { my ( $self ) = @_; - return scalar Koha::Clubs->find( $self->club_id() ); + return Koha::Clubs->find( $self->club_id() ); } =head3 patron @@ -68,7 +68,7 @@ sub club { sub patron { my ( $self ) = @_; - return scalar Koha::Patrons->find( $self->borrowernumber() ); + return Koha::Patrons->find( $self->borrowernumber() ); } =head3 type 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 da43003c88..a3251dfd5d 100644 --- a/Koha/Objects.pm +++ b/Koha/Objects.pm @@ -85,15 +85,15 @@ 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 $result = $self->_resultset()->find( @pars ); - - return unless $result; - - my $object = $self->object_class()->_new_from_dbic( $result ); + my $object; + + @pars = grep { defined } @pars; + if (@pars) { + my $result = $self->_resultset()->find(@pars); + if ($result) { + $object = $self->object_class()->_new_from_dbic($result); + } + } return $object; } diff --git a/Koha/Patron.pm b/Koha/Patron.pm index b1d9b14620..dd560ebf95 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -374,9 +374,12 @@ Returns a Koha::Patron object for this patron's guarantor sub guarantor { my ( $self ) = @_; - return unless $self->guarantorid(); + my $guarantor; + if ($self->guarantorid()) { + $guarantor = Koha::Patrons->find( $self->guarantorid() ); + } - return Koha::Patrons->find( $self->guarantorid() ); + return $guarantor; } sub image { 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 697057ef03..97ac16ee4b 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 0b6f0f802a..e112c6dd9f 100755 --- a/circ/overdue.pl +++ b/circ/overdue.pl @@ -299,7 +299,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 bd86e36b67..f944925e06 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -800,7 +800,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 dd7a1efb5f..df4cb91d0e 100755 --- a/members/pay.pl +++ b/members/pay.pl @@ -110,7 +110,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, @@ -214,7 +214,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/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index f76a30c098..06dc8da572 100755 --- a/opac/opac-memberentry.pl +++ b/opac/opac-memberentry.pl @@ -312,7 +312,7 @@ elsif ( $action eq 'edit' ) { #Display logged in borrower's data $template->param( borrower => $borrower, - guarantor => scalar Koha::Patrons->find($borrowernumber)->guarantor(), + guarantor => Koha::Patrons->find($borrowernumber)->guarantor(), hidden => GetHiddenFields( $mandatory, 'edit' ), csrf_token => Koha::Token->new->generate_csrf({ session_id => scalar $cgi->cookie('CGISESSID'), diff --git a/t/db_dependent/Koha/Objects.t b/t/db_dependent/Koha/Objects.t index db7711e0db..51d20649f7 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