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 <previous commit> (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 <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

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( {

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 };
     }
 

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;