Bug 28026 - Add a 'call_recursive' method to Koha::Plugins
Summary: Add a 'call_recursive' method to Koha::Plugins
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 26351
  Show dependency treegraph
 
Reported: 2021-03-23 16:02 UTC by Martin Renvoize
Modified: 2021-10-08 14:48 UTC (History)
7 users (show)

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


Attachments
Bug 28026: Add call_recursive() as a supliment for call() (3.81 KB, patch)
2021-03-23 16:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28026: Add call_recursive() as a supliment for call() (3.95 KB, patch)
2021-03-23 16:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28026: Add call_recursive() as a supliment for call() (3.96 KB, patch)
2021-06-25 12:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
THIS IS NOT A BUGFIX JUST A PROVE OF CONCEPT (2.73 KB, patch)
2021-07-02 07:26 UTC, Mark Hofstetter
Details | Diff | Splinter Review
Bug 28026: Add call_recursive() as a supliment for call() (4.37 KB, patch)
2021-08-11 12:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28026: Add call_recursive() as a supplement for call() (4.37 KB, patch)
2021-08-11 12:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28026: Add call_recursive() as a supplement for call() (4.41 KB, patch)
2021-08-13 16:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28026: Add call_recursive() as a supplement for call() (4.43 KB, patch)
2021-08-31 17:32 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2021-03-23 16:02:48 UTC
We have a 'call' method already which can be used to call plugin hooks iteratively with the original arguments each time.

It would be helpful to also have a method to allow continuation-passing by passing the returned arguments from one plugin call to the next.
Comment 1 Martin Renvoize 2021-03-23 16:19:28 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.
Comment 2 Martin Renvoize 2021-03-23 16:20:13 UTC
Migrated this patch from bug 26351 so it gets it's own mention in the release notes.
Comment 3 Martin Renvoize 2021-03-23 16:21:41 UTC
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>
Comment 4 Martin Renvoize 2021-03-23 16:24:30 UTC
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"
Comment 5 Julian Maurice 2021-04-09 10:40:20 UTC
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 ?
Comment 6 Kyle M Hall 2021-04-13 16:27:14 UTC
(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!
Comment 7 Mark Hofstetter 2021-04-13 17:35:47 UTC
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.
Comment 8 Julian Maurice 2021-04-16 10:11:30 UTC
(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.
Comment 9 Julian Maurice 2021-04-16 10:14:05 UTC
(In reply to Julian Maurice from comment #8)
> [...] and bug 26252
bug 26352
Comment 10 Mark Hofstetter 2021-04-18 20:06:20 UTC
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?
Comment 11 Katrin Fischer 2021-04-18 21:45:04 UTC
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.
Comment 12 Kyle M Hall 2021-04-23 16:00:22 UTC
(In reply to Julian Maurice from comment #8)

Bug 28211 is ready to sign off Julian!
Comment 13 Marcel de Rooy 2021-06-23 14:14:21 UTC
Please merge with the other one
Comment 14 Kyle M Hall 2021-06-23 15:26:31 UTC
(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.
Comment 15 Kyle M Hall 2021-06-25 12:10:21 UTC
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>
Comment 16 David Cook 2021-06-28 00:57:01 UTC
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.
Comment 17 Mark Hofstetter 2021-06-28 06:52:40 UTC
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
Comment 18 Martin Renvoize 2021-06-28 08:42:00 UTC
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,
Comment 19 Martin Renvoize 2021-06-28 14:10:54 UTC
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.
Comment 20 David Cook 2021-06-28 23:41:41 UTC
(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.
Comment 21 Mark Hofstetter 2021-06-29 06:16:14 UTC
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
Comment 22 David Cook 2021-07-01 01:00:34 UTC
(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.
Comment 23 Kyle M Hall 2021-07-01 14:24:10 UTC
(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!
Comment 24 David Cook 2021-07-02 01:05:32 UTC
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."
Comment 25 Mark Hofstetter 2021-07-02 07:26:01 UTC
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>
Comment 26 Mark Hofstetter 2021-07-02 07:31:38 UTC
"
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)
Comment 27 Kyle M Hall 2021-07-02 11:03:57 UTC
(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!
Comment 28 Kyle M Hall 2021-07-02 11:06:30 UTC
(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.
Comment 29 Marcel de Rooy 2021-07-02 11:53:49 UTC
# 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
Comment 30 Mark Hofstetter 2021-07-02 11:59:39 UTC
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
Comment 31 Tomás Cohen Arazi 2021-08-10 17:00:28 UTC
(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?
Comment 32 Kyle M Hall 2021-08-11 12:37:26 UTC
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>
Comment 33 Kyle M Hall 2021-08-11 12:38:15 UTC
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>
Comment 34 Tomás Cohen Arazi 2021-08-13 16:17:06 UTC
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>
Comment 35 Kyle M Hall 2021-08-31 17:32:59 UTC
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>
Comment 36 Mark Hofstetter 2021-09-01 08:16:17 UTC
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 [..]"
Comment 37 Kyle M Hall 2021-09-01 10:29:03 UTC
(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.
Comment 38 Marcel de Rooy 2021-10-01 09:17:13 UTC
@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?
Comment 39 Jonathan Druart 2021-10-06 12:57:10 UTC
Pushed to master for 21.11, thanks to everybody involved!