Bug 21073 - Improve plugin performance
Summary: Improve plugin performance
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords:
Depends on: 22053
Blocks: 22832 23191 22834 23168 23171 23222 23284
  Show dependency treegraph
 
Reported: 2018-07-13 12:55 UTC by Kyle M Hall
Modified: 2019-11-14 11:38 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Before this patch, whenever a plugin hook was reached in koha code we would iteratively load plugins looking for one that may support the method. This patch adds database level caching of this data so we do one database call instead of iteratively calling 'load'.
Version(s) released in:
19.11.00


Attachments
Bug 21073: Add new table, schema and classes (5.80 KB, patch)
2018-07-13 17:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: Improve plugin performance (8.57 KB, patch)
2018-07-13 17:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: Add new table, schema and classes (6.19 KB, patch)
2018-07-13 17:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: Improve plugin performance (8.57 KB, patch)
2018-07-13 17:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: Improve plugin performance (8.90 KB, patch)
2018-07-16 10:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: Add new table, schema and classes (6.26 KB, patch)
2019-04-22 14:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: Improve plugin performance (9.39 KB, patch)
2019-04-22 14:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings (23.97 KB, patch)
2019-04-22 14:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Atomic update shouldn't fail if plugins are disabled (1.90 KB, patch)
2019-05-02 14:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Restore check on template (1.08 KB, patch)
2019-05-02 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Remove unused libraries (1.72 KB, patch)
2019-05-02 15:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: Regression tests for GetPlugins (5.32 KB, patch)
2019-05-02 17:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: Restore filtering by metadata (2.21 KB, patch)
2019-05-02 17:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: Add new table, schema and classes (6.23 KB, patch)
2019-05-03 02:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: Improve plugin performance (9.47 KB, patch)
2019-05-03 02:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings (24.01 KB, patch)
2019-05-03 02:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Atomic update shouldn't fail if plugins are disabled (1.96 KB, patch)
2019-05-03 02:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Restore check on template (1.13 KB, patch)
2019-05-03 02:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Remove unused libraries (1.77 KB, patch)
2019-05-03 02:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: Regression tests for GetPlugins (5.38 KB, patch)
2019-05-03 02:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: Restore filtering by metadata (2.26 KB, patch)
2019-05-03 02:50 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: (follow-up) Rebase from master (4.53 KB, patch)
2019-05-15 15:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: Add new table, schema and classes (6.23 KB, patch)
2019-05-16 02:48 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: Improve plugin performance (10.12 KB, patch)
2019-05-16 02:48 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings (24.01 KB, patch)
2019-05-16 02:48 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Atomic update shouldn't fail if plugins are disabled (1.96 KB, patch)
2019-05-16 02:48 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Restore check on template (1.13 KB, patch)
2019-05-16 02:48 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Remove unused libraries (1.77 KB, patch)
2019-05-16 02:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: Regression tests for GetPlugins (3.45 KB, patch)
2019-05-16 02:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: Restore filtering by metadata (2.34 KB, patch)
2019-05-16 02:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: (follow-up) QA fixes after rebase (4.53 KB, patch)
2019-05-16 02:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 21073: (follow-up) Add Class::Inspector to PerlDependencies (895 bytes, patch)
2019-06-14 19:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Only public subs should be considered (2.44 KB, patch)
2019-06-14 19:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: Add new table, schema and classes (6.35 KB, patch)
2019-06-17 11:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: Improve plugin performance (10.13 KB, patch)
2019-06-17 11:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings (24.06 KB, patch)
2019-06-17 11:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Atomic update shouldn't fail if plugins are disabled (2.00 KB, patch)
2019-06-17 11:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Restore check on template (1.19 KB, patch)
2019-06-17 11:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Remove unused libraries (1.83 KB, patch)
2019-06-17 11:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: Regression tests for GetPlugins (3.50 KB, patch)
2019-06-17 11:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: Restore filtering by metadata (2.39 KB, patch)
2019-06-17 11:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (follow-up) QA fixes after rebase (4.58 KB, patch)
2019-06-17 11:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (follow-up) Add Class::Inspector to PerlDependencies (895 bytes, patch)
2019-06-17 11:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Only public subs should be considered (2.44 KB, patch)
2019-06-17 11:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Add ->is_enabled and tests (2.10 KB, patch)
2019-06-17 11:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Simplify logic (3.27 KB, patch)
2019-06-17 11:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Don't call -> twice! (884 bytes, patch)
2019-06-17 11:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Avoid unnecessary unless/else construct (1.02 KB, patch)
2019-06-17 11:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: Add new table, schema and classes (6.42 KB, patch)
2019-06-18 12:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: Add new table, schema and classes (6.42 KB, patch)
2019-06-18 12:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: Improve plugin performance (10.21 KB, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings (24.13 KB, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Atomic update shouldn't fail if plugins are disabled (2.08 KB, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Restore check on template (1.26 KB, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Remove unused libraries (1.90 KB, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: Regression tests for GetPlugins (3.57 KB, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: Restore filtering by metadata (2.46 KB, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: (follow-up) QA fixes after rebase (4.65 KB, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: (follow-up) Add Class::Inspector to PerlDependencies (967 bytes, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Only public subs should be considered (2.51 KB, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Add ->is_enabled and tests (2.17 KB, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Simplify logic (3.34 KB, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Don't call -> twice! (956 bytes, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Avoid unnecessary unless/else construct (1.09 KB, patch)
2019-06-18 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Fix typo in class name (670 bytes, patch)
2019-06-18 19:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21073: (QA follow-up) Fix plugin-related tests (3.07 KB, patch)
2019-06-19 18:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-07-13 12:55:18 UTC
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.
Comment 1 Kyle M Hall 2018-07-13 17:28:13 UTC
Created attachment 76955 [details] [review]
Bug 21073: Add new table, schema and classes
Comment 2 Kyle M Hall 2018-07-13 17:28:24 UTC
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
Comment 3 Kyle M Hall 2018-07-13 17:37:58 UTC
Created attachment 76957 [details] [review]
Bug 21073: Add new table, schema and classes
Comment 4 Kyle M Hall 2018-07-13 17:38:11 UTC
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
Comment 5 Kyle M Hall 2018-07-16 10:40:32 UTC
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
Comment 6 David Cook 2018-09-16 22:17:46 UTC
Do you have some examples of plugins we can use before and after this patch?
Comment 7 Kyle M Hall 2018-10-05 17:49:23 UTC
(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.
Comment 8 David Cook 2018-10-08 00:49:23 UTC
(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
Comment 9 Kyle M Hall 2018-10-11 10:59:31 UTC
(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!
Comment 10 M. Tompsett 2018-10-15 17:16:24 UTC
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.
Comment 11 Liz Rea 2019-04-16 18:31:50 UTC
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.
Comment 12 Kyle M Hall 2019-04-22 14:15:26 UTC
Created attachment 88379 [details] [review]
Bug 21073: Add new table, schema and classes
Comment 13 Kyle M Hall 2019-04-22 14:15:33 UTC
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
Comment 14 Kyle M Hall 2019-04-22 14:15:37 UTC
Created attachment 88381 [details] [review]
Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings
Comment 15 Tomás Cohen Arazi 2019-04-22 17:27:55 UTC
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;
Comment 16 David Cook 2019-04-23 00:14:15 UTC
(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?
Comment 17 Liz Rea 2019-04-24 17:29:51 UTC
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
Comment 18 Tomás Cohen Arazi 2019-04-24 18:50:42 UTC
(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
Comment 19 David Cook 2019-04-29 00:16:18 UTC
(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?
Comment 20 Tomás Cohen Arazi 2019-05-02 14:42:34 UTC
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>
Comment 21 Tomás Cohen Arazi 2019-05-02 15:15:27 UTC
Created attachment 89251 [details] [review]
Bug 21073: (QA follow-up) Restore check on template
Comment 22 Tomás Cohen Arazi 2019-05-02 15:16:05 UTC
Created attachment 89252 [details] [review]
Bug 21073: (QA follow-up) Remove unused libraries
Comment 23 Tomás Cohen Arazi 2019-05-02 17:14:09 UTC
Created attachment 89255 [details] [review]
Bug 21073: Regression tests for GetPlugins

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2019-05-02 17:14:13 UTC
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
Comment 25 Agustín Moyano 2019-05-03 02:49:27 UTC
Created attachment 89272 [details] [review]
Bug 21073: Add new table, schema and classes

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 26 Agustín Moyano 2019-05-03 02:49:33 UTC
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>
Comment 27 Agustín Moyano 2019-05-03 02:49:38 UTC
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>
Comment 28 Agustín Moyano 2019-05-03 02:49:44 UTC
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>
Comment 29 Agustín Moyano 2019-05-03 02:49:47 UTC
Created attachment 89276 [details] [review]
Bug 21073: (QA follow-up) Restore check on template

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 30 Agustín Moyano 2019-05-03 02:49:51 UTC
Created attachment 89277 [details] [review]
Bug 21073: (QA follow-up) Remove unused libraries

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 31 Agustín Moyano 2019-05-03 02:49:56 UTC
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>
Comment 32 Agustín Moyano 2019-05-03 02:50:00 UTC
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>
Comment 33 Agustín Moyano 2019-05-03 13:29:01 UTC
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.
Comment 34 Tomás Cohen Arazi 2019-05-03 19:20:12 UTC
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.
Comment 35 Tomás Cohen Arazi 2019-05-06 14:52:28 UTC
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.
Comment 36 Agustín Moyano 2019-05-15 15:49:09 UTC Comment hidden (obsolete)
Comment 37 Agustín Moyano 2019-05-16 02:48:37 UTC Comment hidden (obsolete)
Comment 38 Agustín Moyano 2019-05-16 02:48:43 UTC Comment hidden (obsolete)
Comment 39 Agustín Moyano 2019-05-16 02:48:49 UTC Comment hidden (obsolete)
Comment 40 Agustín Moyano 2019-05-16 02:48:54 UTC Comment hidden (obsolete)
Comment 41 Agustín Moyano 2019-05-16 02:48:58 UTC Comment hidden (obsolete)
Comment 42 Agustín Moyano 2019-05-16 02:49:02 UTC Comment hidden (obsolete)
Comment 43 Agustín Moyano 2019-05-16 02:49:06 UTC Comment hidden (obsolete)
Comment 44 Agustín Moyano 2019-05-16 02:49:10 UTC Comment hidden (obsolete)
Comment 45 Agustín Moyano 2019-05-16 02:49:15 UTC Comment hidden (obsolete)
Comment 46 Agustín Moyano 2019-05-16 03:17:04 UTC
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..
Comment 47 Martin Renvoize 2019-06-11 08:16:39 UTC
(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?)
Comment 48 Martin Renvoize 2019-06-11 08:23:30 UTC
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 49 Martin Renvoize 2019-06-11 08:52:26 UTC
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.
Comment 50 Tomás Cohen Arazi 2019-06-14 19:51:28 UTC
Created attachment 90630 [details] [review]
Bug 21073: (follow-up) Add Class::Inspector to PerlDependencies

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 51 Tomás Cohen Arazi 2019-06-14 19:51:33 UTC
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>
Comment 52 Tomás Cohen Arazi 2019-06-17 10:02:44 UTC
(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.
Comment 53 Tomás Cohen Arazi 2019-06-17 10:02:59 UTC
(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!
Comment 54 Tomás Cohen Arazi 2019-06-17 11:21:10 UTC
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>
Comment 55 Tomás Cohen Arazi 2019-06-17 11:21:14 UTC
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>
Comment 56 Tomás Cohen Arazi 2019-06-17 11:21:19 UTC
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>
Comment 57 Tomás Cohen Arazi 2019-06-17 11:21:23 UTC
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>
Comment 58 Tomás Cohen Arazi 2019-06-17 11:21:28 UTC
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>
Comment 59 Tomás Cohen Arazi 2019-06-17 11:21:32 UTC
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>
Comment 60 Tomás Cohen Arazi 2019-06-17 11:21:37 UTC
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>
Comment 61 Tomás Cohen Arazi 2019-06-17 11:21:41 UTC
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>
Comment 62 Tomás Cohen Arazi 2019-06-17 11:21:46 UTC
Created attachment 90658 [details] [review]
Bug 21073: (follow-up) QA fixes after rebase

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 63 Tomás Cohen Arazi 2019-06-17 11:21:50 UTC
Created attachment 90659 [details] [review]
Bug 21073: (follow-up) Add Class::Inspector to PerlDependencies

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 64 Tomás Cohen Arazi 2019-06-17 11:21:55 UTC
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>
Comment 65 Tomás Cohen Arazi 2019-06-17 11:21:59 UTC
Created attachment 90661 [details] [review]
Bug 21073: (QA follow-up) Add ->is_enabled and tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 66 Tomás Cohen Arazi 2019-06-17 11:22:04 UTC
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>
Comment 67 Tomás Cohen Arazi 2019-06-17 11:22:08 UTC
Created attachment 90663 [details] [review]
Bug 21073: (QA follow-up) Don't call -> twice!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 68 Tomás Cohen Arazi 2019-06-17 11:22:13 UTC
Created attachment 90664 [details] [review]
Bug 21073: (QA follow-up) Avoid unnecessary unless/else construct

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 69 Tomás Cohen Arazi 2019-06-17 11:25:51 UTC
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.
Comment 70 Tomás Cohen Arazi 2019-06-17 11:32:48 UTC
(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!
Comment 71 Martin Renvoize 2019-06-17 14:16:09 UTC
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 :)
Comment 72 Kyle M Hall 2019-06-18 12:42:15 UTC
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>
Comment 73 Kyle M Hall 2019-06-18 12:57:57 UTC
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>
Comment 74 Kyle M Hall 2019-06-18 12:58:06 UTC
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>
Comment 75 Kyle M Hall 2019-06-18 12:58:11 UTC
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>
Comment 76 Kyle M Hall 2019-06-18 12:58:14 UTC
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>
Comment 77 Kyle M Hall 2019-06-18 12:58:18 UTC
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>
Comment 78 Kyle M Hall 2019-06-18 12:58:22 UTC
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>
Comment 79 Kyle M Hall 2019-06-18 12:58:25 UTC
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>
Comment 80 Kyle M Hall 2019-06-18 12:58:29 UTC
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>
Comment 81 Kyle M Hall 2019-06-18 12:58:33 UTC
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>
Comment 82 Kyle M Hall 2019-06-18 12:58:36 UTC
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>
Comment 83 Kyle M Hall 2019-06-18 12:58:40 UTC
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>
Comment 84 Kyle M Hall 2019-06-18 12:58:44 UTC
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>
Comment 85 Kyle M Hall 2019-06-18 12:58:48 UTC
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>
Comment 86 Kyle M Hall 2019-06-18 12:58:51 UTC
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>
Comment 87 Kyle M Hall 2019-06-18 12:58:55 UTC
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>
Comment 88 Kyle M Hall 2019-06-18 13:04:59 UTC
(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!
Comment 89 Martin Renvoize 2019-06-18 17:06:59 UTC
Nice work!

Pushed to master for 19.11.00
Comment 90 Tomás Cohen Arazi 2019-06-18 19:38:04 UTC
Created attachment 90759 [details] [review]
Bug 21073: (QA follow-up) Fix typo in class name

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 91 Tomás Cohen Arazi 2019-06-18 19:38:54 UTC
@RM last patch is needed.
Comment 92 Martin Renvoize 2019-06-19 10:48:42 UTC
Followup pushed, thanks Tomas.. tests look happy now :)
Comment 93 Martin Renvoize 2019-06-19 12:16:41 UTC
Nice work!

Pushed to master for 19.11.00
Comment 94 Tomás Cohen Arazi 2019-06-19 18:38:39 UTC
Created attachment 90816 [details] [review]
Bug 21073: (QA follow-up) Fix plugin-related tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 95 Martin Renvoize 2019-06-20 10:32:58 UTC
Followup Pushed to Master, Thanks Tomas
Comment 96 Andrew Isherwood 2019-06-20 10:36:51 UTC
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.
Comment 97 Martin Renvoize 2019-06-20 11:49:22 UTC
Good point, I missed that.. I'll open a new bug to work that one out.
Comment 98 Kyle M Hall 2019-06-20 11:53:33 UTC
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)