see discussion to 28026
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
Created attachment 122574 [details] [review] use Module-Pluggable as Plugin infrastructure
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.
Your test plan should be more comprehensive: 1. apt-get install libhook-lexwrap-perl 2. perl t/patron-object-hook.t
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.
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...
(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.)
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?
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.
(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.