https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 --- Comment #1 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 197895 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197895&action=edit Batch Permissions Modifier Plugin 1.3.1 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 --- Comment #2 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 197900 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197900&action=edit Batch Permissions Modifier Plugin 1.4.1 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 --- Comment #3 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 197913 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197913&action=edit Bug 42430: Fix issue with stale plugin methods after plugin upgrade When a plugin is upgraded in place (by uploading a new version over the old one), the list of methods Koha knows about for that plugin can get out of date. For example, if a plugin used to have a "tool" method and the new version renames it to "admin", the staff interface can still show the old "Run tool" link, and clicking it produces an error saying the plugin has no "tool" method. This happens because the plugin upload runs inside a long-lived Plack worker that has already loaded the old version of the plugin in memory. When Koha scans the plugin to record its methods, Perl has no reason to re-read the file from disk, so the scan sees the old method list and stores that in the plugin_methods table. The plugin code on disk is new, but the record of what methods it has is stale. This patch forces Koha to throw away the in-memory copy of each plugin and reload it from disk before scanning, so the recorded list of methods matches the plugin that is actually installed. Test Plan: 1) Install the Batch Permission Modifier Plugin version 1.3.1 (this version exposes a "tool" method) 2) Go to Tools > Plugins and confirm the plugin row shows "Run tool" in its actions menu 3) Upload Batch Permission Modifier Plugin version 1.4.1 over the top of 1.3.1 (this version exposes "admin" instead of "tool") 4) Reload the Plugins page 5) Note the plugin still shows "Run tool" in its actions menu 6) Click "Run tool" 7) Note the error: the plugin has no "tool" method! 8) Apply this patch 9) Restart all the things! 10) Uninstall the plugin 11) Repeat steps 1-4 12) Note that "Run tool" is gone and "Run admin tool" appears in its place 13) Click "Run admin tool" and confirm it loads without error! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com | |, | |kyle@bywatersolutions.com, | |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197913|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 198060 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198060&action=edit Bug 42430: Fix issue with stale plugin methods after plugin upgrade When a plugin is upgraded in place (by uploading a new version over the old one), the list of methods Koha knows about for that plugin can get out of date. For example, if a plugin used to have a "tool" method and the new version renames it to "admin", the staff interface can still show the old "Run tool" link, and clicking it produces an error saying the plugin has no "tool" method. This happens because the plugin upload runs inside a long-lived Plack worker that has already loaded the old version of the plugin in memory. When Koha scans the plugin to record its methods, Perl has no reason to re-read the file from disk, so the scan sees the old method list and stores that in the plugin_methods table. The plugin code on disk is new, but the record of what methods it has is stale. This patch forces Koha to throw away the in-memory copy of each plugin and reload it from disk before scanning, so the recorded list of methods matches the plugin that is actually installed. Test Plan: 1) Install the Batch Permission Modifier Plugin version 1.3.1 (this version exposes a "tool" method) 2) Go to Tools > Plugins and confirm the plugin row shows "Run tool" in its actions menu 3) Upload Batch Permission Modifier Plugin version 1.4.1 over the top of 1.3.1 (this version exposes "admin" instead of "tool") 4) Reload the Plugins page 5) Note the plugin still shows "Run tool" in its actions menu 6) Click "Run tool" 7) Note the error: the plugin has no "tool" method! 8) Apply this patch 9) Restart all the things! 10) Uninstall the plugin 11) Repeat steps 1-4 12) Note that "Run tool" is gone and "Run admin tool" appears in its place 13) Click "Run admin tool" and confirm it loads without error! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #7 from David Nind <david@davidnind.com> --- Tests pass, but running qa shows: ... Result: [FAIL] Koha/Plugins.pm FAIL pod_coverage sub _verbose has no pod coverage. sub _restart_after_change has no pod coverage. [PASS] t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t [FAIL] t/db_dependent/Koha/Plugins/Plugins.t FAIL critic Stricture disabled at line 287, column 9. See page 429 of PBP. (Severity: 5) ... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 Kristi Krueger <kkrueger@cuyahogalibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kkrueger@cuyahogalibrary.or | |g -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198060|0 |1 is obsolete| | Attachment #198061|0 |1 is obsolete| | Attachment #198088|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 --- Comment #10 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 201414 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201414&action=edit Bug 42430: Add unit tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 --- Comment #12 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 201416 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201416&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|42976 |42977 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42976 [Bug 42976] InstallPlugins reloads every plugin from disk even when only one is being installed https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42977 [Bug 42977] Plugin upload does not tell InstallPlugins which plugin was uploaded -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 --- Comment #14 from David Cook <dcook@prosentient.com.au> --- So I don't think the description is quite right... but the methodology looks like it could work. I'd need to look at this some more but it's a neat idea. Ages ago, I remember looking into Wordpress and PHP, and from memory it uses wp_opcache_invalidate_directory() to clear out cached plugin code. I didn't think Perl had an equivalent, but Symbol::delete_package is quite interesting. It does come with a warning about it being a bit too extreme, but very interesting. And that's what Martin picked up in his follow up. Not sure I understand his reasoning for not using Symbol::delete_package. Will ask... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 --- Comment #15 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Thanks everyone! Pushed to main for 26.11! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42430 --- Comment #17 from David Cook <dcook@prosentient.com.au> --- In the meantime, riddle me this... Won't this change only affect the code that is installing the plugin? So for the CLI tool it will be unnecessary. For the web UI upload, it will only affect the uploading worker and not the other loaded workers. But I'm guessing it's because we're not trying to fix stale code caches but rather just fix the symbol table so that the methods get installed correctly. Personally, I really dislike how we currently capture the methods for plugin_methods but that's a separate issue. Overall, I'm guessing I won't really see this change in action because I don't allow use of the web UI plugin upload? -- You are receiving this mail because: You are watching all bug changes.