Allow extensions to skip entity operations by catching exceptions thrown in pre and post hooks

Overview

Extensions should be able to skip actions performed on entities with a warning/error message. hook_civicrm_pre() and hook_civicrm_post() implementations do not allow for skipping altogether, but only for altering parameters.

Example use-case

The de.systopia.donrec extension forbids contributions be deleted when they have been legally receipted, since allowing deletion of such contributions would be considered fraudulent behavior by tax authorities.

Current behaviour

Currently, exceptions thrown in implementations of those hooks produce fatal errors, which, when run via Ajax, appear to "hang the system", because the XHR returns a 500. There is no way for extensions to skip such an action in a way that would be handeled by the UI.

Proposed behaviour

hook_civicrm_pre() and hook_civicrm_post() implementations should be able to skip execution of the current action (e.g. deleting an entity). The invoking code should therefore catch exceptions thrown by those implementations. This would allow for displaying exception messages as error message popups in the UI.

Comments

I'm not sure whether this can be done generically or would have to be implemented separately for each caller. I guess it's the latter. In that case, consider this issue as a discussion starter for whether this is desirable to include in CiviCRM Core.

I imagine something like this https://github.com/civicrm/civicrm-core/compare/master...eileenmcnaughton:except?expand=1

@eileen Thanks! Would this work similarly for other operations (update, insert, etc.)?

In theory - I drew @colemanw's attention to it on https://github.com/civicrm/civicrm-core/pull/17172

With old-style hooks I guess throwing an exception is the only recourse, but our hooks also go through the event dispatcher system, which I think would let you do something like $event->cancel(). So we could just say that anyone who wants to cancel event will need to use the newer event style to implement their hook.

@colemanw would that work to prevent a delete? (without derailing everything else?)

Wouldn't it be niceer to implement a hook which is called just before the pre to check whether the action is valid? Because I could imagine that other extension might do different things in the pre/post hook on the assumption the entity is saved/deleted

added git:civicrm-core needs:concept approval needs:docs need update triaged labels

added ~88 type:proposal labels

@jaapjansma Couldn't that be handled by transactions, so that when one hook implementation aborts the action, the transaction would not be committed? I think the hook_civicrm_post() already works like this. Otherwise one could always argue for just another level of "pre-pre" hooks.

Hi,

Thank you for raising an issue to help improve CiviCRM. As you may know, this issue has not had any activity for quite some time, so we have closed it.

We would like you to help us to determine if this issue should be re-opened:

• If this issue was reporting a bug, can you attempt to reproduce it on a latest version of CiviCRM?
• If this issue was proposing a new feature, can you verify if the feature proposal is still relevant? Did it get the concept-approved label? Have other people also shown interest? Could it be implemented as an extension?

If the answer to either question is yes, please feel free to comment or re-open the issue. Please also consider:

• Is it something that you could help implement, either by sending a patch or hiring someone who can?
• Would you be willing to fund the work, either through a CiviCRM partner, or the Core Team?

Thank you for your help and contributions to CiviCRM.

P.S. This is an automated message, see infra/gitlab issue 20. We understand that automatic responses are annoying, but given the number of open issues as the project evolves, we need a bit of help to triage and prioritise the most relevant issues.

closed