|
Description
Kyle M Hall (khall)
2018-07-13 12:55:18 UTC
Created attachment 76955 [details] [review] Bug 21073: Add new table, schema and classes Created attachment 76956 [details] [review] Bug 21073: Improve plugin performance Right now, to check if a plugin is functional and what methods it exposes we load the module and test for a given method at run time. This is highly inefficient. It makes far more sense to do this at install time and store the data in the db. I believe we should store a table of methods that each plugin exposes and check that instead. Then, at install time we can test that a) the plugin can be loaded and b) add the available methods to the plugin_methods table. Test Plan: 1) Apply this patch 2) Restart all the things 3) Run updatedatabase.pl 4) Verify you can use existing plugins 5) Verify you can install new plugins Created attachment 76957 [details] [review] Bug 21073: Add new table, schema and classes Created attachment 76958 [details] [review] Bug 21073: Improve plugin performance Right now, to check if a plugin is functional and what methods it exposes we load the module and test for a given method at run time. This is highly inefficient. It makes far more sense to do this at install time and store the data in the db. I believe we should store a table of methods that each plugin exposes and check that instead. Then, at install time we can test that a) the plugin can be loaded and b) add the available methods to the plugin_methods table. Test Plan: 1) Apply this patch 2) Restart all the things 3) Run updatedatabase.pl 4) Verify you can use existing plugins 5) Verify you can install new plugins Created attachment 76973 [details] [review] Bug 21073: Improve plugin performance Right now, to check if a plugin is functional and what methods it exposes we load the module and test for a given method at run time. This is highly inefficient. It makes far more sense to do this at install time and store the data in the db. I believe we should store a table of methods that each plugin exposes and check that instead. Then, at install time we can test that a) the plugin can be loaded and b) add the available methods to the plugin_methods table. Test Plan: 1) Apply this patch 2) Restart all the things 3) Run updatedatabase.pl 4) Verify you can use existing plugins 5) Verify you can install new plugins Do you have some examples of plugins we can use before and after this patch? (In reply to David Cook from comment #6) > Do you have some examples of plugins we can use before and after this patch? Any and all plugins should be valid. The Kitchen Sink would be a good test. (In reply to Kyle M Hall from comment #7) > (In reply to David Cook from comment #6) > > Do you have some examples of plugins we can use before and after this patch? > > Any and all plugins should be valid. The Kitchen Sink would be a good test. Cool, so something like the following? 0) Install Kitchen Sink plugin 1) Apply this patch 2) Restart all the things 3) Run updatedatabase.pl 4) Verify you can use existing Kitchen Sink plugin 5) Remove Kitchen Sink plugin 6) Verify you can install new Kitchen Sink plugin (In reply to David Cook from comment #8) > (In reply to Kyle M Hall from comment #7) > > (In reply to David Cook from comment #6) > > > Do you have some examples of plugins we can use before and after this patch? > > > > Any and all plugins should be valid. The Kitchen Sink would be a good test. > > Cool, so something like the following? > > 0) Install Kitchen Sink plugin > 1) Apply this patch > 2) Restart all the things > 3) Run updatedatabase.pl > 4) Verify you can use existing Kitchen Sink plugin > 5) Remove Kitchen Sink plugin > 6) Verify you can install new Kitchen Sink plugin Yes, that plan looks great! Revised Test Plan. 0) Backup DB as desired && Install Kitchen Sink plugin 1) Apply this patch 2) Restart all the things 3) Run updatedatabase.pl (testing upgrade) 4) Verify you can use existing Kitchen Sink plugin 5) Remove Kitchen Sink plugin 6) Reset all the things (testing fresh install) 7) Verify you can install new Kitchen Sink plugin 8) Remove Kitchen Sink plugin && Restore DB if backed up. Terribly sorry to report that this no longer applies: Auto-merging t/db_dependent/Plugins.t CONFLICT (content): Merge conflict in t/db_dependent/Plugins.t Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt error: Failed to merge in the changes. Created attachment 88379 [details] [review] Bug 21073: Add new table, schema and classes Created attachment 88380 [details] [review] Bug 21073: Improve plugin performance Right now, to check if a plugin is functional and what methods it exposes we load the module and test for a given method at run time. This is highly inefficient. It makes far more sense to do this at install time and store the data in the db. I believe we should store a table of methods that each plugin exposes and check that instead. Then, at install time we can test that a) the plugin can be loaded and b) add the available methods to the plugin_methods table. Test Plan: 1) Apply this patch 2) Restart all the things 3) Run updatedatabase.pl 4) Verify you can use existing plugins 5) Verify you can install new plugins Created attachment 88381 [details] [review] Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings Proposal for discussion:
- Make it explicit by declaring what 'capabilities' the plugin has, like
our $capabilities = { tools => 1, ... };
- Rename methods => capabilities:
CREATE TABLE IF NOT EXISTS plugin_capabilities (
class varchar(255) NOT NULL,
capability varchar(255) NOT NULL,
PRIMARY KEY ( `class` (191), `capability` (191) )
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
(In reply to Tomás Cohen Arazi from comment #15) > Proposal for discussion: > - Make it explicit by declaring what 'capabilities' the plugin has, like > our $capabilities = { tools => 1, ... }; > - Rename methods => capabilities: Just for the sake of discussion, is there a different word that could be used instead of capabilities? It seems to me like a word where typos might be easily introduced? Hi, Sorry to report that the updatadatabase failed with DEV atomic update: plugin_methods.perl Atomic update generated errors: Can't call method "InstallPlugins" on an undefined value at (eval 1398) line 13. Have a look? Cheers, Liz (In reply to David Cook from comment #16) > (In reply to Tomás Cohen Arazi from comment #15) > > Proposal for discussion: > > - Make it explicit by declaring what 'capabilities' the plugin has, like > > our $capabilities = { tools => 1, ... }; > > - Rename methods => capabilities: > > Just for the sake of discussion, is there a different word that could be > used instead of capabilities? It seems to me like a word where typos might > be easily introduced? As THE native speaker, it is your responsbility to propose another word :-D (In reply to Tomás Cohen Arazi from comment #18) > As THE native speaker, it is your responsbility to propose another word :-D Hehe fair enough, although I'm not sure of the exact context. Why do you want to rename it from "methods" to something else? Is methods too low-level or overloaded in terms of meaning? Created attachment 89247 [details] [review] Bug 21073: (QA follow-up) Atomic update shouldn't fail if plugins are disabled Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89251 [details] [review] Bug 21073: (QA follow-up) Restore check on template Created attachment 89252 [details] [review] Bug 21073: (QA follow-up) Remove unused libraries Created attachment 89255 [details] [review] Bug 21073: Regression tests for GetPlugins Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89256 [details] [review] Bug 21073: Restore filtering by metadata This patch restores filtering the plugins by metadata. That got lost on rebase at some point. Regression tests are added on a prior patch. To test: - Have the 'regression tests for GetPlugins' patch applied - Run: $ kshell k$ prove t/db_dependent/Plugins.t => FAIL: Tests fail! - Apply this patch - Run: k$ prove t/db_dependent/Plugins.t => SUCCESS: Tests pass! - Sign off :-D Created attachment 89272 [details] [review] Bug 21073: Add new table, schema and classes Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89273 [details] [review] Bug 21073: Improve plugin performance Right now, to check if a plugin is functional and what methods it exposes we load the module and test for a given method at run time. This is highly inefficient. It makes far more sense to do this at install time and store the data in the db. I believe we should store a table of methods that each plugin exposes and check that instead. Then, at install time we can test that a) the plugin can be loaded and b) add the available methods to the plugin_methods table. Test Plan: 1) Apply this patch 2) Restart all the things 3) Run updatedatabase.pl 4) Verify you can use existing plugins 5) Verify you can install new plugins Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89274 [details] [review] Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89275 [details] [review] Bug 21073: (QA follow-up) Atomic update shouldn't fail if plugins are disabled Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89276 [details] [review] Bug 21073: (QA follow-up) Restore check on template Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89277 [details] [review] Bug 21073: (QA follow-up) Remove unused libraries Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89278 [details] [review] Bug 21073: Regression tests for GetPlugins Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89279 [details] [review] Bug 21073: Restore filtering by metadata This patch restores filtering the plugins by metadata. That got lost on rebase at some point. Regression tests are added on a prior patch. To test: - Have the 'regression tests for GetPlugins' patch applied - Run: $ kshell k$ prove t/db_dependent/Plugins.t => FAIL: Tests fail! - Apply this patch - Run: k$ prove t/db_dependent/Plugins.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> I set this bug to Failed QA, because even when you 'prove t/db_dependent/Plugins.t' passes, it doesn't remove test plugins from database, and later, plack complains that it cannot find missing classes. The changes I made make it only consider the plugins in t/lib/Koha/Plugin, instead of taking those and also any installed plugin into account (so tests don't depend on setup/env). But I still cannot get the rollback to be effective. Symptom: > DELETE FROM plugin_methods; k$ prove t/db_dependent/Plugins.t > SELECT * FROM plugin_methods; => FAIL: methods from the test plugins. Setting back to SO, as the problem is not introduced by this bug. Will deal with it on a separate report for rewriting all tests. Created attachment 89786 [details] [review] Bug 21073: (follow-up) Rebase from master Created attachment 89800 [details] [review] Bug 21073: Add new table, schema and classes Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89801 [details] [review] Bug 21073: Improve plugin performance Right now, to check if a plugin is functional and what methods it exposes we load the module and test for a given method at run time. This is highly inefficient. It makes far more sense to do this at install time and store the data in the db. I believe we should store a table of methods that each plugin exposes and check that instead. Then, at install time we can test that a) the plugin can be loaded and b) add the available methods to the plugin_methods table. Test Plan: 1) Apply this patch 2) Restart all the things 3) Run updatedatabase.pl 4) Verify you can use existing plugins 5) Verify you can install new plugins Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89802 [details] [review] Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89803 [details] [review] Bug 21073: (QA follow-up) Atomic update shouldn't fail if plugins are disabled Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89804 [details] [review] Bug 21073: (QA follow-up) Restore check on template Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89805 [details] [review] Bug 21073: (QA follow-up) Remove unused libraries Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89806 [details] [review] Bug 21073: Regression tests for GetPlugins Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89807 [details] [review] Bug 21073: Restore filtering by metadata This patch restores filtering the plugins by metadata. That got lost on rebase at some point. Regression tests are added on a prior patch. To test: - Have the 'regression tests for GetPlugins' patch applied - Run: $ kshell k$ prove t/db_dependent/Plugins.t => FAIL: Tests fail! - Apply this patch - Run: k$ prove t/db_dependent/Plugins.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89808 [details] [review] Bug 21073: (follow-up) QA fixes after rebase When testing with qa tool, there where two FAILED pod coverage, one was for Koha/Plugins/Method.pm, and the other for Koha/Schema/Result/PluginMethod.pm, but I believe both are false positives. In both cases the error said "POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666", even though those files where introduced by this bug, so it's kind of impossible that coverage was greater before.. (In reply to Tomás Cohen Arazi from comment #15) > Proposal for discussion: > - Make it explicit by declaring what 'capabilities' the plugin has, like > our $capabilities = { tools => 1, ... }; > - Rename methods => capabilities: > CREATE TABLE IF NOT EXISTS plugin_capabilities ( > class varchar(255) NOT NULL, > capability varchar(255) NOT NULL, > PRIMARY KEY ( `class` (191), `capability` (191) ) > ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; I sort of like the idea behind this (very much borrowed from ILL backends ;) ), but I'm happy for it to wait for another patch if we deem it's helpful. I do however note we're currently loading all class methods into the database.. do we perhaps want to restrict them to 'public' methods only (i.e. missing out _method methods) or even limit down to only 'supported' methods known by the plugin system? (I'm not sure about the latter.. we might want to allow a plugin to introspect itself for some reason in the future... perhaps?) New dependency introduced: 'Class::Inspector' but not added to our dependencies lists yet.. can we get that as a QA followup here please (Seems fine for it to be added to me as it appears to all be packaged for debian already) Comment on attachment 89801 [details] [review] Bug 21073: Improve plugin performance Review of attachment 89801 [details] [review]: ----------------------------------------------------------------- A few, hopefully minor, bits of feedback.. Failing QA whilst I await a reply. ::: Koha/Plugins.pm @@ +73,5 @@ > my $method = $params->{method}; > my $req_metadata = $params->{metadata} // {}; > > + my $dbh = C4::Context->dbh; > + my $plugin_classes = $dbh->selectcol_arrayref('SELECT DISTINCT(plugin_class) FROM plugin_methods'); Why do we mix old dbh calls with dbic calls in this new code? Also, can we not skip a series of DB calls that are found in the loop by filtering on $method if it exists before running through the loop? @@ +119,4 @@ > > my $plugin = $plugin_class->new({ enable_plugins => $self->{'enable_plugins'} }); > > + Koha::Plugins::Methods->search({ plugin_class => $plugin_class })->delete(); Can't make my mind up as to whether we should really rebuilding the whole db table with every/any install/upgrade of any plugin... ::: Koha/Plugins/Handler.pm @@ +63,5 @@ > my $params = $args->{'params'}; > > + my $has_method = Koha::Plugins::Methods->search({ plugin_class => $plugin_class, plugin_method => $plugin_method })->count(); > + if ( $has_method ) { > + load $plugin_class; Is it ever possible for 'load' to fail here and if so should we catch and warn about it?.. We seem to have effectively removed a warning from the prior code.. I'm wondering if we may ever get a case where the plugin exists in the DB but has been deleted from the filesystem. @@ +68,2 @@ > my $plugin = $plugin_class->new( { cgi => $cgi, enable_plugins => $args->{'enable_plugins'} } ); > + my @return = $plugin->$plugin_method( $params ); The above line is never referenced right? ::: plugins/plugins-upload.pl @@ +87,4 @@ > $template->param( ERRORS => [ \%errors ] ); > output_html_with_http_headers $input, $cookie, $template->output; > exit; > + } else { 'unless else' is a weird looking construct.. as we're calling 'exit' in the unless block we can just call the 'Koha::Plugins->new()->InstallPlugins' method below the block and not in it's own else block. Created attachment 90630 [details] [review] Bug 21073: (follow-up) Add Class::Inspector to PerlDependencies Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90631 [details] [review] Bug 21073: (QA follow-up) Only public subs should be considered Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Martin Renvoize from comment #47) > I do however note we're currently loading all class methods into the > database.. do we perhaps want to restrict them to 'public' methods only > (i.e. missing out _method methods) or even limit down to only 'supported' > methods known by the plugin system? (I'm not sure about the latter.. we > might want to allow a plugin to introspect itself for some reason in the > future... perhaps?) Addressed. (In reply to Martin Renvoize from comment #48) > New dependency introduced: 'Class::Inspector' but not added to our > dependencies lists yet.. can we get that as a QA followup here please (Seems > fine for it to be added to me as it appears to all be packaged for debian > already) Addressed! Created attachment 90650 [details] [review] Bug 21073: Add new table, schema and classes Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90651 [details] [review] Bug 21073: Improve plugin performance Right now, to check if a plugin is functional and what methods it exposes we load the module and test for a given method at run time. This is highly inefficient. It makes far more sense to do this at install time and store the data in the db. I believe we should store a table of methods that each plugin exposes and check that instead. Then, at install time we can test that a) the plugin can be loaded and b) add the available methods to the plugin_methods table. Test Plan: 1) Apply this patch 2) Restart all the things 3) Run updatedatabase.pl 4) Verify you can use existing plugins 5) Verify you can install new plugins Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90652 [details] [review] Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90653 [details] [review] Bug 21073: (QA follow-up) Atomic update shouldn't fail if plugins are disabled Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90654 [details] [review] Bug 21073: (QA follow-up) Restore check on template Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90655 [details] [review] Bug 21073: (QA follow-up) Remove unused libraries Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90656 [details] [review] Bug 21073: Regression tests for GetPlugins Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90657 [details] [review] Bug 21073: Restore filtering by metadata This patch restores filtering the plugins by metadata. That got lost on rebase at some point. Regression tests are added on a prior patch. To test: - Have the 'regression tests for GetPlugins' patch applied - Run: $ kshell k$ prove t/db_dependent/Plugins.t => FAIL: Tests fail! - Apply this patch - Run: k$ prove t/db_dependent/Plugins.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90658 [details] [review] Bug 21073: (follow-up) QA fixes after rebase Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90659 [details] [review] Bug 21073: (follow-up) Add Class::Inspector to PerlDependencies Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90660 [details] [review] Bug 21073: (QA follow-up) Only public subs should be considered Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90661 [details] [review] Bug 21073: (QA follow-up) Add ->is_enabled and tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90662 [details] [review] Bug 21073: (QA follow-up) Simplify logic This patch simplifies the logic inside GetPlugins so: - It uses Koha::Plugins::Methods instead of plain SQL - It doesn't do more DB calls than needed, by filtering on method in the initial query to Koha::Plugins::Methods. It also relies on the (newly introduced) ->is_enabled method in Koha::Plugins::Base, for better readability. To test: - Run the tests and notice no behaviour changes are introduced. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90663 [details] [review] Bug 21073: (QA follow-up) Don't call -> twice! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 90664 [details] [review] Bug 21073: (QA follow-up) Avoid unnecessary unless/else construct Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I'm leaving it as NSO, I would like Kyle's review on the latest changes even if they are simple. Specially as a bug is fixed in 'Don't call ->$method twice!' and I might have missed something. Once someone else add the SO stamp on this I will switch it to PQA. (In reply to Martin Renvoize from comment #49) > Comment on attachment 89801 [details] [review] [review] > Bug 21073: Improve plugin performance > > Review of attachment 89801 [details] [review] [review]: > ----------------------------------------------------------------- > > A few, hopefully minor, bits of feedback.. Failing QA whilst I await a reply. > > ::: Koha/Plugins.pm > @@ +73,5 @@ > > my $method = $params->{method}; > > my $req_metadata = $params->{metadata} // {}; > > > > + my $dbh = C4::Context->dbh; > > + my $plugin_classes = $dbh->selectcol_arrayref('SELECT DISTINCT(plugin_class) FROM plugin_methods'); > > Why do we mix old dbh calls with dbic calls in this new code? Fixed! > Also, can we not skip a series of DB calls that are found in the loop by > filtering on $method if it exists before running through the loop? Fixed! > @@ +119,4 @@ > > > > my $plugin = $plugin_class->new({ enable_plugins => $self->{'enable_plugins'} }); > > > > + Koha::Plugins::Methods->search({ plugin_class => $plugin_class })->delete(); > > Can't make my mind up as to whether we should really rebuilding the whole db > table with every/any install/upgrade of any plugin... I agree with this uncomfortable feeling,. I've added a note about why we do this in the POD. Hopefully once this is in master and we start using it, we realize there are simple ways to deal with faulty scenarios. I volunteer to work on it if we find out. > ::: Koha/Plugins/Handler.pm > @@ +63,5 @@ > > my $params = $args->{'params'}; > > > > + my $has_method = Koha::Plugins::Methods->search({ plugin_class => $plugin_class, plugin_method => $plugin_method })->count(); > > + if ( $has_method ) { > > + load $plugin_class; > > Is it ever possible for 'load' to fail here and if so should we catch and > warn about it?.. We seem to have effectively removed a warning from the > prior code.. I'm wondering if we may ever get a case where the plugin exists > in the DB but has been deleted from the filesystem. Before this patchset, we really queried the FS for plugins, and we could only find existing plugins! With this way of not doing so, we might have scenarios in which the DB contains references to (manually) deleted plugins. See above for discussion about this. I vote for this approach until things settle a bit. > @@ +68,2 @@ > > my $plugin = $plugin_class->new( { cgi => $cgi, enable_plugins => $args->{'enable_plugins'} } ); > > + my @return = $plugin->$plugin_method( $params ); > > The above line is never referenced right? It was a bug! ->$method was being called twice even! This is probably some rebasing problem. > ::: plugins/plugins-upload.pl > @@ +87,4 @@ > > $template->param( ERRORS => [ \%errors ] ); > > output_html_with_http_headers $input, $cookie, $template->output; > > exit; > > + } else { > > 'unless else' is a weird looking construct.. as we're calling 'exit' in the > unless block we can just call the 'Koha::Plugins->new()->InstallPlugins' > method below the block and not in it's own else block. Fixed! Thanks Tomas, this is looking really good now.. Hopefully Kyle will have a chance to take a quick peek and give us that second SO to go for PQA :) Created attachment 90724 [details] [review] Bug 21073: Add new table, schema and classes Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90725 [details] [review] Bug 21073: Add new table, schema and classes Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90726 [details] [review] Bug 21073: Improve plugin performance Right now, to check if a plugin is functional and what methods it exposes we load the module and test for a given method at run time. This is highly inefficient. It makes far more sense to do this at install time and store the data in the db. I believe we should store a table of methods that each plugin exposes and check that instead. Then, at install time we can test that a) the plugin can be loaded and b) add the available methods to the plugin_methods table. Test Plan: 1) Apply this patch 2) Restart all the things 3) Run updatedatabase.pl 4) Verify you can use existing plugins 5) Verify you can install new plugins Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90727 [details] [review] Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90728 [details] [review] Bug 21073: (QA follow-up) Atomic update shouldn't fail if plugins are disabled Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90729 [details] [review] Bug 21073: (QA follow-up) Restore check on template Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90730 [details] [review] Bug 21073: (QA follow-up) Remove unused libraries Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90731 [details] [review] Bug 21073: Regression tests for GetPlugins Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90732 [details] [review] Bug 21073: Restore filtering by metadata This patch restores filtering the plugins by metadata. That got lost on rebase at some point. Regression tests are added on a prior patch. To test: - Have the 'regression tests for GetPlugins' patch applied - Run: $ kshell k$ prove t/db_dependent/Plugins.t => FAIL: Tests fail! - Apply this patch - Run: k$ prove t/db_dependent/Plugins.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90733 [details] [review] Bug 21073: (follow-up) QA fixes after rebase Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90734 [details] [review] Bug 21073: (follow-up) Add Class::Inspector to PerlDependencies Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90735 [details] [review] Bug 21073: (QA follow-up) Only public subs should be considered Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90736 [details] [review] Bug 21073: (QA follow-up) Add ->is_enabled and tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90737 [details] [review] Bug 21073: (QA follow-up) Simplify logic This patch simplifies the logic inside GetPlugins so: - It uses Koha::Plugins::Methods instead of plain SQL - It doesn't do more DB calls than needed, by filtering on method in the initial query to Koha::Plugins::Methods. It also relies on the (newly introduced) ->is_enabled method in Koha::Plugins::Base, for better readability. To test: - Run the tests and notice no behaviour changes are introduced. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90738 [details] [review] Bug 21073: (QA follow-up) Don't call -> twice! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 90739 [details] [review] Bug 21073: (QA follow-up) Avoid unnecessary unless/else construct Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> (In reply to Martin Renvoize from comment #71) > Thanks Tomas, this is looking really good now.. Hopefully Kyle will have a > chance to take a quick peek and give us that second SO to go for PQA :) Looks great! Nice work! Pushed to master for 19.11.00 Created attachment 90759 [details] [review] Bug 21073: (QA follow-up) Fix typo in class name Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> @RM last patch is needed. Followup pushed, thanks Tomas.. tests look happy now :) Nice work! Pushed to master for 19.11.00 Created attachment 90816 [details] [review] Bug 21073: (QA follow-up) Fix plugin-related tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Followup Pushed to Master, Thanks Tomas I pulled latest master today and received this bug, looks great, nice job!
I spotted something though and am not sure if it's expected behaviour or a bug.
The database update that adds the plugin_methods table and then calls Koha::Plugins::InstallPlugins to populate it. However, upon looking at the plugin_methods table, I'm seeing far more methods than are exposed by my single installed plugin (CLA Permissions Check).
Here are the methods that the plugin contains:
koha-koha@aidev:/var/lib/koha/koha/plugins/Koha/Plugin/Com/PTFSEurope$ cat CLAPermissionsCheck.pm | grep '^sub '
sub new {
sub intranet_catalog_biblio_enhancements {
sub intranet_catalog_biblio_enhancements_toolbar_button {
sub get_link {
sub clean_isbn {
sub clean_issn {
sub check_start {
sub configure {
sub install() {
sub upgrade {
sub uninstall() {
...and this is the contents of my plugin_methods table:
MariaDB [koha_demo1]> select * from plugin_methods;
+----------------------------------------------------+-----------------------------------------------------+
| plugin_class | plugin_method |
+----------------------------------------------------+-----------------------------------------------------+
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | abs_path |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | as_heavy |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | bundle_path |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | canonpath |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | catdir |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | catfile |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | check_start |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | clean_isbn |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | clean_issn |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | configure |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | curdir |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | decode_json |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | disable |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | dt_from_string |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | enable |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | except |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | export |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | export_fail |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | export_ok_tags |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | export_tags |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | export_to_level |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | file_name_is_absolute |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | get_link |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | get_metadata |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | get_plugin_http_path |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | get_qualified_table_name |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | get_template |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | go_home |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | import |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | install |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | intranet_catalog_biblio_enhancements |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | intranet_catalog_biblio_enhancements_toolbar_button |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | is_enabled |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | max |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | mbf_dir |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | mbf_exists |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | mbf_open |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | mbf_path |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | mbf_read |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | mbf_validate |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | new |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | no_upwards |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | only |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | output |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | output_html |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | output_html_with_http_headers |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | output_with_http_headers |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | path |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | plugins |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | require_version |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | retrieve_data |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | rootdir |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | search_path |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | sha256_hex |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | store_data |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | uninstall |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | updir |
| Koha::Plugin::Com::PTFSEurope::CLAPermissionsCheck | upgrade |
+----------------------------------------------------+-----------------------------------------------------+
It seems to be grabbing all public methods, even those imported from other modules (explicitly or otherwise). Which, though they could be strictly argued to be capabilities of the plugin, aren't native to it and could easily lead to confusion.
I couldn't see any discussion in this bug pertaining to this, so apologies if it's already been mentioned.
Good point, I missed that.. I'll open a new bug to work that one out. Yes, this is expected behavior and is a compromise to keep existing plugins from breaking. If we had whitelisted methods then any plugin using 'non-standard' plugin methods would have broken. The next step in plugins evolution is to allow each plugin to specify which methods it wants to declare ( on top of the 'official' ones ) and add those to the database instead. Then we can use this manual scanning method as a fallback for plugins that don't specify their available methods in the metadata. (In reply to Andrew Isherwood from comment #96) > I pulled latest master today and received this bug, looks great, nice job! > > I spotted something though and am not sure if it's expected behaviour or a > bug. > > The database update that adds the plugin_methods table and then calls > Koha::Plugins::InstallPlugins to populate it. However, upon looking at the > plugin_methods table, I'm seeing far more methods than are exposed by my > single installed plugin (CLA Permissions Check) |