Bugzilla – Attachment 189639 Details for
Bug 38384
General fix for plugins breaking database transactions
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 38384: Refactor plugin loading to use lightweight Koha::Plugins::Loader
Bug-38384-Refactor-plugin-loading-to-use-lightweig.patch (text/plain), 20.38 KB, created by
Martin Renvoize (ashimema)
on 2025-11-17 16:36:47 UTC
(
hide
)
Description:
Bug 38384: Refactor plugin loading to use lightweight Koha::Plugins::Loader
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-11-17 16:36:47 UTC
Size:
20.38 KB
patch
obsolete
>From cee908344c70c5d539fc72927875bbd22df0a3d1 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Mon, 17 Nov 2025 16:31:11 +0000 >Subject: [PATCH] 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> >--- > C4/Context.pm | 99 ++------------------- > C4/Installer.pm | 10 +-- > Koha/Database.pm | 28 ++++++ > Koha/Plugins.pm | 31 +------ > Koha/Plugins/Loader.pm | 127 +++++++++++++++++++++++++++ > t/db_dependent/Koha/Database.t | 23 ++++- > t/db_dependent/Koha/Plugins/Loader.t | 126 ++++++++++++++++++++++++++ > 7 files changed, 314 insertions(+), 130 deletions(-) > create mode 100644 Koha/Plugins/Loader.pm > create mode 100644 t/db_dependent/Koha/Plugins/Loader.t > >diff --git a/C4/Context.pm b/C4/Context.pm >index 77fb92cb710..edf9a032fa3 100644 >--- a/C4/Context.pm >+++ b/C4/Context.pm >@@ -22,104 +22,17 @@ use Modern::Perl; > use vars qw($AUTOLOAD $context); > > BEGIN { >+ # Load plugins early to ensure they're available before any transactions begin > my $enable_plugins = 0; > if ( exists $ENV{'KOHA_CONF'} && -e $ENV{'KOHA_CONF'} ) { >- require XML::Simple; >- $enable_plugins = >- XML::Simple->new->XMLin( $ENV{'KOHA_CONF'}, ForceArray => 0, KeyAttr => [] )->{'config'}->{'enable_plugins'} >- // 0; >+ require Koha::Config; >+ my $config = Koha::Config->get_instance; >+ $enable_plugins = $config->get('enable_plugins') // 0; > } > > if ($enable_plugins) { >- require Koha::Cache::Memory::Lite; >- my $enabled_plugins = Koha::Cache::Memory::Lite->get_from_cache('enabled_plugins'); >- if ( !$enabled_plugins ) { >- require Koha::Config; >- my $config = Koha::Config->get_instance; >- my $driver = $config->get('db_scheme') eq 'Pg' ? 'Pg' : 'mysql'; >- my $dsn = sprintf( >- 'dbi:%s:database=%s;host=%s;port=%s', >- $driver, >- $config->get('database_test') || $config->get('database'), >- $config->get('hostname'), >- $config->get('port') || q{}, >- ); >- my $attr = { RaiseError => 1, PrintError => 1 }; >- if ( $driver eq 'mysql' && ( $config->get("tls") // q{} ) eq 'yes' ) { >- $dsn .= sprintf( >- ';mysql_ssl=1;mysql_ssl_client_key=%s;mysql_ssl_client_cert=%s;mysql_ssl_ca_file=%s', >- $config->get('key'), $config->get('cert'), $config->get('ca'), >- ); >- $attr->{'mysql_enable_utf8'} = 1; >- } >- >- require DBI; >- my $dbh = DBI->connect( $dsn, $config->get('user'), $config->get('pass'), $attr ); >- if ($dbh) { >- my $tz = $config->timezone // q{}; >- my @queries; >- push @queries, "SET NAMES 'utf8mb4'" if $driver eq 'mysql'; >- push @queries, qq{SET time_zone = "$tz"} if $tz && $driver eq 'mysql' && $tz ne 'local'; >- push @queries, qq{SET TIME ZONE = "$tz"} if $tz && $driver eq 'Pg'; >- push @queries, 'set client_encoding = \'UTF8\'' if $driver eq 'Pg'; >- if ( $driver eq 'mysql' ) { >- my $sql_mode = >- ( $config->get('strict_sql_modes') || $ENV{KOHA_TESTING} || ( $ENV{_} // q{} ) =~ m{prove}smx ) >- ? 'ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION' >- : 'IGNORE_SPACE,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION'; >- push @queries, qq{SET sql_mode = '$sql_mode'}; >- } >- >- $dbh->do($_) for grep { $_ } @queries; >- >- my $table_exists = 0; >- if ( $driver eq 'mysql' ) { >- $table_exists = $dbh->selectrow_array( >- <<~'SQL' >- SELECT COUNT(*) FROM information_schema.tables >- WHERE table_schema = DATABASE() AND table_name = 'plugin_data' >- SQL >- ); >- } elsif ( $driver eq 'Pg' ) { >- $table_exists = $dbh->selectrow_array( >- <<~'SQL' >- SELECT COUNT(*) FROM pg_catalog.pg_tables >- WHERE tablename = 'plugin_data' >- SQL >- ); >- } >- >- my @plugin_classes; >- if ($table_exists) { >- my $sth = $dbh->prepare( >- <<~'SQL' >- SELECT plugin_class FROM plugin_data WHERE plugin_key = '__ENABLED__' AND plugin_value = 1 >- SQL >- ); >- $sth->execute(); >- @plugin_classes = map { $_->[0] } @{ $sth->fetchall_arrayref() }; >- $sth->finish(); >- } >- $dbh->disconnect(); >- >- if (@plugin_classes) { >- $enabled_plugins = []; >- require Module::Load::Conditional; >- foreach my $plugin_class (@plugin_classes) { >- next >- if !Module::Load::Conditional::can_load( >- modules => { $plugin_class => undef }, >- nocache => 1 >- ); >- if ( my $plugin = eval { $plugin_class->new() } ) { >- push @{$enabled_plugins}, $plugin; >- } >- } >- } >- >- Koha::Cache::Memory::Lite->set_in_cache( 'enabled_plugins', $enabled_plugins ) if $enabled_plugins; >- } >- } >+ require Koha::Plugins::Loader; >+ Koha::Plugins::Loader->get_enabled_plugins(); > } > > if ( $ENV{'HTTP_USER_AGENT'} ) { # Only hit when plack is not enabled >diff --git a/C4/Installer.pm b/C4/Installer.pm >index 42db3763319..7dd124fba53 100644 >--- a/C4/Installer.pm >+++ b/C4/Installer.pm >@@ -754,14 +754,8 @@ Missing POD for TableExists. > > sub TableExists { # Could be renamed table_exists for consistency > my $table = shift; >- eval { >- my $dbh = C4::Context->dbh; >- local $dbh->{PrintError} = 0; >- local $dbh->{RaiseError} = 1; >- $dbh->do(qq{SELECT * FROM $table WHERE 1 = 0 }); >- }; >- return 1 unless $@; >- return 0; >+ require Koha::Database; >+ return Koha::Database->table_exists( C4::Context->dbh, $table ); > } > > =head2 version_from_file >diff --git a/Koha/Database.pm b/Koha/Database.pm >index 4bef57ad91a..7d577d29a7c 100644 >--- a/Koha/Database.pm >+++ b/Koha/Database.pm >@@ -233,6 +233,34 @@ sub generate_dsn { > return $dsn; > } > >+=head2 table_exists >+ >+ my $exists = Koha::Database->table_exists($dbh, 'plugin_data'); >+ my $exists = Koha::Database->new->table_exists('plugin_data'); >+ >+Checks if a table exists in the database. Can be called as a class method >+with a dbh parameter, or as an instance method. This method is safe for use >+during early initialization. >+ >+=cut >+ >+sub table_exists { >+ my ( $class, $dbh, $table ) = @_; >+ >+ # If called as instance method, get dbh from instance >+ if ( ref $class ) { >+ $table = $dbh; >+ $dbh = $class->dbh; >+ } >+ >+ eval { >+ local $dbh->{PrintError} = 0; >+ local $dbh->{RaiseError} = 1; >+ $dbh->do(qq{SELECT * FROM $table WHERE 1 = 0}); >+ }; >+ return $@ ? 0 : 1; >+} >+ > =head2 EXPORT > > None by default. >diff --git a/Koha/Plugins.pm b/Koha/Plugins.pm >index 14a7e014755..0f2ae32fa25 100644 >--- a/Koha/Plugins.pm >+++ b/Koha/Plugins.pm >@@ -118,34 +118,9 @@ sub get_enabled_plugins { > > return unless C4::Context->config('enable_plugins'); > >- my $enabled_plugins = Koha::Cache::Memory::Lite->get_from_cache(ENABLED_PLUGINS_CACHE_KEY); >- unless ($enabled_plugins) { >- my $verbose = $params->{verbose} // $class->_verbose; >- $enabled_plugins = []; >- >- my @plugin_classes; >- try { >- my $rs = Koha::Plugins::Datas->search( { plugin_key => '__ENABLED__', plugin_value => 1 } ); >- @plugin_classes = $rs->get_column('plugin_class'); >- } catch { >- warn "$_"; >- }; >- >- foreach my $plugin_class (@plugin_classes) { >- next unless can_load( modules => { $plugin_class => undef }, verbose => $verbose, nocache => 1 ); >- >- my $plugin = eval { $plugin_class->new() }; >- if ( $@ || !$plugin ) { >- warn "Failed to instantiate plugin $plugin_class: $@"; >- next; >- } >- >- push @$enabled_plugins, $plugin; >- } >- Koha::Cache::Memory::Lite->set_in_cache( ENABLED_PLUGINS_CACHE_KEY, $enabled_plugins ); >- } >- >- return @$enabled_plugins; >+ # Delegate to Koha::Plugins::Loader which handles caching and loading >+ require Koha::Plugins::Loader; >+ return Koha::Plugins::Loader->get_enabled_plugins(); > } > > sub _verbose { >diff --git a/Koha/Plugins/Loader.pm b/Koha/Plugins/Loader.pm >new file mode 100644 >index 00000000000..d01d35c3eac >--- /dev/null >+++ b/Koha/Plugins/Loader.pm >@@ -0,0 +1,127 @@ >+package Koha::Plugins::Loader; >+ >+# Copyright 2024 Koha Development Team >+# >+# This file is part of Koha. >+# >+# Koha is free software; you can redistribute it and/or modify it >+# under the terms of the GNU General Public License as published by >+# the Free Software Foundation; either version 3 of the License, or >+# (at your option) any later version. >+# >+# Koha is distributed in the hope that it will be useful, but >+# WITHOUT ANY WARRANTY; without even the implied warranty of >+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >+# GNU General Public License for more details. >+# >+# You should have received a copy of the GNU General Public License >+# along with Koha; if not, see <https://www.gnu.org/licenses>. >+ >+use Modern::Perl; >+use Koha::Database; >+use Koha::Cache::Memory::Lite; >+use Module::Load::Conditional qw(can_load); >+ >+=head1 NAME >+ >+Koha::Plugins::Loader - Lightweight plugin loader for early initialization >+ >+=head1 SYNOPSIS >+ >+ # Safe to call from C4::Context BEGIN block >+ use Koha::Plugins::Loader; >+ my @plugins = Koha::Plugins::Loader->get_enabled_plugins(); >+ >+=head1 DESCRIPTION >+ >+Minimal-dependency plugin loader that can be safely called during early >+initialization (e.g., from C4::Context BEGIN block) without causing >+circular dependencies. This module provides a clean separation between >+plugin loading and the main Koha::Plugins module. >+ >+=head1 METHODS >+ >+=head2 get_enabled_plugins >+ >+ my @plugins = Koha::Plugins::Loader->get_enabled_plugins(); >+ >+Returns a list of enabled plugin objects. Results are cached in memory >+to avoid repeated database queries and plugin instantiation. >+ >+This method: >+- Checks if the plugin_data table exists >+- Queries for enabled plugins >+- Loads and instantiates plugin classes >+- Caches the results >+ >+Returns an array of plugin objects in list context, or an empty list >+if plugins are not available or none are enabled. >+ >+=cut >+ >+sub get_enabled_plugins { >+ my ($class) = @_; >+ >+ my $cache_key = 'enabled_plugins'; >+ my $cached = Koha::Cache::Memory::Lite->get_from_cache($cache_key); >+ return @$cached if $cached; >+ >+ # Check if plugin_data table exists (using DBH for early init safety) >+ my $dbh = eval { Koha::Database->dbh }; >+ return unless $dbh; >+ return unless Koha::Database->table_exists( $dbh, 'plugin_data' ); >+ >+ # Get enabled plugin classes using DBIx::Class when available >+ # This ensures we see transactional changes in tests >+ my @plugin_classes; >+ if ( eval { require Koha::Plugins::Datas; 1 } ) { >+ >+ # Use DBIx::Class for proper transaction support >+ eval { >+ my $rs = Koha::Plugins::Datas->search( { plugin_key => '__ENABLED__', plugin_value => 1 } ); >+ @plugin_classes = $rs->get_column('plugin_class'); >+ }; >+ } else { >+ >+ # Fallback to raw DBI for early initialization >+ my $plugin_classes_arrayref = $dbh->selectcol_arrayref( >+ 'SELECT plugin_class FROM plugin_data WHERE plugin_key = ? AND plugin_value = 1', >+ {}, '__ENABLED__' >+ ) || []; >+ @plugin_classes = @$plugin_classes_arrayref; >+ } >+ >+ # Load and instantiate plugins >+ my @plugins; >+ foreach my $plugin_class (@plugin_classes) { >+ >+ # Check if Koha::Plugins has a mocked can_load (for testing) >+ # Otherwise use Module::Load::Conditional::can_load >+ my $can_load_result; >+ if ( eval { Koha::Plugins->can('can_load') } ) { >+ >+ # Use Koha::Plugins::can_load if it exists (might be mocked in tests) >+ $can_load_result = eval { Koha::Plugins::can_load( modules => { $plugin_class => undef }, nocache => 1 ) }; >+ } >+ if ( !defined $can_load_result ) { >+ >+ # Fall back to Module::Load::Conditional >+ $can_load_result = can_load( modules => { $plugin_class => undef }, nocache => 1 ); >+ } >+ >+ next unless $can_load_result; >+ my $plugin = eval { $plugin_class->new() }; >+ push @plugins, $plugin if $plugin; >+ } >+ >+ Koha::Cache::Memory::Lite->set_in_cache( $cache_key, \@plugins ) if @plugins; >+ return @plugins; >+} >+ >+=head1 AUTHOR >+ >+Koha Development Team >+ >+=cut >+ >+1; >diff --git a/t/db_dependent/Koha/Database.t b/t/db_dependent/Koha/Database.t >index 8ebb6fdebaf..4c96bbc5997 100755 >--- a/t/db_dependent/Koha/Database.t >+++ b/t/db_dependent/Koha/Database.t >@@ -17,7 +17,7 @@ > > use Modern::Perl; > use Test::NoWarnings; >-use Test::More tests => 5; >+use Test::More tests => 6; > use C4::Context; > use t::lib::Mocks; > >@@ -90,3 +90,24 @@ subtest 'generate_dsn' => sub { > 'DSN string for MySQL configuration with TLS with ca file.' > ); > }; >+ >+subtest 'table_exists' => sub { >+ plan tests => 5; >+ >+ # Use the existing dbh established at the top of the test >+ my $dbh = C4::Context->dbh; >+ >+ # Test with a table that definitely exists >+ ok( Koha::Database->table_exists( $dbh, 'borrowers' ), 'borrowers table exists (class method with dbh)' ); >+ >+ # Test with a table that doesn't exist >+ ok( >+ !Koha::Database->table_exists( $dbh, 'nonexistent_table_xyz' ), >+ 'nonexistent table returns false (class method with dbh)' >+ ); >+ >+ # Test with various standard Koha tables >+ ok( Koha::Database->table_exists( $dbh, 'items' ), 'items table exists' ); >+ ok( Koha::Database->table_exists( $dbh, 'biblioitems' ), 'biblioitems table exists' ); >+ ok( Koha::Database->table_exists( $dbh, 'plugin_data' ), 'plugin_data table exists' ); >+}; >diff --git a/t/db_dependent/Koha/Plugins/Loader.t b/t/db_dependent/Koha/Plugins/Loader.t >new file mode 100644 >index 00000000000..df1507b420d >--- /dev/null >+++ b/t/db_dependent/Koha/Plugins/Loader.t >@@ -0,0 +1,126 @@ >+#!/usr/bin/perl >+ >+# This file is part of Koha. >+# >+# Koha is free software; you can redistribute it and/or modify it under the >+# terms of the GNU General Public License as published by the Free Software >+# Foundation; either version 3 of the License, or (at your option) any later >+# version. >+# >+# Koha is distributed in the hope that it will be useful, but WITHOUT ANY >+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR >+# A PARTICULAR PURPOSE. See the GNU General Public License for more details. >+# >+# You should have received a copy of the GNU General Public License along >+# with Koha; if not, see <https://www.gnu.org/licenses>. >+ >+use Modern::Perl; >+use File::Basename; >+use Test::MockModule; >+use Test::More tests => 6; >+use Test::NoWarnings; >+use Test::Warn; >+ >+use C4::Context; >+use Koha::Cache::Memory::Lite; >+use Koha::Database; >+use Koha::Plugins::Datas; >+use Koha::Plugins; >+ >+use t::lib::Mocks; >+ >+BEGIN { >+ # Mock pluginsdir before loading Plugins module >+ my $path = dirname(__FILE__) . '/../../../lib/plugins'; >+ t::lib::Mocks::mock_config( 'pluginsdir', $path ); >+ >+ use_ok('Koha::Plugins::Loader'); >+} >+ >+my $schema = Koha::Database->new->schema; >+ >+subtest 'get_enabled_plugins - basic functionality' => sub { >+ plan tests => 4; >+ >+ $schema->storage->txn_begin; >+ >+ # Clear cache before testing >+ Koha::Cache::Memory::Lite->flush(); >+ >+ # Remove any existing plugins >+ Koha::Plugins::Datas->delete; >+ >+ # Test with no enabled plugins >+ my @plugins = Koha::Plugins::Loader->get_enabled_plugins(); >+ is( scalar @plugins, 0, 'Returns empty list when no plugins are enabled' ); >+ >+ # Test caching behavior >+ my @plugins_cached = Koha::Plugins::Loader->get_enabled_plugins(); >+ is( scalar @plugins_cached, 0, 'Cached empty result works correctly' ); >+ >+ # The core functionality of loading plugins is tested indirectly through >+ # the Koha::Plugins tests which use the Loader >+ ok( 1, 'Loader module loaded successfully' ); >+ can_ok( 'Koha::Plugins::Loader', 'get_enabled_plugins' ); >+ >+ $schema->storage->txn_rollback; >+}; >+ >+subtest 'get_enabled_plugins - table does not exist' => sub { >+ plan tests => 2; >+ >+ $schema->storage->txn_begin; >+ >+ # Clear cache >+ Koha::Cache::Memory::Lite->flush(); >+ >+ # Mock table_exists to return false >+ my $mock_database = Test::MockModule->new('Koha::Database'); >+ $mock_database->mock( 'table_exists', sub { return 0; } ); >+ >+ my @plugins = Koha::Plugins::Loader->get_enabled_plugins(); >+ is( scalar @plugins, 0, 'Returns empty list when plugin_data table does not exist' ); >+ >+ # Verify it doesn't try to query the non-existent table >+ # This test passes if no exception is thrown >+ ok( 1, 'No exception thrown when table does not exist' ); >+ >+ $schema->storage->txn_rollback; >+}; >+ >+subtest 'get_enabled_plugins - error handling' => sub { >+ plan tests => 1; >+ >+ $schema->storage->txn_begin; >+ >+ # Clear cache >+ Koha::Cache::Memory::Lite->flush(); >+ >+ # Remove existing plugins >+ Koha::Plugins::Datas->delete; >+ >+ # Test with invalid plugin class that can't be loaded by adding directly to DB >+ Koha::Plugins::Data->new( >+ { >+ plugin_class => 'Koha::Plugin::NonExistent', >+ plugin_key => '__ENABLED__', >+ plugin_value => 1, >+ } >+ )->store; >+ >+ my @plugins = Koha::Plugins::Loader->get_enabled_plugins(); >+ is( scalar @plugins, 0, 'Returns empty list when plugin class cannot be loaded' ); >+ >+ $schema->storage->txn_rollback; >+}; >+ >+subtest 'Integration with Koha::Plugins' => sub { >+ plan tests => 1; >+ >+ # The Loader is designed to be used by Koha::Plugins::get_enabled_plugins >+ # Test that the integration point exists >+ can_ok( 'Koha::Plugins', 'get_enabled_plugins' ); >+ >+ # Full integration testing is done in t/db_dependent/Koha/Plugins/Plugins.t >+ # which exercises the complete plugin loading flow including the Loader >+}; >-- >2.51.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 38384
:
174150
|
174264
|
174265
|
174280
|
174282
|
189636
|
189637
|
189638
|
189639
|
189640