|
Description
Martin Renvoize (ashimema)
2021-03-23 16:02:48 UTC
Created attachment 118679 [details] [review] Bug 28026: Add call_recursive() as a supliment for call() The method call() is not sufficient for all plugin hook types. It's possible that more than one plugin will be installed that wants to return the arguaments passed in an updated form. These transformation plugins need to work recursively rather than independantly. This patch adss a `call_recursive()` method that takes the output of the first plugin and uses it as the input for the next plugin and so on. This allowes each plugin to see the current version of the arguament list and modify it as necessary. Migrated this patch from bug 26351 so it gets it's own mention in the release notes. Created attachment 118680 [details] [review] Bug 28026: Add call_recursive() as a supliment for call() The method call() is not sufficient for all plugin hook types. It's possible that more than one plugin will be installed that wants to return the arguaments passed in an updated form. These transformation plugins need to work recursively rather than independantly. This patch adss a `call_recursive()` method that takes the output of the first plugin and uses it as the input for the next plugin and so on. This allowes each plugin to see the current version of the arguament list and modify it as necessary. Test plan 1/ Run the included tests - t/db_dependent/Koha/Plugins/Plugins.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> I'd love a comment/signoff from Mark Hofstetter to confirm this meets the needs he identifies in bug 28010.. and I think it's worth a QA eye other than my own as it skipped Signoff as a QA followup on it's "birth bug" I suggested something that would not require adding a new method nor modifying the 'call' method (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26351#c7) What do you think of it ? (In reply to Julian Maurice from comment #5) > I suggested something that would not require adding a new method nor > modifying the 'call' method > (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26351#c7) > What do you think of it ? There's a decent amount of code utilizing call_recursive now, so I'm a bit hesitant to make changes to code that has passed qa. If you want to submit your idea is a dependency on 26352 I'd absolutely take a look! I doubt that call() is sufficient, and as call_recursive() just adds functionality and does not interfere with the rest of teh plugin system. I'll try to QA it. (In reply to Mark Hofstetter from comment #7) > I doubt that call() is sufficient, and as call_recursive() just adds > functionality and does not interfere with the rest of teh plugin system. > I'll try to QA it. It is sufficient. I tested it. It's just a little less convenient for the caller when you want scalars to be modified (like in bug 26351 and bug 26252). Otherwise for objects it works very well: Koha::Plugins->call('object_transform', $object) will allow $object to be modified by all plugins. This is also true for hashrefs and arrayrefs. I believe call_recursive would be useful only for scalar values (and 'call' can already be used for that as shown in bug 26351 comment 7). And it duplicates the code of Koha::Plugins::call, which should be avoided IMO. (In reply to Julian Maurice from comment #8) > [...] and bug 26252 bug 26352 beeing new to (contributing to) Koha, I'd really to test/QA whatever comes out of this discussion (if this would be of help), but honestly I don't know what really to test - could somebody please give me a helping hand? Usually for sign-off you start with the test plan in the commit message... but here it's a little sparse: Test plan 1/ Run the included tests - t/db_dependent/Koha/Plugins/Plugins.t Checking coding guidelines etc. is great, but not expected for sign off, but for QA. (In reply to Julian Maurice from comment #8) Bug 28211 is ready to sign off Julian! Please merge with the other one (In reply to Marcel de Rooy from comment #13) > Please merge with the other one I'm not sure what bug you are referring too, but this is a series of 4 bugs now, so I don't think that would be practical. It would require modifying two bugs that are already passed qa. Created attachment 122443 [details] [review] Bug 28026: Add call_recursive() as a supliment for call() The method call() is not sufficient for all plugin hook types. It's possible that more than one plugin will be installed that wants to return the arguaments passed in an updated form. These transformation plugins need to work recursively rather than independantly. This patch adss a `call_recursive()` method that takes the output of the first plugin and uses it as the input for the next plugin and so on. This allowes each plugin to see the current version of the arguament list and modify it as necessary. Test plan 1/ Run the included tests - t/db_dependent/Koha/Plugins/Plugins.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> I think "call_pipeline" would've been better than "call_recursive", since I would expect "call_recursive" to invoke itself, instead of just pipelining results through a series of plugin calls. call_pipeline is a very fine name for me, I finally found the time to setup a proper devel system for koha. if somebody could find the time (30minutes) to chat with me how to do commit/package it/test it up to community standards I'd be happy to work on the patch Please see discussion and resulting bug 28211 where call_recursive is subsequently removed again in preference to passing references. If you don't believe this can fulfil your requirements Mark please let us know here to Julian and Kyle can chip in. Many thanks, Also, In bug 27066 comment 10 I talked about the concept of 'before', 'after' and 'around' as a hook naming convention.. I feel the hook names should be tied to their actions and effects.. So, in many ways I think 'before' should be passed the data prior to an action taking place and it's use should not affect said days, after should be passed the data after the action has taken place and again not affect that data.. around could be used for the case where we want to intercept and alter that data in the plugin... thoughts on such a naming convention would be great. (In reply to Martin Renvoize from comment #18) > Please see discussion and resulting bug 28211 where call_recursive is > subsequently removed again in preference to passing references. I was wondering why call_recursive was copying data rather than passing references hehe. (In reply to Martin Renvoize from comment #19) > Also, In bug 27066 comment 10 I talked about the concept of 'before', > 'after' and 'around' as a hook naming convention.. I feel the hook names > should be tied to their actions and effects.. > > So, in many ways I think 'before' should be passed the data prior to an > action taking place and it's use should not affect said days, after should > be passed the data after the action has taken place and again not affect > that data.. around could be used for the case where we want to intercept and > alter that data in the plugin... thoughts on such a naming convention would > be great. +1 from me. I use before/after/around with frameworks on other system, and it's very easy. I like the predictability of knowing when before/after/around hooks are called. As stated before I consider the plugin system to explicit ie you have to "tell" every function/method that there may be a plugin.
For example something I am currently working on:
Problem: Customer wants to change how patron cardnumbers are autogenerated, (a prefix and a fixed padded length)
One could make a plugin for Koha::Patron::fixup_cardnumber
OR, my suggestion (semi pseudo code, I hop my intentions are clear)
extend the base class(es) in this case
Koha::Object->_new_from_dbic();
if (<hooksenabled>) {
require KohaObjectHooks; # whatever
}
KohaObjectHooks
use Hook::LexWrap;
wrap 'Koha::Patron::fixup_cardnumber', post =>
sub { my ( $self ) = @_;
$self->cardnumber( sprintf("LB%09d", $self->cardnumber));
};
Hook::LexWrap hat pre/post a around hook may be implemented easily
I know this a major change, but solves a least some problems
(In reply to Mark Hofstetter from comment #21) > > OR, my suggestion (semi pseudo code, I hop my intentions are clear) > I'm sorry but I didn't really follow that. But that might just be me. (In reply to Mark Hofstetter from comment #21) > As stated before I consider the plugin system to explicit ie you have to > "tell" every function/method that there may be a plugin. > > For example something I am currently working on: > > Problem: Customer wants to change how patron cardnumbers are autogenerated, > (a prefix and a fixed padded length) > > One could make a plugin for Koha::Patron::fixup_cardnumber > > OR, my suggestion (semi pseudo code, I hop my intentions are clear) > > extend the base class(es) in this case > > Koha::Object->_new_from_dbic(); > > if (<hooksenabled>) { > require KohaObjectHooks; # whatever > } > > > KohaObjectHooks > > use Hook::LexWrap; > wrap 'Koha::Patron::fixup_cardnumber', post => > sub { my ( $self ) = @_; > $self->cardnumber( sprintf("LB%09d", $self->cardnumber)); > }; > > > > Hook::LexWrap hat pre/post a around hook may be implemented easily > > I know this a major change, but solves a least some problems That's interesting. I'd love to see a proof of concept! I think I understand a little better now, although I still don't understand the logic flow. You'd have to load the plugin to override the original code, but your example doesn't say when that would happen. Unless you're preloading modules at startup time (which will only work with Plack and could mean large worker processes), then you're going to have to load at script load time, but then you'll probably want to be targeting particular plugins for particular situations, and then I'm not sure that you're actually gaining any advantage over the current way of doing things? Hook::LexWrap is also black magic. Perl lets a person do all kinds of wild things, but it reminds me of Jurassic Park: "Your scientists were so preoccupied with whether or not they could, they didn't stop to think if they should." Created attachment 122527 [details] [review] 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> " Hook::LexWrap is also black magic. Perl lets a person do all kinds of wild things, but it reminds me of Jurassic Park: "Your scientists were so preoccupied with whether or not they could, they didn't stop to think if they should." " As I've got a background in biochemistry I am huge fan of tinkering with old (genetic) code Hook::LexWrap is widely used (and written by Damien), so it's not too bad please have a look at it, if the idea makes sense for you I'll make it pluggable (probably with something like Module::Pluggable) (In reply to Mark Hofstetter from comment #26) > " > Hook::LexWrap is also black magic. Perl lets a person do all kinds of wild > things, but it reminds me of Jurassic Park: "Your scientists were so > preoccupied with whether or not they could, they didn't stop to think if > they should." > " > > As I've got a background in biochemistry I am huge fan of tinkering with old > (genetic) code > > Hook::LexWrap is widely used (and written by Damien), so it's not too bad > > please have a look at it, if the idea makes sense for you I'll make it > pluggable (probably with something like Module::Pluggable) Mark, this is not the right place to put your patch. Can you please open a new b ug report and put it there? Thanks! (In reply to Kyle M Hall from comment #27) > (In reply to Mark Hofstetter from comment #26) > > " > > Hook::LexWrap is also black magic. Perl lets a person do all kinds of wild > > things, but it reminds me of Jurassic Park: "Your scientists were so > > preoccupied with whether or not they could, they didn't stop to think if > > they should." > > " > > > > As I've got a background in biochemistry I am huge fan of tinkering with old > > (genetic) code > > > > Hook::LexWrap is widely used (and written by Damien), so it's not too bad > > > > please have a look at it, if the idea makes sense for you I'll make it > > pluggable (probably with something like Module::Pluggable) > > Mark, this is not the right place to put your patch. Can you please open a > new b ug report and put it there? Thanks! And please add me on that bug! This idea is very interesting but definitely warrants discussion on its own bug. # Subtest: call_recursive() tests
1..6
ok 1 - Got back one element
not ok 2 - Got expected response
# Failed test 'Got expected response'
# at t/db_dependent/Koha/Plugins/Plugins.t line 97.
# got: '1'
# expected: '4'
ok 3 - Got back one element
ok 4 - Got expected response
ok 5 - Got back one element
ok 6 - call_recursive should return the original arguments if plugins are disabled
# Looks like you failed 1 test of 6.
not ok 6 - call_recursive() tests
sorry for the noise, moved the "proposal" to https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28655 please don't hesitate to correct/nudge me to koha community standards as I am new at commiting to this project (In reply to Marcel de Rooy from comment #29) > # Subtest: call_recursive() tests > 1..6 > ok 1 - Got back one element > not ok 2 - Got expected response > # Failed test 'Got expected response' > # at t/db_dependent/Koha/Plugins/Plugins.t line 97. > # got: '1' > # expected: '4' > ok 3 - Got back one element > ok 4 - Got expected response > ok 5 - Got back one element > ok 6 - call_recursive should return the original arguments if plugins > are disabled > # Looks like you failed 1 test of 6. > not ok 6 - call_recursive() tests Same here, @khall? Created attachment 123788 [details] [review] Bug 28026: Add call_recursive() as a supliment for call() The method call() is not sufficient for all plugin hook types. It's possible that more than one plugin will be installed that wants to return the arguaments passed in an updated form. These transformation plugins need to work recursively rather than independantly. This patch adss a `call_recursive()` method that takes the output of the first plugin and uses it as the input for the next plugin and so on. This allowes each plugin to see the current version of the arguament list and modify it as necessary. Test plan 1/ Run the included tests - t/db_dependent/Koha/Plugins/Plugins.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 123789 [details] [review] Bug 28026: Add call_recursive() as a supplement for call() The method call() is not sufficient for all plugin hook types. It's possible that more than one plugin will be installed that wants to return the arguaments passed in an updated form. These transformation plugins need to work recursively rather than independantly. This patch adss a `call_recursive()` method that takes the output of the first plugin and uses it as the input for the next plugin and so on. This allowes each plugin to see the current version of the arguament list and modify it as necessary. Test plan 1/ Run the included tests - t/db_dependent/Koha/Plugins/Plugins.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 123829 [details] [review] Bug 28026: Add call_recursive() as a supplement for call() The method call() is not sufficient for all plugin hook types. It's possible that more than one plugin will be installed that wants to return the arguaments passed in an updated form. These transformation plugins need to work recursively rather than independantly. This patch adss a `call_recursive()` method that takes the output of the first plugin and uses it as the input for the next plugin and so on. This allowes each plugin to see the current version of the arguament list and modify it as necessary. Test plan 1/ Run the included tests - t/db_dependent/Koha/Plugins/Plugins.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 124311 [details] [review] Bug 28026: Add call_recursive() as a supplement for call() The method call() is not sufficient for all plugin hook types. It's possible that more than one plugin will be installed that wants to return the arguaments passed in an updated form. These transformation plugins need to work recursively rather than independantly. This patch adss a `call_recursive()` method that takes the output of the first plugin and uses it as the input for the next plugin and so on. This allowes each plugin to see the current version of the arguament list and modify it as necessary. Test plan 1/ Run the included tests - t/db_dependent/Koha/Plugins/Plugins.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> this is a rehash of my previous argument, I'll look at the code. How to you gurantee that the plugins are executed in a certain order because that's what you somehow imply by "method that takes the output of the first plugin and uses it as the input for the next plugin [..]" (In reply to Mark Hofstetter from comment #36) > this is a rehash of my previous argument, I'll look at the code. > > How to you gurantee that the plugins are executed in a certain order because > that's what you somehow imply by "method that takes the output of > the first plugin and uses it as the input for the next plugin [..]" There is no guarantee, and plugins should be written with that understanding. It would be possible to some day give all plugins a "priority" but at this time every plugin author should understand their plugin could be run first, last and or anywhere in between. @RM: What holds you back from pushing this development? This makes me doubt if I should QA things that depend on it. If you have doubts on this one, please indicate so on the report? Pushed to master for 21.11, thanks to everybody involved! |