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> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit is contained in:
Martin Renvoize
Lucas Gass
parent
6dd32161af
commit
e091b0d4bc
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
@ -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' );
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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
|
||||
};
|
||||
Loading…
Reference in New Issue