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.
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.
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>
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, } ); }
sub generate_userid { my ( $self ) = @_; my @responses = Koha::Plugins->call( 'patron_generate_userid', { patron => $self },
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>
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>
Apart from release notes maybe even db rev to print warning? How do we arrange for the needed follow-up ?
min or enh? What about backports?
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.
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.
Created attachment 164910 [details] [review] Bug 36343: (QA follow-up) Add warning in DB update
Pushed for 24.05! Well done everyone, thank you!
I suggest we do not backport to 23.11.x