Bug 38384 - General fix for plugins breaking database transactions
Summary: General fix for plugins breaking database transactions
Status: Pushed to main
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: rel_25_11_candidate
Depends on: 36736
Blocks:
  Show dependency treegraph
 
Reported: 2024-11-07 09:56 UTC by Philip Orr
Modified: 2026-02-25 16:49 UTC (History)
11 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:
26.05.00
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
Bug 38384: Trigger plugin modules loading as soon as possible in Koha (1.77 KB, patch)
2025-12-15 16:05 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38384: Inline plugin loading into C4::Context's BEGIN block (4.91 KB, patch)
2025-12-15 16:05 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38384: Prevent initialization failure by checking for existence of plugin_data first (5.46 KB, patch)
2025-12-15 16:05 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38384: Refactor plugin loading to use lightweight Koha::Plugins::Loader (20.45 KB, patch)
2025-12-15 16:05 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38384: (follow-up) Move loading into Koha::Plugins BEGIN block (1.93 KB, patch)
2026-01-22 13:53 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 38384: (follow-up) move require Koha::Plugins to use (2.26 KB, patch)
2026-01-28 14:35 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 38384: Trigger plugin modules loading as soon as possible in Koha (1.87 KB, patch)
2026-02-06 15:21 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38384: Inline plugin loading into C4::Context's BEGIN block (5.01 KB, patch)
2026-02-06 15:21 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38384: Prevent initialization failure by checking for existence of plugin_data first (5.56 KB, patch)
2026-02-06 15:21 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38384: Refactor plugin loading to use lightweight Koha::Plugins::Loader (20.58 KB, patch)
2026-02-06 15:21 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38384: (follow-up) Move loading into Koha::Plugins BEGIN block (2.09 KB, patch)
2026-02-06 15:21 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38384: (follow-up) move require Koha::Plugins to use (2.41 KB, patch)
2026-02-06 15:22 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38384: Trigger plugin modules loading as soon as possible in Koha (1.92 KB, patch)
2026-02-06 20:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 38384: Inline plugin loading into C4::Context's BEGIN block (5.06 KB, patch)
2026-02-06 20:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 38384: Prevent initialization failure by checking for existence of plugin_data first (5.61 KB, patch)
2026-02-06 20:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 38384: Refactor plugin loading to use lightweight Koha::Plugins::Loader (20.64 KB, patch)
2026-02-06 20:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 38384: (follow-up) Move loading into Koha::Plugins BEGIN block (2.14 KB, patch)
2026-02-06 20:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 38384: (follow-up) move require Koha::Plugins to use (2.46 KB, patch)
2026-02-06 20:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 38384: (QA follow-up) Restore verbose option (1.81 KB, patch)
2026-02-13 17:40 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 38384: (follow-up) Improve tests for caching (3.09 KB, patch)
2026-02-13 17:40 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 38384: (follow-up) use Koha::Database in C4::Context (670 bytes, patch)
2026-02-13 17:40 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 38384: Trigger plugin modules loading as soon as possible in Koha (2.01 KB, patch)
2026-02-16 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38384: Inline plugin loading into C4::Context's BEGIN block (5.16 KB, patch)
2026-02-16 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38384: Prevent initialization failure by checking for existence of plugin_data first (5.71 KB, patch)
2026-02-16 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38384: Refactor plugin loading to use lightweight Koha::Plugins::Loader (20.72 KB, patch)
2026-02-16 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38384: (follow-up) Move loading into Koha::Plugins BEGIN block (2.23 KB, patch)
2026-02-16 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38384: (follow-up) move require Koha::Plugins to use (2.55 KB, patch)
2026-02-16 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38384: (QA follow-up) Restore verbose option (1.90 KB, patch)
2026-02-16 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38384: (follow-up) Improve tests for caching (3.19 KB, patch)
2026-02-16 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38384: (follow-up) use Koha::Database in C4::Context (765 bytes, patch)
2026-02-16 09:57 UTC, Marcel de Rooy
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.
Comment 29 Martin Renvoize (ashimema) 2025-12-15 11:31:58 UTC
It does indeed break unrelated transactions.  By killing the connection on first plugin load we nuke whatever transaction may be in progress a d as such a rollback happens.
Comment 30 Kyle M Hall (khall) 2025-12-15 16:05:11 UTC
Created attachment 190514 [details] [review]
Bug 38384: Trigger plugin modules loading as soon as possible in Koha

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: John Doe <you@example.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Kyle M Hall (khall) 2025-12-15 16:05:19 UTC
Created attachment 190515 [details] [review]
Bug 38384: Inline plugin loading into C4::Context's BEGIN block

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: John Doe <you@example.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Kyle M Hall (khall) 2025-12-15 16:05:21 UTC
Created attachment 190516 [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>
Signed-off-by: John Doe <you@example.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Kyle M Hall (khall) 2025-12-15 16:05:24 UTC
Created attachment 190517 [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>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Tomás Cohen Arazi (tcohen) 2026-01-14 16:46:43 UTC
I haven't read the code, but I confirm this patches DON'T FIX something similar I found and reported on bug 41603.
Comment 35 Nick Clemens (kidclamp) 2026-01-21 20:24:25 UTC
This looks good, however, if I install the fancy plugin and run the test script I get "No biblio"

It is there after the transaction.

When I disable fancy plugin, then I get the biblio - so I think we haven't solved the issue
Comment 36 Nick Clemens (kidclamp) 2026-01-21 20:59:59 UTC
Doing some debug - it seems the first pass through we don't successfully cache the plugins because they cannot be loaded, because they can't be found

I tried add the addition of pluginsdir to @INC into Koha::Plugins::Loader, but then I get failures when the plugins try to call C4::Context->config - but we haven't finished loading C4::Context yet

That's as far as I could get, I don't know how to resolve that
Comment 37 Nick Clemens (kidclamp) 2026-01-22 13:53:56 UTC
Created attachment 191860 [details] [review]
Bug 38384: (follow-up) Move loading into Koha::Plugins BEGIN block

Thinking about this today, we really just need the plugins initialized anytime we are using Koha::Plugins

Moving the loading into the BEGIN there seems to resolve the issue
Comment 38 Paul Derscheid 2026-01-22 14:22:53 UTC
That's even better.
Comment 39 Kyle M Hall (khall) 2026-01-26 13:00:03 UTC
(In reply to Nick Clemens (kidclamp) from comment #37)
> Created attachment 191860 [details] [review] [review]
> Bug 38384: (follow-up) Move loading into Koha::Plugins BEGIN block
> 
> Thinking about this today, we really just need the plugins initialized
> anytime we are using Koha::Plugins
> 
> Moving the loading into the BEGIN there seems to resolve the issue

I think in addition, we should update all "require Koha::Plugins" to "use Koha::Plugins" otherwise the BEGIN block is evaluated at runtime, right? It shouldn't hurt anything at least, and we only have a handful of instances!
Comment 40 Nick Clemens (kidclamp) 2026-01-28 14:35:30 UTC
Created attachment 192119 [details] [review]
Bug 38384: (follow-up) move require Koha::Plugins to use

remaning ocurrences in test files seem okay, this covers the others
Comment 41 Tomás Cohen Arazi (tcohen) 2026-02-06 15:21:49 UTC
Created attachment 192628 [details] [review]
Bug 38384: Trigger plugin modules loading as soon as possible in Koha

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 42 Tomás Cohen Arazi (tcohen) 2026-02-06 15:21:51 UTC
Created attachment 192629 [details] [review]
Bug 38384: Inline plugin loading into C4::Context's BEGIN block

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 43 Tomás Cohen Arazi (tcohen) 2026-02-06 15:21:53 UTC
Created attachment 192630 [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>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 44 Tomás Cohen Arazi (tcohen) 2026-02-06 15:21:56 UTC
Created attachment 192631 [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>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 45 Tomás Cohen Arazi (tcohen) 2026-02-06 15:21:58 UTC
Created attachment 192632 [details] [review]
Bug 38384: (follow-up) Move loading into Koha::Plugins BEGIN block

Thinking about this today, we really just need the plugins initialized anytime we are using Koha::Plugins

Moving the loading into the BEGIN there seems to resolve the issue

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 46 Tomás Cohen Arazi (tcohen) 2026-02-06 15:22:00 UTC
Created attachment 192633 [details] [review]
Bug 38384: (follow-up) move require Koha::Plugins to use

remaning ocurrences in test files seem okay, this covers the others

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 47 Andrew Fuerste-Henry 2026-02-06 20:58:41 UTC
Created attachment 192662 [details] [review]
Bug 38384: Trigger plugin modules loading as soon as possible in Koha

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 48 Andrew Fuerste-Henry 2026-02-06 20:58:45 UTC
Created attachment 192663 [details] [review]
Bug 38384: Inline plugin loading into C4::Context's BEGIN block

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 49 Andrew Fuerste-Henry 2026-02-06 20:58:48 UTC
Created attachment 192664 [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>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 50 Andrew Fuerste-Henry 2026-02-06 20:58:51 UTC
Created attachment 192665 [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>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 51 Andrew Fuerste-Henry 2026-02-06 20:58:55 UTC
Created attachment 192666 [details] [review]
Bug 38384: (follow-up) Move loading into Koha::Plugins BEGIN block

Thinking about this today, we really just need the plugins initialized anytime we are using Koha::Plugins

Moving the loading into the BEGIN there seems to resolve the issue

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 52 Andrew Fuerste-Henry 2026-02-06 20:58:58 UTC
Created attachment 192667 [details] [review]
Bug 38384: (follow-up) move require Koha::Plugins to use

remaning ocurrences in test files seem okay, this covers the others

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 53 Lisette Scheer 2026-02-12 16:09:28 UTC
Marcel, are you available for QA for this? We're having a couple of plugins impacted by this.
Comment 54 Marcel de Rooy 2026-02-13 08:07:32 UTC
Looking here now
Comment 55 Marcel de Rooy 2026-02-13 10:37:42 UTC
Looks good to me. Just a few minor questions:

-        my $verbose = $params->{verbose} // $class->_verbose;
Removed. Verbose test still passes. So seems to be OK.

+    my $cache_key = 'enabled_plugins';
Constant?

+    # Test caching behavior
+    my @plugins_cached = Koha::Plugins::Loader->get_enabled_plugins();
+    is( scalar @plugins_cached, 0, 'Cached empty result works correctly' );
When paranoid, you could still doubt if the cache was really used?
And what about this line: Koha::Cache::Memory::Lite->set_in_cache( $cache_key, \@plugins ) if @plugins;
This shows that an empty result is not even cached ! So this test is misleading..
Should we remove the if @plugins or adjust the test?

t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t ... 1/7     # Looks like you planned 6 tests but ran 4.
#   Failed test 'after_biblio_action() and after_item_action() hooks tests'
#   at t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t line 92.
[Foreign key constraint broken on deleting resource]# Looks like your test exited with 255 just after 5.
t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t ... Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 3/7 subtests
=> Fails for me too without this patch set.. So ignoring.

Although it seems bit out of scope, C4::Context was initially modified here. It does call Koha::Database but it does not include a use or require of that module. That looks wrong. (It comes with Koha::Config::Sysprefs -> Koha::Objects.)
Should we formally add it here too? And perhaps reorder the modules?

Changing status for feedback.
Comment 56 Nick Clemens (kidclamp) 2026-02-13 17:40:14 UTC
Created attachment 193142 [details] [review]
Bug 38384: (QA follow-up) Restore verbose option
Comment 57 Nick Clemens (kidclamp) 2026-02-13 17:40:17 UTC
Created attachment 193143 [details] [review]
Bug 38384: (follow-up) Improve tests for caching
Comment 58 Nick Clemens (kidclamp) 2026-02-13 17:40:21 UTC
Created attachment 193144 [details] [review]
Bug 38384: (follow-up) use Koha::Database in C4::Context
Comment 59 Marcel de Rooy 2026-02-16 09:57:06 UTC
Created attachment 193171 [details] [review]
Bug 38384: Trigger plugin modules loading as soon as possible in Koha

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 60 Marcel de Rooy 2026-02-16 09:57:09 UTC
Created attachment 193172 [details] [review]
Bug 38384: Inline plugin loading into C4::Context's BEGIN block

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 61 Marcel de Rooy 2026-02-16 09:57:12 UTC
Created attachment 193173 [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>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 62 Marcel de Rooy 2026-02-16 09:57:16 UTC
Created attachment 193174 [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>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 63 Marcel de Rooy 2026-02-16 09:57:20 UTC
Created attachment 193175 [details] [review]
Bug 38384: (follow-up) Move loading into Koha::Plugins BEGIN block

Thinking about this today, we really just need the plugins initialized anytime we are using Koha::Plugins

Moving the loading into the BEGIN there seems to resolve the issue

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 64 Marcel de Rooy 2026-02-16 09:57:23 UTC
Created attachment 193176 [details] [review]
Bug 38384: (follow-up) move require Koha::Plugins to use

remaning ocurrences in test files seem okay, this covers the others

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 65 Marcel de Rooy 2026-02-16 09:57:26 UTC
Created attachment 193177 [details] [review]
Bug 38384: (QA follow-up) Restore verbose option

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 66 Marcel de Rooy 2026-02-16 09:57:29 UTC
Created attachment 193178 [details] [review]
Bug 38384: (follow-up) Improve tests for caching

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 67 Marcel de Rooy 2026-02-16 09:57:32 UTC
Created attachment 193179 [details] [review]
Bug 38384: (follow-up) use Koha::Database in C4::Context

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 68 Lucas Gass (lukeg) 2026-02-25 14:38:49 UTC
Nice work everyone!

Pushed to main for 26.05
Comment 69 Paul Derscheid 2026-02-25 14:40:11 UTC
Awesome that this is fixed! Thanks for all the work.

Any chance we can backport this to 25.11.x?