Bug 34713 - Standardize plugin hooks
Summary: Standardize plugin hooks
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-09-05 10:50 UTC by Pedro Amorim
Modified: 2024-04-11 08:13 UTC (History)
11 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-09-05 10:50:07 UTC
We should standardize the way plugin hooks are implemented and behave.

Looking at:
https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
https://github.com/bywatersolutions/dev-koha-plugin-kitchen-sink

Some hooks are called like:
Koha::Plugins->new()->GetPlugins( { method => 'background_tasks', } );
and then the respective method invoked

Others are just called like:
Koha::Plugins->call('patron_barcode_transform', \$patron_id );

Some hooks return stuff (e.g. opac_detail_xslt_variables), others expect the param to be a ref and change in place without returning anything (e.g. patron_barcode_transform)

---

Discussion:

We already have hooks that behave like actions, and others that behave like filters. Wordpress put's it this way:
- an action interrupts the code flow to do something, and then returns back to the normal flow without modifying anything;
- a filter is used to modify something in a specific way so that the modification is then used by code later on.

My proposal is:
- Make it clear that a hook is either an action or a filter, either by name or by the way it's called
- All actions should implement the same (example) Plugins->call('action_name', $args) signature
- All filters should implement the same (example) Plugins->call('filter_name', $args) signature and return $args
Comment 1 Marcel de Rooy 2023-09-05 11:11:45 UTC
(In reply to Pedro Amorim from comment #0)
> We should standardize the way plugin hooks are implemented and behave.

> Others are just called like:
> Some hooks return stuff (e.g. opac_detail_xslt_variables), others expect the
> param to be a ref and change in place without returning anything (e.g.
> patron_barcode_transform)

The ->call pattern is more recent. I think we should go in that direction.

> We already have hooks that behave like actions, and others that behave like
> filters.
> - Make it clear that a hook is either an action or a filter, either by name
> or by the way it's called

Not sure what you exactly mean with a filter here. But a plugin could modify or just return results that allow further customization.
If you pass a ref to a plugin, you wont know what will happen. So you cant say that it is just a filter or so? Passing that ref means that it may be changed. Calling the plugin may lead to db changes too. The name is no protection..
Comment 2 Julian Maurice 2023-09-05 12:24:13 UTC
I don't think the distinction between filters and actions is that important. A plugin method can do both. For instance an item_barcode_transform hook can modify the barcode, or log it somewhere, or both.
Sure, the caller sometimes expects the plugin to act as a filter (like item_barcode_transform) but most of the times it does not expect anything. It's more like "This event just happened (or will happen right now), do what you want with that info"

I think standardizing name cannot be a bad thing, but the thing that matters the most (to me at least) is a proper documentation of the hook arguments: what they contain and if (and how) they are used after the hook returns.

In some implementations of an event manager, the event listener (the hook) gets an object acting as a container for the event and its parameter. Maybe we can take inspirations from that to standardize the hook signatures.
Comment 3 Pedro Amorim 2023-09-05 15:36:23 UTC
(In reply to Marcel de Rooy from comment #1)
> The ->call pattern is more recent. I think we should go in that direction.

Great! I agree. I think any eventual future documentation should have this as a pattern to be adopted (for new hooks being added in the future).

(In reply to Julian Maurice from comment #2)
> I think standardizing name cannot be a bad thing, but the thing that matters
> the most (to me at least) is a proper documentation of the hook arguments:
> what they contain and if (and how) they are used after the hook returns.

Yes, I agree and this is something I left out of my initial post but something I strongly believe we need: Proper documentation (and usage examples) of all available plugin hooks, as well as their arguments and return value - if any.
The best closest thing we have atm is the kitchen sink plugin's actual code imo.

@Marcel the idea behind "filter" versus "action" would be to make it clear, right off the bat, to anyone implementing any hook whether it just triggers an action (and has no return value) e.g. after_biblio_action or if the hook filters some args and returns that after some modifications have been made  e.g. patron_barcode_transform

I only used the naming argument as a starting point of discussion regarding standardization and I don't feel strongly one way or another. I started that as a suggestion but I believe the main 2 issues are:
- Hooks are not standardized - each hook has its own way of handling arguments, and in different ways, or no arguments at all.
- Docs are poor, the koha plugin hooks page is better than nothing but a bit lacking in terms of dev documentation or usage examples.

On a wider topic:
https://perldoc.koha-community.org/ - Doesn't look like it's been maintained (20.05 is the latest listed there). And looks ugly =D
I wonder if we could use something like https://about.readthedocs.com/ for Koha, to document patterns we use, not just plugin related.
Or a (better) way of getting documentation generated using POD from the code.