[Bug 38384] New: General fix for plugins breaking database transactions
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Bug ID: 38384 Summary: General fix for plugins breaking database transactions Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: philip.orr@lmscloud.de QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.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=38384 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |major --- Comment #1 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- 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=38384 --- Comment #2 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 174150 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174150&action=edit Bug 38384: Trigger plugin modules loading as soon as possible in Koha -- 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=38384 --- Comment #3 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- I think this may solve the issue. I don't have a specific test plan to move this forward though. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |paul.derscheid@lmscloud.de --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Is this ready for testing, Kyle? And thanks! -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #5 from Paul Derscheid <paul.derscheid@lmscloud.de> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #6 from Paul Derscheid <paul.derscheid@lmscloud.de> --- 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #7 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 174264 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174264&action=edit koha-plugin-fancyplugin from KohaAdvent -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #8 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 174265 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174265&action=edit Test case (script) to reproduce the breaking database transaction -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA -- 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=38384 --- Comment #9 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Using Koha::Cache::Memory::Lite in C4::Context's BEGIN should be find, though. -- 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=38384 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- 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=38384 --- Comment #10 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 174280 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174280&action=edit Bug 38384: Inline plugin loading into C4::Context's BEGIN block -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #11 from Paul Derscheid <paul.derscheid@lmscloud.de> --- 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? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #12 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 174282 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174282&action=edit Bug 38384: Prevent initialization failure by checking for existence of plugin_data first -- 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=38384 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Assignee|koha-bugs@lists.koha-commun |paul.derscheid@lmscloud.de |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |dcook@prosentient.com.au --- Comment #13 from David Cook <dcook@prosentient.com.au> --- 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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #14 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #15 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #16 from David Cook <dcook@prosentient.com.au> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #17 from Philip Orr <philip.orr@lmscloud.de> --- @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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #18 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (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; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |36736 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36736 [Bug 36736] Add ability to load DBIx::Class Schema files found in plugins -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #174150|0 |1 is obsolete| | --- Comment #19 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 189636 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189636&action=edit Bug 38384: Trigger plugin modules loading as soon as possible in Koha 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=38384 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #174280|0 |1 is obsolete| | --- Comment #20 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 189637 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189637&action=edit Bug 38384: Inline plugin loading into C4::Context's BEGIN block 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=38384 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #174282|0 |1 is obsolete| | --- Comment #21 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 189638 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189638&action=edit Bug 38384: Prevent initialization failure by checking for existence of plugin_data first 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=38384 --- Comment #22 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 189639 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189639&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #23 from Martin Renvoize (ashimema) <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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|major |blocker -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #189639|0 |1 is obsolete| | --- Comment #24 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 189640 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189640&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|blocker |critical -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Looking here -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Needs Signoff --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #27 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #28 from Paul Derscheid <paul.derscheid@lmscloud.de> ---
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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #29 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #189636|0 |1 is obsolete| | --- Comment #30 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 190514 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=190514&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #189637|0 |1 is obsolete| | --- Comment #31 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 190515 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=190515&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #189638|0 |1 is obsolete| | --- Comment #32 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 190516 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=190516&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #189640|0 |1 is obsolete| | --- Comment #33 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 190517 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=190517&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=41603 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #34 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- I haven't read the code, but I confirm this patches DON'T FIX something similar I found and reported on bug 41603. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #35 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #36 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=41684 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #37 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 191860 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=191860&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #38 from Paul Derscheid <paul.derscheid@lmscloud.de> --- That's even better. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lisette@bywatersolutions.co | |m -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #39 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- (In reply to Nick Clemens (kidclamp) from comment #37)
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
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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #40 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 192119 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192119&action=edit Bug 38384: (follow-up) move require Koha::Plugins to use remaning ocurrences in test files seem okay, this covers the others -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Chip Halvorsen <Chip.Halvorsen@WestlakeLibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |Chip.Halvorsen@WestlakeLibr | |ary.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #190514|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=38384 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #190515|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=38384 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #190516|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=38384 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #190517|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=38384 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #191860|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=38384 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192119|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=38384 --- Comment #41 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 192628 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192628&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #42 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 192629 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192629&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #43 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 192630 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192630&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #44 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 192631 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192631&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #45 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 192632 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192632&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #46 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 192633 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192633&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192628|0 |1 is obsolete| | --- Comment #47 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 192662 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192662&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192629|0 |1 is obsolete| | --- Comment #48 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 192663 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192663&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192630|0 |1 is obsolete| | --- Comment #49 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 192664 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192664&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192631|0 |1 is obsolete| | --- Comment #50 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 192665 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192665&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192632|0 |1 is obsolete| | --- Comment #51 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 192666 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192666&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192633|0 |1 is obsolete| | --- Comment #52 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 192667 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192667&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #53 from Lisette Scheer <lisette@bywatersolutions.com> --- Marcel, are you available for QA for this? We're having a couple of plugins impacted by this. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED --- Comment #54 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Looking here now -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #55 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #56 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 193142 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193142&action=edit Bug 38384: (QA follow-up) Restore verbose option -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #57 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 193143 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193143&action=edit Bug 38384: (follow-up) Improve tests for caching -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #58 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 193144 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193144&action=edit Bug 38384: (follow-up) use Koha::Database in C4::Context -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192662|0 |1 is obsolete| | --- Comment #59 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 193171 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193171&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192663|0 |1 is obsolete| | --- Comment #60 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 193172 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193172&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192664|0 |1 is obsolete| | --- Comment #61 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 193173 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193173&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192665|0 |1 is obsolete| | --- Comment #62 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 193174 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193174&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192666|0 |1 is obsolete| | --- Comment #63 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 193175 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193175&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #192667|0 |1 is obsolete| | --- Comment #64 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 193176 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193176&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193142|0 |1 is obsolete| | --- Comment #65 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 193177 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193177&action=edit Bug 38384: (QA follow-up) Restore verbose option Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193143|0 |1 is obsolete| | --- Comment #66 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 193178 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193178&action=edit Bug 38384: (follow-up) Improve tests for caching Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193144|0 |1 is obsolete| | --- Comment #67 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 193179 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193179&action=edit Bug 38384: (follow-up) use Koha::Database in C4::Context Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |26.05.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #68 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 26.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #69 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Awesome that this is fixed! Thanks for all the work. Any chance we can backport this to 25.11.x? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_25_11_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=41890 CC| |jonathan.druart@gmail.com --- Comment #70 from Jonathan Druart <jonathan.druart@gmail.com> --- Last patches are related to bug 41890. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |39522 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39522 [Bug 39522] Add hooks to allow 'Valuebuilder' plugins to be installable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Jacob O'Mara <jacob.omara@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|26.05.00 |26.05.00,25.11.04 released in| | Status|Pushed to main |Pushed to stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #71 from Jacob O'Mara <jacob.omara@openfifth.co.uk> --- Thanks all, pushed to 25.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42374 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 [Bug 42374] DB Upgrade from the UI is broken -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 --- Comment #72 from David Cook <dcook@prosentient.com.au> --- (In reply to Kyle M Hall (khall) from comment #39)
(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!
Famous last words, eh? Looks like it broke the web installer as per bug 42374 I don't think updatedatabase.pl needs "use Koha::Plugins" since I think it's only used for 1 old DB update. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_25_11_candidate | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Needs documenting CC| |wainuiwitikapark@catalyst.n | |et.nz --- Comment #73 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Dependency not in 25.05 - not backporting -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org