Bug 28655

Summary: New ideas/proposal to enhance pluggability
Product: Koha Reporter: Mark Hofstetter <koha>
Component: Plugin architectureAssignee: Mark Hofstetter <koha>
Status: ASSIGNED --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: dcook, domm, fridolin.somers, jacob.omara, jonathan.druart, julian.maurice, kyle, martin.renvoize, matt.blenkinsop, pedro.amorim, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34713
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28173
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: see 28026
use Module-Pluggable as Plugin infrastructure

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.)
Comment 8 Pedro Amorim 2023-09-26 10:58:46 UTC
I'm very interested in this and would love to see Koha moving in this direction.
I don't like that every new niche little requirement needs to be pushed to core.
Mark, you were working on this during last HackFest 2023?
Do you have updates on this?
Comment 9 Martin Renvoize 2023-09-26 18:57:05 UTC
I not opposed to this approach so long as we can resolve the issues already noted by David.

I would really like to see more formal trust process for security with a more centralised app store style approach.. I see that as a prerequisite for this level of exposure/power in the plugin system.

I also feel we'd need to massively improve our code documentation.. right now we make no promises to keep subroutine signatures or names etc as they are.. in fact with all the refactoring and modernisation work going on we're changing things under the hood a lot.  Opening up every method to being wrapped opens up a new can if worms and requirements for core to become significantly more stable than I think it currently is.
Comment 10 Kyle M Hall 2024-04-11 10:42:49 UTC
(In reply to Martin Renvoize from comment #9)
> I not opposed to this approach so long as we can resolve the issues already
> noted by David.
> 
> I would really like to see more formal trust process for security with a
> more centralised app store style approach.. I see that as a prerequisite for
> this level of exposure/power in the plugin system.
> 
> I also feel we'd need to massively improve our code documentation.. right
> now we make no promises to keep subroutine signatures or names etc as they
> are.. in fact with all the refactoring and modernisation work going on we're
> changing things under the hood a lot.  Opening up every method to being
> wrapped opens up a new can if worms and requirements for core to become
> significantly more stable than I think it currently is.

Agreed! I can see us versioning our signatures so to make it easier to know if a plugin may no longer be compatible.