Bug 40984

Summary: Synchronise before_ and after_authority_action hook signatures with biblio action hooks
Product: Koha Reporter: Martin Renvoize (ashimema) <martin.renvoize>
Component: Plugin architectureAssignee: Martin Renvoize (ashimema) <martin.renvoize>
Status: Failed QA --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: fridolin.somers, kyle, m.de.rooy, pedro.amorim, tomascohen, wainuiwitikapark
Version: MainKeywords: Hackfest
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36343
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
IMPORTANT: The authority and authority_id parameters of after_authority_action_hooks plugins have been deprecated! Please switch to using the payload hash. The old parameters will be removed in a future version of Koha. Note that this report also adds before_authority_action_hooks allowing you to modify authority records before saving.
Version(s) released in:
Circulation function:
Attachments: Bug 40984: Synchronize before_ and after_authority_action hooks with biblio action hooks

Description Martin Renvoize (ashimema) 2025-10-09 12:09:56 UTC
Following the work on bug 36343, which standardized the signatures for before_biblio_action and after_biblio_action hooks to include a payload parameter, a similar inconsistency exists within the authority hooks.

To improve the consistency and predictability of the plugin hook system, we should align the authority action hooks with the syntax established for biblios.

This bug covers two related issues:

Update after_authority_action hook signature:
An after_authority_action hook already exists, but its signature is missing the payload parameter. We should update this hook to include the payload, bringing it in line with after_biblio_action. A deprecation period and warning should be introduced for the older signature to ensure backward compatibility for existing plugins.

Create before_authority_action hook:
There is currently no equivalent before_authority_action hook. We should create this new hook to allow plugins to interact with authority records before an action (like saving) is completed. This new hook should be implemented with a signature that is consistent with before_biblio_action from the outset, including the payload parameter.

Completing this work will provide a more coherent and powerful interface for plugins that interact with both bibliographic and authority records.
Comment 1 Martin Renvoize (ashimema) 2025-10-09 12:23:35 UTC
Created attachment 187653 [details] [review]
Bug 40984: Synchronize before_ and after_authority_action hooks with biblio action hooks

This patch updates the authority action plugin hooks to match the
signature and behavior of the biblio action hooks:

1. Created before_authority_action hook that is called before authority
   records are saved, allowing plugins to modify records before they are
   stored in the database

2. Updated after_authority_action hook to use a payload parameter
   containing authority, authority_id, and record, matching the pattern
   used by after_biblio_action hook

3. Maintained backward compatibility by passing both old parameters
   (authority, authority_id) and new payload parameter, with deprecation
   warnings logged when plugins implement the hook

4. Updated test plugin to use new payload-based signatures for both hooks

5. Added test coverage for before_authority_action hook

Test plan:
1. Run: prove t/db_dependent/Koha/Plugins/authority_hooks.t
2. Run: prove t/db_dependent/Koha/Plugins/
3. All tests should pass
Comment 2 Marcel de Rooy 2025-10-10 07:08:29 UTC
I am not so sure about this:

+    # Log deprecation warning if any plugin is still using the old signature
+    my @plugins = Koha::Plugins->get_enabled_plugins( { verbose => 0 } );
+    @plugins = grep { $_->can('after_authority_action') } @plugins;
+    if (@plugins) {
+        Koha::Logger->get->warn(
+            "after_authority_action hook: The old signature (with authority and authority_id as direct parameters) "
+                . "is deprecated and will be removed in a future release. "
+                . "Please update your plugin to use the 'payload' parameter instead." );
+    }
Comment 3 Marcel de Rooy 2025-10-10 07:09:53 UTC
Potentially flooding the logs with deprecation warns and not even checking if we use the old or the new syntax?
Comment 4 Marcel de Rooy 2025-10-10 07:23:17 UTC
    # Call before hook
    _before_authority_action_hooks( { action => 'save', authority_id => $authid, record => $record } );

Not sure about the exact place we should add this call.
It is a bit in the middle of various changes in AddAuthority now.
For MARC21 we did already update fields like leader, 003, 005, 008. But did not yet update 001 and heading.

Comparing with the biblio action, we do the before call after record changes in ModBiblioMarc and just before saving to biblio_metadata.

Could you explain why we do it here? Shouldnt we better move it after the heading change just before the $authority->update?