From 611328b5dbe2388b5498c36120a97c727d348198 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 1 Jul 2026 22:29:54 +0100 Subject: [PATCH] Bug 42430: (QA follow-up) Fix nested plugin package corruption on reload InstallPlugins forces every discovered plugin class to be deleted and re-required from disk on every call, so plugin_methods reflects the .pm file as it currently exists rather than whatever was already loaded into a long-lived Plack worker. That deletion used Symbol::delete_package, which clears the target package's whole symbol table -- including any already-loaded packages nested under it (e.g. a REST API controller a plugin bundles alongside itself, such as Koha/Plugin/Foo/Controller.pm). Those nested classes keep their own %INC entry, so they are never re-required afterwards and are left permanently blanked out for the rest of the worker's life. This was caught when running `prove -r t/db_dependent/Koha/Plugins/ t/db_dependent/Koha/REST/Plugin/` on top of the original patch. Tests fail several files that have nothing to do with the bug being fixed (Biblio_and_Items_plugin_hooks.t, Holds_hooks.t, TemplateIncludePathHook.t, REST/Plugin/PluginRoutes.t), because Koha::Plugin::Test::Controller (a bundled test-fixture controller living in t/lib/plugins/Koha/Plugin/Test/Controller.pm) gets its symbol table wiped as collateral damage every time Koha::Plugin::Test is reloaded. _forget_plugin_class() replaces the delete_package call with the same technique used by the CPAN module Class::Unload (not added as a dependency here, just the technique): only clear the target class's own symbols and reset its @ISA, explicitly leaving any nested package entries in its symbol table alone. Module::Pluggable discovers bundled nested classes as their own entries in the plugin list regardless, so they still get reloaded correctly on their own turn. I also annotate the no strict 'refs' block added by the previous commit with a '## no critic' justification, as flagged by koha-qa.pl. Test plan: 1. Confirm the bug exists without this patch: a) git checkout (drop this commit, keeping the first 3 commits of the bug applied) b) Inside KTD: prove -r t/db_dependent/Koha/Plugins/ t/db_dependent/Koha/REST/Plugin/ c) Observe failing files unrelated to this bug's own tests: Biblio_and_Items_plugin_hooks.t, Holds_hooks.t, TemplateIncludePathHook.t and REST/Plugin/PluginRoutes.t. 2. Confirm the fix: a) Re-apply this commit b) Re-run the same command from 1(b) -- all tests now pass 3. Confirm the original bug this branch set out to fix is still fixed, both via the manual UI steps from the first commit's test plan (install Batch Permission Modifier Plugin 1.3.1, upload 1.4.1 over it via Tools > Plugins, confirm the renamed method takes effect without restarting Plack) and via: prove t/db_dependent/Koha/Plugins/Plugins.t (see the 'InstallPlugins() reloads stale symbol tables tests' subtest) 4. Confirm QA tooling is clean: koha-qa.pl -c 4 => PASS for Koha/Plugins.pm and all touched test files, no critic or tidiness failures. Signed-off-by: Martin Renvoize Signed-off-by: Pedro Amorim --- Koha/Plugins.pm | 43 ++++++++++++++++--- t/db_dependent/Koha/Plugins/Plugins.t | 2 +- .../Koha/REST/Plugin/PluginRoutes.t | 10 +++-- 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/Koha/Plugins.pm b/Koha/Plugins.pm index a6cd468007..81a099f30e 100644 --- a/Koha/Plugins.pm +++ b/Koha/Plugins.pm @@ -26,7 +26,6 @@ use Module::Load::Conditional qw( can_load ); use Module::Load; use Module::Pluggable search_path => ['Koha::Plugin'], except => qr/::Edifact(|::Line|::Message|::Order|::Segment|::Transport)$/; -use Symbol qw( delete_package ); use Try::Tiny; use POSIX qw(getpid); @@ -335,10 +334,7 @@ sub InstallPlugins { # symbol table during an upgrade, so plugin_methods ends up out of # sync with the installed .pm (e.g. missing newly-added methods or # retaining methods that were renamed or removed). - ( my $plugin_inc_key = $plugin_class ) =~ s{::}{/}g; - $plugin_inc_key .= '.pm'; - delete $INC{$plugin_inc_key}; - delete_package($plugin_class); + $self->_forget_plugin_class($plugin_class); if ( can_load( modules => { $plugin_class => undef }, verbose => $verbose, nocache => 1 ) ) { next unless $plugin_class->isa('Koha::Plugins::Base'); @@ -377,6 +373,43 @@ sub InstallPlugins { return @plugins; } +=head3 _forget_plugin_class + +Force Perl to forget an in-memory plugin class, so a subsequent re-require +picks up the .pm file as it currently exists on disk, rather than whatever +was loaded into this worker previously. + +We deliberately do not use Symbol::delete_package here: it clears the +whole symbol table, including any already-loaded packages nested under +it (e.g. Koha::Plugin::Foo::Controller, bundled by the plugin in its own +Koha/Plugin/Foo/Controller.pm). Since those keep their own %INC entry, +they are never re-required afterwards and are left permanently blanked +out. Module::Pluggable discovers them as their own entries in the plugin +list regardless, so they get reloaded on their own turn; this helper +only ever clears the target class's own symbols, leaving any nested +packages alone. + +=cut + +sub _forget_plugin_class { + my ( $self, $plugin_class ) = @_; + + return unless Class::Inspector->loaded($plugin_class); + + no strict 'refs'; ## no critic (ProhibitNoStrict) + @{"${plugin_class}::ISA"} = (); + my $stash = \%{"${plugin_class}::"}; + for my $symbol ( keys %$stash ) { + next if $symbol =~ /::\z/; # leave nested packages alone + delete $stash->{$symbol}; + } + + ( my $plugin_inc_key = $plugin_class ) =~ s{::}{/}g; + delete $INC{"$plugin_inc_key.pm"}; + + return; +} + =head2 RemovePlugins Koha::Plugins->RemovePlugins( { diff --git a/t/db_dependent/Koha/Plugins/Plugins.t b/t/db_dependent/Koha/Plugins/Plugins.t index c2996520eb..c4547218d2 100755 --- a/t/db_dependent/Koha/Plugins/Plugins.t +++ b/t/db_dependent/Koha/Plugins/Plugins.t @@ -284,7 +284,7 @@ subtest 'InstallPlugins() reloads stale symbol tables tests' => sub { # A run of InstallPlugins should reload from disk and the phantom method # should disappear. { - no strict 'refs'; + no strict 'refs'; ## no critic (ProhibitNoStrict) *{'Koha::Plugin::Test::stale_phantom_method'} = sub { 1 }; } diff --git a/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t b/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t index bacf3f044c..0b6d61d745 100755 --- a/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t +++ b/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t @@ -152,15 +152,17 @@ subtest 'Permissions and access to plugin routes tests' => sub { # enable BASIC auth t::lib::Mocks::mock_preference( 'RESTBasicAuth', 1 ); - # Silence warnings from unrelated plugins feature - my $plugin_mock = Test::MockModule->new('Koha::Plugin::Test'); - $plugin_mock->mock( 'patron_barcode_transform', undef ); - # remove any existing plugins that might interfere Koha::Plugins::Methods->search->delete; my $plugins = Koha::Plugins->new; $plugins->InstallPlugins; + # Silence warnings from unrelated plugins feature. Must be applied + # after InstallPlugins, which reloads plugin classes from disk and + # would otherwise wipe the mock. + my $plugin_mock = Test::MockModule->new('Koha::Plugin::Test'); + $plugin_mock->mock( 'patron_barcode_transform', undef ); + my @plugins = $plugins->GetPlugins( { all => 1 } ); foreach my $plugin (@plugins) { $plugin->enable;