Bug 38384

Summary: General fix for plugins breaking database transactions
Product: Koha Reporter: Philip Orr <philip.orr>
Component: Architecture, internals, and plumbingAssignee: Paul Derscheid <paul.derscheid>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: david, dcook, kyle, martin.renvoize, nick, paul.derscheid
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 38384: Trigger plugin modules loading as soon as possible in Koha
koha-plugin-fancyplugin from KohaAdvent
Test case (script) to reproduce the breaking database transaction
Bug 38384: Inline plugin loading into C4::Context's BEGIN block
Bug 38384: Prevent initialization failure by checking for existence of plugin_data first

Description Philip Orr 2024-11-07 09:56:55 UTC
In Bug 36736, a call to get_enabled_plugins was added to background jobs to ensure plugins wouldn't break the database transaction.
We need a fix for this problem in general (Database transactions breaking due to plugin hooks being called), not just for background jobs.
Comment 1 Katrin Fischer 2024-11-07 10:20:54 UTC
We had discussed that at the hackfest and it has caused great trouble for some plugin users. We should also consider adding a warning to documentation, even the advent calendar.
Comment 2 Kyle M Hall (khall) 2024-11-07 11:53:53 UTC
Created attachment 174150 [details] [review]
Bug 38384: Trigger plugin modules loading as soon as possible in Koha
Comment 3 Kyle M Hall (khall) 2024-11-07 11:54:29 UTC
I think this may solve the issue. I don't have a specific test plan to move this forward though.
Comment 4 Katrin Fischer 2024-11-08 08:23:11 UTC
Is this ready for testing, Kyle? And thanks!
Comment 5 Paul Derscheid 2024-11-08 09:17:58 UTC
The problem is that this fix creates a circular dependency that just breaks.

We actually need a way to move the logic within 'get_enabled_plugins' into the BEGIN block of C4::Context w/o depending on Koha::Plugins at all.
Comment 6 Paul Derscheid 2024-11-08 09:21:02 UTC
To test (preliminary):
1) Get the koha-plugin-fancyplugin from KohaAdvent.
2) Install it
3) Use test_transaction.pl and observe that due to the reinitialization, the transaction breaks and the biblionumber is returned(!) but after that we can't find the corresponding biblio.

Then:
4) Find a way to implement comment #5
5) Apply your patch
6) See that the biblio is found by Koha::Biblios->find
Comment 7 Paul Derscheid 2024-11-08 09:22:25 UTC
Created attachment 174264 [details]
koha-plugin-fancyplugin from KohaAdvent
Comment 8 Paul Derscheid 2024-11-08 09:23:21 UTC
Created attachment 174265 [details]
Test case (script) to reproduce the breaking database transaction
Comment 9 Paul Derscheid 2024-11-08 09:26:30 UTC
Using Koha::Cache::Memory::Lite in C4::Context's BEGIN should be find, though.
Comment 10 Paul Derscheid 2024-11-08 15:58:03 UTC
Created attachment 174280 [details] [review]
Bug 38384: Inline plugin loading into C4::Context's BEGIN block
Comment 11 Paul Derscheid 2024-11-08 15:59:48 UTC
Not too sure on that latest patch. I essentially only translated the call to Koha::Plugins->get_enabled_plugins to an inlined version without dependencies on other local modules.

This is however very ugly, I am just curious what others think. Thoughts?
Comment 12 Paul Derscheid 2024-11-08 16:10:42 UTC
Created attachment 174282 [details] [review]
Bug 38384: Prevent initialization failure by checking for existence of plugin_data first
Comment 13 David Cook 2024-11-10 22:54:28 UTC
I don't think we should be using XML::Simple here, even for a simple use case. Worth exploring other fast alternatives though...

Btw, you should be able to use Koha::Database instead of DBI directly there, since it's already lightweight (and uses the same modules, so it's same-same but with less duplicated code).
Comment 14 David Cook 2024-11-10 23:05:08 UTC
(In reply to David Cook from comment #13)
> Btw, you should be able to use Koha::Database instead of DBI directly there,
> since it's already lightweight (and uses the same modules, so it's same-same
> but with less duplicated code).

For instance, look at bug 37657 where C4::Context->dbh is replaced by Koha::Database->dbh for big speed gain.
Comment 15 Nick Clemens (kidclamp) 2024-11-12 17:13:02 UTC
We have "TableExists" in C4::Installer - it doesn't need a different syntax per engine, could we do the same here? Select all form the table and check if any warning
Comment 16 David Cook 2024-11-12 22:14:47 UTC
(In reply to Nick Clemens (kidclamp) from comment #15)
> We have "TableExists" in C4::Installer - it doesn't need a different syntax
> per engine, could we do the same here? Select all form the table and check
> if any warning

Did you comment on the right bug here, Nick?
Comment 17 Philip Orr 2024-11-13 19:25:52 UTC
@David I think Nick means the "TableExists" here:
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174282&action=diff&headers=0#a/C4/Context.pm_sec3