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.
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.
Created attachment 174150 [details] [review] Bug 38384: Trigger plugin modules loading as soon as possible in Koha
I think this may solve the issue. I don't have a specific test plan to move this forward though.
Is this ready for testing, Kyle? And thanks!
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.
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
Created attachment 174264 [details] koha-plugin-fancyplugin from KohaAdvent
Created attachment 174265 [details] Test case (script) to reproduce the breaking database transaction
Using Koha::Cache::Memory::Lite in C4::Context's BEGIN should be find, though.
Created attachment 174280 [details] [review] Bug 38384: Inline plugin loading into C4::Context's BEGIN block
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?
Created attachment 174282 [details] [review] Bug 38384: Prevent initialization failure by checking for existence of plugin_data first
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).
(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.
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
(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?
@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
(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;
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>
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>
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>
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>
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.
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>
Looking here
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.
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?
> 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.