Bug 36343 - The 'after_biblio_action' hooks have an inconsistent signature compared to before_biblio_action, and actions in reserves and items
Summary: The 'after_biblio_action' hooks have an inconsistent signature compared to be...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords: Hackfest, release-notes-needed
Depends on:
Blocks:
 
Reported: 2024-03-18 13:29 UTC by Martin Renvoize
Modified: 2024-05-23 13:01 UTC (History)
5 users (show)

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


Attachments
Bug 36343: Add support for 'payload' to _after_biblio_action (1.34 KB, patch)
2024-03-18 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36343: Add support for 'payload' to _after_biblio_action (1.48 KB, patch)
2024-03-25 09:45 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 36343: Add support for 'payload' to _after_biblio_action (1.57 KB, patch)
2024-04-05 07:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36343: (QA follow-up) Adjust two other occurrences (1.62 KB, patch)
2024-04-05 07:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36343: (QA follow-up) Add warning in DB update (1.10 KB, patch)
2024-04-16 12:14 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2024-03-18 13:29:59 UTC
Our hooks should attempt to have a consistent routine signature whenever that can.. unfortunately, we've not been very good at spotting when patterns change.

Whilst QAing bug 34943 we discovered an inconsistency in the hook signatures again.

'payload' vs no-payload.

I propose we add support for 'payload' to the _after_biblio_action hook and set a deprecation period for the older signature style.
Comment 1 Martin Renvoize 2024-03-18 13:37:59 UTC
Created attachment 163328 [details] [review]
Bug 36343: Add support for 'payload' to _after_biblio_action

This patch adds support for the payload key into the _after_bilio_action
plugin hook wrapper routine.

This brings it more inline with how these 'action hooks' appear in Holds
and circulation.
Comment 2 Frédéric Demians 2024-03-25 09:45:33 UTC
Created attachment 163768 [details] [review]
Bug 36343: Add support for 'payload' to _after_biblio_action

This patch adds support for the payload key into the _after_bilio_action
plugin hook wrapper routine.

This brings it more inline with how these 'action hooks' appear in Holds
and circulation.

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
Comment 3 Marcel de Rooy 2024-04-05 07:20:35 UTC
sub _after_item_action_hooks {
    my ( $self, $params ) = @_;

    my $action = $params->{action};

    Koha::Plugins->call(
        'after_item_action',
        {
            action  => $action,
            item    => $self,
            item_id => $self->itemnumber,
        }
    );
}
Comment 4 Marcel de Rooy 2024-04-05 07:25:03 UTC
sub generate_userid {
    my ( $self ) = @_;
    my @responses = Koha::Plugins->call(
        'patron_generate_userid', { patron => $self },
Comment 5 Marcel de Rooy 2024-04-05 07:34:02 UTC
Created attachment 164455 [details] [review]
Bug 36343: Add support for 'payload' to _after_biblio_action

This patch adds support for the payload key into the _after_bilio_action
plugin hook wrapper routine.

This brings it more inline with how these 'action hooks' appear in Holds
and circulation.

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2024-04-05 07:34:05 UTC
Created attachment 164456 [details] [review]
Bug 36343: (QA follow-up) Adjust two other occurrences

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2024-04-05 07:35:12 UTC
Apart from release notes maybe even db rev to print warning?
How do we arrange for the needed follow-up ?
Comment 8 Marcel de Rooy 2024-04-05 07:35:49 UTC
min or enh? What about backports?
Comment 9 Katrin Fischer 2024-04-05 09:34:32 UTC
Agree with the change, but would like to see a reply to Marcel's comment before pushing. Another thing we should do is update the hooks wiki page with a note.
Comment 10 Martin Renvoize 2024-04-16 09:23:14 UTC
Good questions.. we can certainly add a DB update to visibly warn people at upgrade time.

In the longer term I'd like to see this sort of thing documented in the developers' side of the proposed plugin store from Hackfest24.

We could use the https://docs.mojolicious.org/Mojo/Util#deprecated to visibly warn.. but I'm not entirely sure how we could get explicit about catching if the plugin author used the old syntax.

As for ensuring we have a removal happen.. I'm not entirely sure.
Comment 11 Martin Renvoize 2024-04-16 12:14:39 UTC
Created attachment 164910 [details] [review]
Bug 36343: (QA follow-up) Add warning in DB update
Comment 12 Katrin Fischer 2024-04-16 16:01:58 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 13 Fridolin Somers 2024-05-23 13:01:55 UTC
I suggest we do not backport to 23.11.x