[Bug 21073] New: Improve plugin performance
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Bug ID: 21073 Summary: Improve plugin performance Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 76955 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76955&action=edit Bug 21073: Add new table, schema and classes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 76956 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76956&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #76955|0 |1 is obsolete| | Attachment #76956|0 |1 is obsolete| | --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 76957 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76957&action=edit Bug 21073: Add new table, schema and classes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 76958 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76958&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #76958|0 |1 is obsolete| | --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 76973 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76973&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- Do you have some examples of plugins we can use before and after this patch? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Michal Denar <black23@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |black23@gmail.com, | |rbit@rbit.cz -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #8 from David Cook <dcook@prosentient.com.au> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #10 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |wizzyrea@gmail.com --- Comment #11 from Liz Rea <wizzyrea@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #76957|0 |1 is obsolete| | Attachment #76973|0 |1 is obsolete| | --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 88379 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=88379&action=edit Bug 21073: Add new table, schema and classes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 88380 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=88380&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 88381 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=88381&action=edit Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #16 from David Cook <dcook@prosentient.com.au> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #17 from Liz Rea <wizzyrea@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #18 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #19 from David Cook <dcook@prosentient.com.au> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |tomascohen@gmail.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #20 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 89247 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89247&action=edit Bug 21073: (QA follow-up) Atomic update shouldn't fail if plugins are disabled Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|--- |19.05 Version|unspecified |master Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #21 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 89251 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89251&action=edit Bug 21073: (QA follow-up) Restore check on template -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #22 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 89252 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89252&action=edit Bug 21073: (QA follow-up) Remove unused libraries -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |22832 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22832 [Bug 22832] Require plugins to specify the methods they implement -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #23 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 89255 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89255&action=edit Bug 21073: Regression tests for GetPlugins Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #24 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 89256 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89256&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |22834 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22834 [Bug 22834] Add a method for plugins to return the absolute path for bundled files -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |agustinmoyano@theke.io Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89251|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89252|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89255|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89256|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #88379|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #88380|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #88381|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89247|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #25 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89272 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89272&action=edit Bug 21073: Add new table, schema and classes Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #26 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89273 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89273&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #27 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89274 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89274&action=edit Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #28 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89275 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89275&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #29 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89276 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89276&action=edit Bug 21073: (QA follow-up) Restore check on template Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #30 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89277 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89277&action=edit Bug 21073: (QA follow-up) Remove unused libraries Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #31 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89278 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89278&action=edit Bug 21073: Regression tests for GetPlugins Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #32 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89279 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89279&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |arthur.suzuki@biblibre.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #33 from Agustín 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_19_05_candidate CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #34 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #35 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #36 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89786 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89786&action=edit Bug 21073: (follow-up) Rebase from master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89786|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89272|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89273|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89274|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89275|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89276|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89277|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89278|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89279|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #37 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89800 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89800&action=edit Bug 21073: Add new table, schema and classes Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #38 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89801 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89801&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #39 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89802 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89802&action=edit Bug 21073: Upgrade the kitchen sink plugin to get rid of warnings Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #40 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89803 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89803&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #41 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89804 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89804&action=edit Bug 21073: (QA follow-up) Restore check on template Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #42 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89805 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89805&action=edit Bug 21073: (QA follow-up) Remove unused libraries Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #43 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89806 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89806&action=edit Bug 21073: Regression tests for GetPlugins Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #44 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89807 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89807&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #45 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 89808 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89808&action=edit Bug 21073: (follow-up) QA fixes after rebase -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #46 from Agustín Moyano <agustinmoyano@theke.io> --- 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.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_19_05_candidate | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|tomascohen@gmail.com | CC| |katrin.fischer@bsz-bw.de -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|19.05 |--- -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #47 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (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?) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #48 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mirko@abunchofthings.net -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #49 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 89801 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89801 Bug 21073: Improve plugin performance Review of attachment 89801: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=21073&attachment=89801) ----------------------------------------------------------------- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |RM_priority -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |Before this patch, whenever release notes| |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'. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #50 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90630 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90630&action=edit Bug 21073: (follow-up) Add Class::Inspector to PerlDependencies Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #51 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90631 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90631&action=edit Bug 21073: (QA follow-up) Only public subs should be considered Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #52 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #53 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90630|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90631|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89800|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89801|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89802|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89803|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89804|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89805|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89806|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89807|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #89808|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #54 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90650 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90650&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #55 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90651 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90651&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #56 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90652 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90652&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #57 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90653 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90653&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #58 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90654 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90654&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #59 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90655 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90655&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #60 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90656 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90656&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #61 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90657 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90657&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #62 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90658 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90658&action=edit Bug 21073: (follow-up) QA fixes after rebase Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #63 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90659 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90659&action=edit Bug 21073: (follow-up) Add Class::Inspector to PerlDependencies Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #64 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90660 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90660&action=edit Bug 21073: (QA follow-up) Only public subs should be considered Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #65 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90661 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90661&action=edit Bug 21073: (QA follow-up) Add ->is_enabled and tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #66 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90662 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90662&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #67 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90663 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90663&action=edit Bug 21073: (QA follow-up) Don't call -> twice! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #68 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90664 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90664&action=edit Bug 21073: (QA follow-up) Avoid unnecessary unless/else construct Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #69 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #70 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Martin Renvoize from comment #49)
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?
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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #71 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90650|0 |1 is obsolete| | Attachment #90651|0 |1 is obsolete| | Attachment #90652|0 |1 is obsolete| | Attachment #90653|0 |1 is obsolete| | Attachment #90654|0 |1 is obsolete| | Attachment #90655|0 |1 is obsolete| | Attachment #90656|0 |1 is obsolete| | Attachment #90657|0 |1 is obsolete| | Attachment #90658|0 |1 is obsolete| | Attachment #90659|0 |1 is obsolete| | Attachment #90660|0 |1 is obsolete| | Attachment #90661|0 |1 is obsolete| | Attachment #90662|0 |1 is obsolete| | Attachment #90663|0 |1 is obsolete| | Attachment #90664|0 |1 is obsolete| | --- Comment #72 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90724 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90724&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90724|0 |1 is obsolete| | --- Comment #73 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90725 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90725&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #74 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90726 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90726&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #75 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90727 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90727&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #76 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90728 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90728&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #77 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90729 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90729&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #78 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90730 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90730&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #79 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90731 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90731&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #80 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90732 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90732&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #81 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90733 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90733&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #82 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90734 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90734&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #83 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90735 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90735&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #84 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90736 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90736&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #85 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90737 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90737&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #86 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90738 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90738&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #87 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90739 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90739&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #88 from 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master Version(s)| |19.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #89 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work! Pushed to master for 19.11.00 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #90 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90759 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90759&action=edit Bug 21073: (QA follow-up) Fix typo in class name Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|RM_priority |additional_work_needed --- Comment #91 from Tomás Cohen Arazi <tomascohen@gmail.com> --- @RM last patch is needed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #92 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Followup pushed, thanks Tomas.. tests look happy now :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #93 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work! Pushed to master for 19.11.00 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Depends on| |22053 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22053 [Bug 22053] Ability to disable some plugins -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #94 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 90816 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90816&action=edit Bug 21073: (QA follow-up) Fix plugin-related tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |23168 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23168 [Bug 23168] Ability to disable some plugins -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23168 Depends on|23168 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23168 [Bug 23168] Database Updates broken due to conflicts in bug 21073 and bug 22053 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #95 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Followup Pushed to Master, Thanks Tomas -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Andrew Isherwood <andrew.isherwood@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew.isherwood@ptfs-europ | |e.com --- Comment #96 from Andrew Isherwood <andrew.isherwood@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #97 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Good point, I missed that.. I'll open a new bug to work that one out. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23171 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23171 [Bug 23171] Improve plugin performance -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 --- Comment #98 from Kyle M Hall <kyle@bywatersolutions.com> --- 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)
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23222 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23222 [Bug 23222] Fix DISABLE/ENABLE plugin label in plugins home -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23191 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23191 [Bug 23191] Administrators should be able to install plugins from the command line -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23284 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23284 [Bug 23284] Duplicate test in t/db_dependent/Plugins.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|Architecture, internals, |Plugin architecture |and plumbing | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21073 Andrew Isherwood <bugzilla@warmlight.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|bugzilla@warmlight.co.uk | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org