Bugzilla – Attachment 160029 Details for
Bug 35536
Improve removal of Koha plugins in unit tests
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 35536: Refine verbose handling in some Koha::Plugins calls
Bug-35536-Refine-verbose-handling-in-some-KohaPlug.patch (text/plain), 10.84 KB, created by
Marcel de Rooy
on 2023-12-19 15:57:02 UTC
(
hide
)
Description:
Bug 35536: Refine verbose handling in some Koha::Plugins calls
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2023-12-19 15:57:02 UTC
Size:
10.84 KB
patch
obsolete
>From a184eaaa90787f9640148f5e5b56709be952eb27 Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >Date: Mon, 18 Dec 2023 08:52:20 +0000 >Subject: [PATCH] Bug 35536: Refine verbose handling in some Koha::Plugins > calls >Content-Type: text/plain; charset=utf-8 > >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. >--- > 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 76632d275d..38134cc472 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 >@@ -160,7 +160,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 ); > } >@@ -175,12 +175,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 { >@@ -188,7 +196,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; > >@@ -199,11 +210,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; >@@ -234,13 +244,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 ce9d9c5ce2..48dd2941db 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 >@@ -154,9 +154,9 @@ > <span class="label label-warning">ERRORS</span> > </td> > [% IF ( CAN_user_plugins_configure || CAN_user_plugins_manage || CAN_user_plugins_report || CAN_user_plugins_tool ) %] >- <td colspan="7">Error found whilst attempting to load plugin</td> >+ <td colspan="8">Error found whilst attempting to load plugin</td> > [% ELSE %] >- <td colspan="6">Error found whilst attempting to load plugin</td> >+ <td colspan="8">Error found whilst attempting to load plugin</td> > [% END %] > </tr> > [% ELSE %] >diff --git a/misc/devel/install_plugins.pl b/misc/devel/install_plugins.pl >index 8e2cee9ad9..98d7691c6e 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.30.2
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=160029">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 35536
:
159804
|
159820
|
159821
|
159822
|
159823
|
159824
|
159825
|
159830
|
159839
|
159840
|
159841
|
159842
|
159843
|
159844
|
160024
|
160025
|
160026
|
160027
|
160028
|
160029
|
160030
|
160031
|
160032
|
160801
|
160802
|
160803
|
160804
|
160805
|
160806
|
160807
|
160808
|
160809
|
160817
|
160818
|
160819
|
160820
|
160821
|
160822
|
160823
|
160824
|
160825
|
160881
|
160882
|
160883
|
160884
|
160885
|
160886
|
160887
|
160888
|
160889
|
163701
|
163797