Bug 28655 - new ideas/proposal to enhance pluggability
Summary: new ideas/proposal to enhance pluggability
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-02 11:52 UTC by Mark Hofstetter
Modified: 2021-07-05 03:40 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
see 28026 (2.79 KB, patch)
2021-07-02 11:54 UTC, Mark Hofstetter
Details | Diff | Splinter Review
use Module-Pluggable as Plugin infrastructure (2.20 KB, patch)
2021-07-02 19:22 UTC, Mark Hofstetter
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hofstetter 2021-07-02 11:52:38 UTC
see discussion to 28026
Comment 1 Mark Hofstetter 2021-07-02 11:54:55 UTC
Created attachment 122542 [details] [review]
see 28026

THIS IS NOT A BUGFIX JUST A PROVE OF CONCEPT

just run

t/patron-object-hook.t

and see what happens

to make this work properly something like

https://metacpan.org/pod/Module::Pluggable

should be used to find & include the plugins

Signed-off-by: Mark Hofstetter <mark@hofstetter.at>

https://bugs.koha-community.org/show_bug.cgi?id=28026
Comment 2 Mark Hofstetter 2021-07-02 19:22:21 UTC
Created attachment 122574 [details] [review]
use Module-Pluggable as Plugin infrastructure
Comment 3 David Cook 2021-07-05 03:02:59 UTC
I was just looking at Hook::LexWrap, and it looks like it hasn't been updated since 2017 and the two most recent updates before that were in 2014 and 2010. I suppose code like this wouldn't require many changes, but I wonder how well it is maintained. I also like how the perldoc says "There are undoubtedly serious bugs lurking somewhere in code this funky :-)".

That said, it does look like Debian packages it: https://packages.debian.org/buster/libhook-lexwrap-perl

I have some other concerns about the approach here, but I'm going to check them in a few minutes.
Comment 4 David Cook 2021-07-05 03:14:56 UTC
Your test plan should be more comprehensive:

1. apt-get install libhook-lexwrap-perl
2. perl t/patron-object-hook.t
Comment 5 David Cook 2021-07-05 03:32:51 UTC
Comment on attachment 122574 [details] [review]
use Module-Pluggable as Plugin infrastructure

Review of attachment 122574 [details] [review]:
-----------------------------------------------------------------

::: Koha/Object.pm
@@ +110,5 @@
> +    my $ok = Koha::ObjectHooks->new();
> +    my @plugins = $ok->plugins();
> +    foreach my $plugin (@plugins) {
> +        $plugin->run();
> +    }

Running this code in _new_from_dbic() isn't scalable. If you fetched 1000 biblios, you'd be running this code 1000 times (at least). 

Since you're looking to wrap subs, it would be more performant to do this at compile time ran than run time (although that would mean it would be less dynamic but dynamic plugins are problematic anyway in a persistent (eg Plack) context). 

--

Also, this code won't work in all contexts. If Koha::Patron hasn't been loaded into memory, you'll get the following fatal error: "Can't wrap non-existent subroutine Koha::Patron::fixup_cardnumber".

It works in your example, but it has a race condition which would break in unexpected ways. 

That said, the way around that would be to make your plugins specific to the Koha::Object you're trying to use, so that when you load Koha::Patron, you're just fetching plugins for 'Koha::Patron' so you can always count on it being loaded. But probably solvable.
Comment 6 David Cook 2021-07-05 03:39:08 UTC
I admit that I'm intrigued by the overall premise though. I like the idea of being able to upload plugins which can implement hooks that haven't previously been defined.

That said... there could be some security implications. I mean... the whole plugin infrastructure at the moment is a security problem. But allowing a plugin to arbitrarily wrap functions could let a plugin completely take over Koha in a rather insidious way which could be difficult to detect.

Of course, now I'm dreaming up potential exploits, which takes me back to bug 24632 to build a trust system so that the likelihood of running malicious code is reduced...
Comment 7 David Cook 2021-07-05 03:40:23 UTC
(Apologies if I sound too critical. I'm not intending to be discouraging. I'm trying to provide helpful feedback. I would like to see more on this topic.)