From 18c15007725e88ffeb79f5d17dd3d3c4f7e978d8 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 23 Apr 2021 11:53:51 -0400 Subject: [PATCH] Bug 28211: Replace use of call_recursive() with call() This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Plugins/Plugins.t 3) prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t Signed-off-by: Marcel de Rooy Signed-off-by: Jonathan Druart --- C4/Circulation.pm | 2 +- C4/SIP/Sip/MsgType.pm | 16 ++++++------ Koha/Patron.pm | 6 +++-- Koha/Plugins.pm | 35 --------------------------- circ/circulation.pl | 5 ++-- members/memberentry.pl | 4 ++- opac/sco/sco-main.pl | 2 +- t/db_dependent/Koha/Plugins/Plugins.t | 22 ++++++++--------- t/lib/Koha/Plugin/Test.pm | 8 +++--- 9 files changed, 36 insertions(+), 64 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 5342668ba3..a43c104d53 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -166,7 +166,7 @@ sub barcodedecode { my ($barcode, $filter) = @_; my $branch = C4::Context::mybranch(); $filter = C4::Context->preference('itemBarcodeInputFilter') unless $filter; - ($barcode) = Koha::Plugins->call_recursive('item_barcode_transform', $barcode ); + Koha::Plugins->call('item_barcode_transform', \$barcode ); $filter or return $barcode; # ensure filter is defined, else return untouched barcode if ($filter eq 'whitespace') { $barcode =~ s/\s//g; diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm index 6de5f33067..d18029dcd0 100644 --- a/C4/SIP/Sip/MsgType.pm +++ b/C4/SIP/Sip/MsgType.pm @@ -522,7 +522,7 @@ sub handle_checkout { $fields = $self->{fields}; $patron_id = $fields->{ (FID_PATRON_ID) }; - ($patron_id) = Koha::Plugins->call_recursive('patron_barcode_transform', $patron_id ); + Koha::Plugins->call('patron_barcode_transform', \$patron_id ); $item_id = $fields->{ (FID_ITEM_ID) }; my $fee_ack = $fields->{ (FID_FEE_ACK) }; @@ -750,7 +750,7 @@ sub handle_block_patron { $patron_id = $fields->{ (FID_PATRON_ID) }; $terminal_pwd = $fields->{ (FID_TERMINAL_PWD) }; - ($patron_id) = Koha::Plugins->call_recursive('patron_barcode_transform', $patron_id ); + Koha::Plugins->call('patron_barcode_transform', \$patron_id ); # Terminal passwords are different from account login # passwords, but I have no idea what to do with them. So, @@ -976,7 +976,7 @@ sub handle_patron_info { $start = $fields->{ (FID_START_ITEM) }; $end = $fields->{ (FID_END_ITEM) }; - ($patron_id) = Koha::Plugins->call_recursive('patron_barcode_transform', $patron_id ); + Koha::Plugins->call('patron_barcode_transform', \$patron_id ); $patron = $ils->find_patron($patron_id); @@ -1135,7 +1135,7 @@ sub handle_fee_paid { $fee_id = $fields->{ (FID_FEE_ID) }; $trans_id = $fields->{ (FID_TRANSACTION_ID) }; - ($patron_id) = Koha::Plugins->call_recursive('patron_barcode_transform', $patron_id ); + Koha::Plugins->call('patron_barcode_transform', \$patron_id ); $ils->check_inst_id( $inst_id, "handle_fee_paid" ); @@ -1339,7 +1339,7 @@ sub handle_patron_enable { $patron_id = $fields->{ (FID_PATRON_ID) }; $patron_pwd = $fields->{ (FID_PATRON_PWD) }; - ($patron_id) = Koha::Plugins->call_recursive('patron_barcode_transform', $patron_id ); + Koha::Plugins->call('patron_barcode_transform', \$patron_id ); siplog( "LOG_DEBUG", "handle_patron_enable: patron_id: '%s', patron_pwd: '%s'", $patron_id, $patron_pwd ); @@ -1404,7 +1404,7 @@ sub handle_hold { $title_id = $fields->{ (FID_TITLE_ID) } || ''; $fee_ack = $fields->{ (FID_FEE_ACK) } || 'N'; - ($patron_id) = Koha::Plugins->call_recursive('patron_barcode_transform', $patron_id ); + Koha::Plugins->call('patron_barcode_transform', \$patron_id ); if ( $hold_mode eq '+' ) { $status = $ils->add_hold( $patron_id, $patron_pwd, $item_id, $title_id, $expiry_date, $pickup_locn, $hold_type, $fee_ack ); @@ -1471,7 +1471,7 @@ sub handle_renew { $item_props = $fields->{ (FID_ITEM_PROPS) }; $fee_ack = $fields->{ (FID_FEE_ACK) }; - ($patron_id) = Koha::Plugins->call_recursive('patron_barcode_transform', $patron_id ); + Koha::Plugins->call('patron_barcode_transform', \$patron_id ); $status = $ils->renew( $patron_id, $patron_pwd, $item_id, $title_id, $no_block, $nb_due_date, $third_party, $item_props, $fee_ack ); @@ -1554,7 +1554,7 @@ sub handle_renew_all { $terminal_pwd = $fields->{ (FID_TERMINAL_PWD) }; $fee_ack = $fields->{ (FID_FEE_ACK) }; - ($patron_id) = Koha::Plugins->call_recursive('patron_barcode_transform', $patron_id ); + Koha::Plugins->call('patron_barcode_transform', \$patron_id ); $status = $ils->renew_all( $patron_id, $patron_pwd, $fee_ack ); diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 9fd93e8c44..6160c440b7 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -112,7 +112,8 @@ Autogenerate next cardnumber from highest value found in database sub fixup_cardnumber { my ( $self ) = @_; - my ( $max ) = Koha::Plugins->call_recursive( 'patron_barcode_transform', $self->cardnumber ); + my $max = $self->cardnumber; + Koha::Plugins->call( 'patron_barcode_transform', \$max ); $max ||= Koha::Patrons->search({ cardnumber => {-regexp => '^-?[0-9]+$'} @@ -201,7 +202,8 @@ sub store { $self->trim_whitespaces; - my ( $new_cardnumber ) = Koha::Plugins->call_recursive( 'patron_barcode_transform', $self->cardnumber ); + my $new_cardnumber = $self->cardnumber; + Koha::Plugins->call( 'patron_barcode_transform', \$new_cardnumber ); $self->cardnumber( $new_cardnumber ); # Set surname to uppercase if uppercasesurname is true diff --git a/Koha/Plugins.pm b/Koha/Plugins.pm index 5cb9107cdb..fa4d235287 100644 --- a/Koha/Plugins.pm +++ b/Koha/Plugins.pm @@ -85,41 +85,6 @@ sub call { return @responses; } -=head2 call_recursive - -Calls a plugin method for all enabled plugins, -passing the return value from the previous plugin -to the next one. - - @response = Koha::Plugins->call_recursive($method, @args) - -=cut - -sub call_recursive { - my ( $class, $method, @args ) = @_; - - my @responses; - if ( C4::Context->config('enable_plugins') ) { - - my @plugins = $class->new( { enable_plugins => 1 } )->GetPlugins( { method => $method } ); - @plugins = grep { $_->can($method) } @plugins; - - foreach my $plugin (@plugins) { - my @response = eval { $plugin->$method(@args) }; - - if ($@) { - warn sprintf( "Plugin error (%s): %s", $plugin->get_metadata->{name}, $@ ); - next; - } else { - @args = @response; - } - } - - } - - return @args; -} - =head2 GetPlugins This will return a list of all available plugins, optionally limited by diff --git a/circ/circulation.pl b/circ/circulation.pl index 0ec0156612..261f26dd57 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -75,7 +75,8 @@ my $autoswitched; my $borrowernumber = $query->param('borrowernumber'); if (C4::Context->preference("AutoSwitchPatron") && $barcode) { - my ( $new_barcode ) = Koha::Plugins->call_recursive( 'patron_barcode_transform', $findborrower ); + my $new_barcode = $findborrower; + Koha::Plugins->call( 'patron_barcode_transform', \$new_barcode ); if (Koha::Patrons->search( { cardnumber => $new_barcode} )->count() > 0) { $findborrower = $barcode; undef $barcode; @@ -221,7 +222,7 @@ if ( @$barcodes == 0 && $charges eq 'yes' ) { # my $message; if ($findborrower) { - ( $findborrower ) = Koha::Plugins->call_recursive( 'patron_barcode_transform', $findborrower ); + Koha::Plugins->call( 'patron_barcode_transform', \$findborrower ); my $patron = Koha::Patrons->find( { cardnumber => $findborrower } ); if ( $patron ) { $borrowernumber = $patron->borrowernumber; diff --git a/members/memberentry.pl b/members/memberentry.pl index d85e55e54a..50c86a4c13 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -325,7 +325,9 @@ if ($op eq 'save' || $op eq 'insert'){ # If the cardnumber is blank, treat it as null. $newdata{'cardnumber'} = undef if $newdata{'cardnumber'} =~ /^\s*$/; - my ( $new_barcode ) = Koha::Plugins->call_recursive( 'patron_barcode_transform', $newdata{'cardnumber'} ); + my $new_barcode = $newdata{'cardnumber'}; + Koha::Plugins->call( 'patron_barcode_transform', \$new_barcode ); + $newdata{'cardnumber'} = $new_barcode; if (my $error_code = checkcardnumber( $newdata{cardnumber}, $borrowernumber )){ diff --git a/opac/sco/sco-main.pl b/opac/sco/sco-main.pl index df3d619e57..073763186e 100755 --- a/opac/sco/sco-main.pl +++ b/opac/sco/sco-main.pl @@ -120,7 +120,7 @@ if (C4::Context->preference('SelfCheckoutByLogin') && !$patronid) { my ( $borrower, $patron ); if ( $patronid ) { - ( $patronid ) = Koha::Plugins->call_recursive( 'patron_barcode_transform', $patronid ); + Koha::Plugins->call( 'patron_barcode_transform', \$patronid ); $patron = Koha::Patrons->find( { cardnumber => $patronid } ); $borrower = $patron->unblessed if $patron; } diff --git a/t/db_dependent/Koha/Plugins/Plugins.t b/t/db_dependent/Koha/Plugins/Plugins.t index 0d898f9d4e..3f48adb1c3 100755 --- a/t/db_dependent/Koha/Plugins/Plugins.t +++ b/t/db_dependent/Koha/Plugins/Plugins.t @@ -86,8 +86,8 @@ subtest 'call() tests' => sub { $schema->storage->txn_rollback; }; -subtest 'call_recursive() tests' => sub { - plan tests => 6; +subtest 'more call() tests' => sub { + plan tests => 3; $schema->storage->txn_begin; # Temporarily remove any installed plugins data @@ -100,18 +100,18 @@ subtest 'call_recursive() tests' => sub { $plugin->enable(); } - my (@responses) = Koha::Plugins->call_recursive('test_call_recursive', 1); - is( scalar @responses, 1, "Got back one element" ); - is( $responses[0], 11, "Got expected response" ); + my $bc = 1; + Koha::Plugins->call('item_barcode_transform', \$bc); + is( $bc, 4, "Got expected response" ); - (@responses) = Koha::Plugins->call_recursive('test_call_recursive', 'abcd'); - is( scalar @responses, 1, "Got back one element" ); - is( $responses[0], 'abcdabcd', "Got expected response" ); + my $cn = 'abcd'; + Koha::Plugins->call('item_barcode_transform', \$cn); + is( $cn, 'abcd', "Got expected response" ); t::lib::Mocks::mock_config('enable_plugins', 0); - (@responses) = Koha::Plugins->call_recursive('test_call_recursive', 1); - is( scalar @responses, 1, "Got back one element" ); - is( $responses[0], 1, "call_recursive should return the original arguments if plugins are disabled" ); + $bc = 1; + Koha::Plugins->call('item_barcode_transform', \$bc); + is( $bc, 1, "call should return the original arguments if plugins are disabled" ); $schema->storage->txn_rollback; }; diff --git a/t/lib/Koha/Plugin/Test.pm b/t/lib/Koha/Plugin/Test.pm index af8c35cd34..aa056526e5 100644 --- a/t/lib/Koha/Plugin/Test.pm +++ b/t/lib/Koha/Plugin/Test.pm @@ -95,13 +95,15 @@ sub intranet_js { sub item_barcode_transform { my ( $self, $barcode ) = @_; - Koha::Exceptions::Exception->throw("item_barcode_transform called with parameter: $barcode"); + my $param = $$barcode; + $$barcode = 4 if "$$barcode" eq 1; + Koha::Exceptions::Exception->throw("item_barcode_transform called with parameter: $param"); } sub patron_barcode_transform { my ( $self, $barcode ) = @_; - $barcode //= ''; - Koha::Exceptions::Exception->throw("patron_barcode_transform called with parameter: $barcode"); + $$barcode //= ''; + Koha::Exceptions::Exception->throw("patron_barcode_transform called with parameter: $$barcode"); } sub configure { -- 2.39.5