From b00f04a7609a958352142fc50e20a34d431e6d64 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 18 Dec 2023 08:52:20 +0000 Subject: [PATCH] Bug 35536: Refine verbose handling in some Koha::Plugins calls Three routines in Plugins got the verbose parameter on 35507. We can refine this a bit further. The idea here is report when you are installing plugins but not report when just calling plugins (flooding logs). [1] GetPlugins: Most callers do not expect (or check) results for failing plugins. This patch makes GetPlugins only return errors when passing the *errors* flag (in 2 cases). [a] The misc/devel script prints warnings now using verbose, so does not need the errors flag anymore. [b] plugins/plugins-home is the only case left. Tiny adjustment to keep current behavior. Fixed colspan in template. Does not need verbose in favor of 'errors' (passed to template). [c] For most calls we do not want verbose. New default is 0. [2] InstallPlugins [a] Disabled verbose in plugin-upload. Not really needed. Added a FIXME; we need to improve individual install. [b] misc/devel: No warnings anymore when calling InstallPlugins after GetPlugins. [3] get_enabled_plugins [a] Plugins->call does not need verbose. [b] Plugins->feature_enabled does not need it too. Test plan: [1] See previous plan. With TestMR data but without patch, run misc script and go to plugins-home. Do you see load errors on commandline or form? [2] Run plugins/plugins-upload (uploading just some file is good enough); verify that you do not see TestMR lines in logfile. [3] Run t/db_dependent/Koha/Plugins/Plugins.t for the additional test on verbose and errors flag. Signed-off-by: Martin Renvoize Signed-off-by: Kyle M Hall Signed-off-by: Katrin Fischer (cherry picked from commit d8e04545b80869821057d5b2c3ac46f6e18b1b78) Signed-off-by: Fridolin Somers --- Koha/Plugins.pm | 29 ++++++++++++------- .../prog/en/modules/plugins/plugins-home.tt | 4 +-- misc/devel/install_plugins.pl | 13 +++------ plugins/plugins-home.pl | 4 +-- plugins/plugins-upload.pl | 5 +++- t/db_dependent/Koha/Plugins/Plugins.t | 26 ++++++++++++++++- 6 files changed, 56 insertions(+), 25 deletions(-) diff --git a/Koha/Plugins.pm b/Koha/Plugins.pm index 3f2bed2196..6379e43db0 100644 --- a/Koha/Plugins.pm +++ b/Koha/Plugins.pm @@ -81,7 +81,7 @@ sub call { return unless C4::Context->config('enable_plugins'); my @responses; - my @plugins = $class->get_enabled_plugins(); + my @plugins = $class->get_enabled_plugins( { verbose => 0 } ); @plugins = grep { $_->can($method) } @plugins; # TODO: Remove warn when after_hold_create is removed from the codebase @@ -167,7 +167,7 @@ sub feature_enabled { my $key = "ENABLED_PLUGIN_FEATURE_" . $method; my $feature = Koha::Cache::Memory::Lite->get_from_cache($key); unless ( defined($feature) ) { - my @plugins = $class->get_enabled_plugins(); + my @plugins = $class->get_enabled_plugins( { verbose => 0 } ); my $enabled = any { $_->can($method) } @plugins; Koha::Cache::Memory::Lite->set_in_cache( $key, $enabled ); } @@ -182,12 +182,20 @@ method or metadata value. my @plugins = Koha::Plugins::GetPlugins({ method => 'some_method', metadata => { some_key => 'some_value' }, - [ verbose => 1 ], + [ all => 1, errors => 1, verbose => 1 ], }); The method and metadata parameters are optional. If you pass multiple keys in the metadata hash, all keys must match. +If you pass errors (only used in plugins-home), we return two arrayrefs: + + ( $good, $bad ) = Koha::Plugins::GetPlugins( { errors => 1 } ); + +If you pass verbose, you can enable or disable explicitly warnings +from Module::Load::Conditional. Disabled by default to not flood +the logs. + =cut sub GetPlugins { @@ -195,7 +203,10 @@ sub GetPlugins { my $method = $params->{method}; my $req_metadata = $params->{metadata} // {}; - my $verbose = delete $params->{verbose} // $self->_verbose; + my $errors = $params->{errors}; + + # By default dont warn here unless asked to do so. + my $verbose = $params->{verbose} // 0; my $filter = ( $method ) ? { plugin_method => $method } : undef; @@ -206,11 +217,10 @@ sub GetPlugins { } )->_resultset->get_column('plugin_class'); - my @plugins; # Loop through all plugins that implement at least a method + my ( @plugins, @failing ); while ( my $plugin_class = $plugin_classes->next ) { - if ( can_load( modules => { $plugin_class => undef }, verbose => $verbose, nocache => 1 ) ) { my $plugin; @@ -241,13 +251,12 @@ sub GetPlugins { and any { !$plugin_metadata->{$_} || $plugin_metadata->{$_} ne $req_metadata->{$_} } keys %$req_metadata; push @plugins, $plugin; - } elsif ( defined($params->{errors}) && $params->{errors} ){ - push @plugins, { error => 'cannot_load', name => $plugin_class }; + } elsif( $errors ) { + push @failing, { error => 1, name => $plugin_class }; } - } - return @plugins; + return $errors ? ( \@plugins, \@failing ) : @plugins; } =head2 InstallPlugins diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/plugins/plugins-home.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/plugins/plugins-home.tt index 2ab2d916b7..5cf2fb5889 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/plugins/plugins-home.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/plugins/plugins-home.tt @@ -155,9 +155,9 @@ ERRORS [% IF ( CAN_user_plugins_configure || CAN_user_plugins_manage || CAN_user_plugins_report || CAN_user_plugins_tool ) %] - Error found whilst attempting to load plugin + Error found whilst attempting to load plugin [% ELSE %] - Error found whilst attempting to load plugin + Error found whilst attempting to load plugin [% END %] [% ELSE %] diff --git a/misc/devel/install_plugins.pl b/misc/devel/install_plugins.pl index 74f43edc56..2ccc68a15b 100755 --- a/misc/devel/install_plugins.pl +++ b/misc/devel/install_plugins.pl @@ -38,20 +38,15 @@ unless ( C4::Context->config("enable_plugins") ) { } my @existing_plugins = Koha::Plugins->new()->GetPlugins({ - all => 1, - errors => 1, + all => 1, + verbose => 1, # warns about plugins failing to load }); my $existing_plugins; for my $existing_plugin (@existing_plugins) { - if( defined $existing_plugin->{error} ){ - print "Could not load: ".$existing_plugin->{name}." any associated method will be removed\n"; - } else { - $existing_plugins->{ $existing_plugin->{metadata}->{name} } = - $existing_plugin->{metadata}->{version}; - } + $existing_plugins->{ $existing_plugin->{metadata}->{name} } = $existing_plugin->{metadata}->{version}; } -my @installed_plugins = Koha::Plugins->new()->InstallPlugins(); +my @installed_plugins = Koha::Plugins->new()->InstallPlugins( { verbose => 0 } ); unless (@installed_plugins) { my $plugins_dir = C4::Context->config("pluginsdir"); if ( ref($plugins_dir) eq 'ARRAY' ) { diff --git a/plugins/plugins-home.pl b/plugins/plugins-home.pl index 08d005f1cf..60b3cde213 100755 --- a/plugins/plugins-home.pl +++ b/plugins/plugins-home.pl @@ -50,7 +50,7 @@ if ($plugins_enabled) { method => $method, ); - my @plugins = Koha::Plugins->new()->GetPlugins( + my ( $plugins, $failures ) = Koha::Plugins->new()->GetPlugins( { method => $method, all => 1, @@ -58,7 +58,7 @@ if ($plugins_enabled) { } ); - $template->param( plugins => \@plugins, ); + $template->param( plugins => [ @$plugins, @$failures ] ); $template->param( plugins_restricted => C4::Context->config('plugins_restricted') ); $template->param( can_search => C4::Context->config('plugin_repos') ? 1 : 0 ); diff --git a/plugins/plugins-upload.pl b/plugins/plugins-upload.pl index 02fd2548c7..bbf4a8c7b7 100755 --- a/plugins/plugins-upload.pl +++ b/plugins/plugins-upload.pl @@ -124,7 +124,10 @@ if ( ( $op eq 'Upload' ) && ( $uploadfile || $uploadlocation ) ) { exit; } - Koha::Plugins->new()->InstallPlugins(); + # Install plugins; verbose not needed, we redirect to plugins-home. + # FIXME There is no good way to verify the install below; we need an + # individual plugin install. + Koha::Plugins->new->InstallPlugins( { verbose => 0 } ); } } elsif ( ( $op eq 'Upload' ) && !$uploadfile && !$uploadlocation ) { warn "Problem uploading file or no file uploaded."; diff --git a/t/db_dependent/Koha/Plugins/Plugins.t b/t/db_dependent/Koha/Plugins/Plugins.t index 3838d46cbe..53ef4f9a31 100755 --- a/t/db_dependent/Koha/Plugins/Plugins.t +++ b/t/db_dependent/Koha/Plugins/Plugins.t @@ -24,7 +24,7 @@ use File::Temp qw( tempdir tempfile ); use FindBin qw($Bin); use Module::Load::Conditional qw(can_load); use Test::MockModule; -use Test::More tests => 17; +use Test::More tests => 18; use Test::Warn; use C4::Context; @@ -404,6 +404,30 @@ subtest 'RemovePlugins' => sub { $schema->storage->txn_rollback; }; +subtest 'verbose and errors flag' => sub { + plan tests => 6; + + $schema->storage->txn_begin; + t::lib::Mocks::mock_config( 'enable_plugins', 1 ); + Koha::Plugins->RemovePlugins( { destructive => 1 } ); # clean start + + our $class_basename = 'Koha::Plugin::TestMR::' . time; + my $plugin_mocks = []; + + # Recreate TestMR plugins without module and mock on Module::Load::Conditional + reload_plugin( $_, $plugin_mocks ) for 1 .. 2; + warnings_like { Koha::Plugins->new->GetPlugins } [], 'Verbose was off'; + warnings_like { Koha::Plugins->new->GetPlugins( { verbose => 1 } ) } + [ qr/TestMR/, qr/TestMR/ ], 'Verbose was on, two warns'; + + my ( $plugins, $failures ) = Koha::Plugins->new->GetPlugins( { errors => 1 } ); + is( @$plugins, 0, 'No good plugins left' ); + is( @$failures, 2, 'Two failing plugins' ); + is( $failures->[0]->{error}, 1, 'Failure hash contains error key' ); + is( $failures->[0]->{name}, "${class_basename}1", 'Failure hash contains name key' ); + + $schema->storage->txn_rollback; +}; $schema->storage->txn_begin; # Matching rollback at very end -- 2.39.5