Bug 38384 - General fix for plugins breaking database transactions
Summary: General fix for plugins breaking database transactions
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical
Assignee: Paul Derscheid
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 36736
Blocks:
  Show dependency treegraph
 
Reported: 2024-11-07 09:56 UTC by Philip Orr
Modified: 2025-11-28 09:41 UTC (History)
7 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: Small patch
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 (1.46 KB, patch)
2024-11-07 11:53 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
koha-plugin-fancyplugin from KohaAdvent (13.29 KB, application/zip)
2024-11-08 09:22 UTC, Paul Derscheid
Details
Test case (script) to reproduce the breaking database transaction (632 bytes, text/x-perl-script)
2024-11-08 09:23 UTC, Paul Derscheid
Details
Bug 38384: Inline plugin loading into C4::Context's BEGIN block (4.88 KB, patch)
2024-11-08 15:58 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38384: Prevent initialization failure by checking for existence of plugin_data first (5.29 KB, patch)
2024-11-08 16:10 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38384: Trigger plugin modules loading as soon as possible in Koha (1.66 KB, patch)
2025-11-17 16:36 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38384: Inline plugin loading into C4::Context's BEGIN block (4.80 KB, patch)
2025-11-17 16:36 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38384: Prevent initialization failure by checking for existence of plugin_data first (5.35 KB, patch)
2025-11-17 16:36 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38384: Refactor plugin loading to use lightweight Koha::Plugins::Loader (20.38 KB, patch)
2025-11-17 16:36 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38384: Refactor plugin loading to use lightweight Koha::Plugins::Loader (20.38 KB, patch)
2025-11-17 16:41 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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
Comment 18 Nick Clemens (kidclamp) 2025-03-12 20:16:31 UTC
(In reply to Philip Orr from comment #17)
> @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

Yeah, just suggesting we could use existing code in C4/Installer:
 719 sub TableExists {    # Could be renamed table_exists for consistency
 720     my $table = shift;
 721     eval {
 722         my $dbh = C4::Context->dbh;
 723         local $dbh->{PrintError} = 0;
 724         local $dbh->{RaiseError} = 1;
 725         $dbh->do(qq{SELECT * FROM $table WHERE 1 = 0 });
 726     };
 727     return 1 unless $@;
 728     return 0;
Comment 19 Martin Renvoize (ashimema) 2025-11-17 16:36:40 UTC
Created attachment 189636 [details] [review]
Bug 38384: Trigger plugin modules loading as soon as possible in Koha

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 20 Martin Renvoize (ashimema) 2025-11-17 16:36:42 UTC
Created attachment 189637 [details] [review]
Bug 38384: Inline plugin loading into C4::Context's BEGIN block

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 21 Martin Renvoize (ashimema) 2025-11-17 16:36:45 UTC
Created attachment 189638 [details] [review]
Bug 38384: Prevent initialization failure by checking for existence of plugin_data first

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 22 Martin Renvoize (ashimema) 2025-11-17 16:36:47 UTC
Created attachment 189639 [details] [review]
Bug 38384: Refactor plugin loading to use lightweight Koha::Plugins::Loader

This patch addresses the circular dependency issues and code quality
concerns identified in QA by introducing a clean architectural solution.

Changes:

1. Created Koha::Plugins::Loader - A lightweight plugin loader with
   minimal dependencies that can be safely called during early
   initialization. Uses DBIx::Class when available for proper
   transaction support, falls back to raw DBI for early init.

2. Added Koha::Database::table_exists() - Consolidated table checking
   logic into a reusable method that can be called as both class and
   instance method.

3. Simplified C4::Context BEGIN block - Reduced from ~120 lines to
   ~13 lines (91% reduction). Removed XML::Simple dependency and
   manual database connection code.

4. Updated Koha::Plugins::get_enabled_plugins() - Now delegates to
   the Loader module for consistency.

5. Updated C4::Installer::TableExists - Delegates to the new
   Koha::Database::table_exists method.

This addresses all QA concerns:
- Eliminates XML::Simple usage (David Cook)
- Uses Koha::Database instead of manual DBI (David Cook)
- Reuses table checking logic (Nick Clemens)
- Eliminates circular dependencies (Paul Derscheid)
- Much cleaner and maintainable code

Test plan:
1. Run tests:
   prove t/db_dependent/Koha/Database.t
   prove t/db_dependent/Koha/Plugins/Loader.t
   prove t/db_dependent/Koha/Plugins/Plugins.t
2. All tests should pass
3. Install and enable a plugin via the UI
4. Verify plugin hooks are called correctly
5. Verify no regressions in plugin functionality

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 23 Martin Renvoize (ashimema) 2025-11-17 16:38:58 UTC
OK.. So this bit me hard today in edi_cron.pl.

It gave me the impetus to look carefully at all the QA comments here and come up with my own follow-up that hopefully addresses them all.
Comment 24 Martin Renvoize (ashimema) 2025-11-17 16:41:02 UTC
Created attachment 189640 [details] [review]
Bug 38384: Refactor plugin loading to use lightweight Koha::Plugins::Loader

This patch addresses the circular dependency issues and code quality
concerns identified in QA by introducing a clean architectural solution.

Changes:

1. Created Koha::Plugins::Loader - A lightweight plugin loader with
   minimal dependencies that can be safely called during early
   initialization. Uses DBIx::Class when available for proper
   transaction support, falls back to raw DBI for early init.

2. Added Koha::Database::table_exists() - Consolidated table checking
   logic into a reusable method that can be called as both class and
   instance method.

3. Simplified C4::Context BEGIN block - Reduced from ~120 lines to
   ~13 lines (91% reduction). Removed XML::Simple dependency and
   manual database connection code.

4. Updated Koha::Plugins::get_enabled_plugins() - Now delegates to
   the Loader module for consistency.

5. Updated C4::Installer::TableExists - Delegates to the new
   Koha::Database::table_exists method.

This addresses all QA concerns:
- Eliminates XML::Simple usage (David Cook)
- Uses Koha::Database instead of manual DBI (David Cook)
- Reuses table checking logic (Nick Clemens)
- Eliminates circular dependencies (Paul Derscheid)
- Much cleaner and maintainable code

Test plan:
1. Run tests:
   prove t/db_dependent/Koha/Database.t
   prove t/db_dependent/Koha/Plugins/Loader.t
   prove t/db_dependent/Koha/Plugins/Plugins.t
2. All tests should pass
3. Install and enable a plugin via the UI
4. Verify plugin hooks are called correctly
5. Verify no regressions in plugin functionality

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 25 Marcel de Rooy 2025-11-28 08:18:10 UTC
Looking here
Comment 26 Marcel de Rooy 2025-11-28 09:29:05 UTC
Looks good to me. But reading the referred 36736 I would appreciate any proof of testing with a plugin that adds schema files from a plugin directory like Koha::Plugin::FancyPlugin seems to do.

Since the last patch rearranges quite a lot, another signoff doing so would be nice.
Comment 27 Marcel de Rooy 2025-11-28 09:31:50 UTC
Wondering if we really break current transactions btw, or do we create connections that do not include all plugin table schemas and fail for that reason?
Comment 28 Paul Derscheid 2025-11-28 09:41:40 UTC
> Wondering if we really break current transactions btw, or do we create connections that do not include all plugin table schemas and fail for that reason?

I can say from what we observed that this breaks unrelated transactions.