From 74effac4f391e5daa8d0d43eecbe9f1350b75dfc Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Fri, 20 Sep 2019 14:46:13 -0700 Subject: [PATCH] (POC) Add `hook_civicrm_postCommit`, a variant of `hook_civicrm_post` Overview -------- So here's a pattern that's occurred a few times: one wants to provide an extra notification or log or correlated-record after something is chanaged or created. So you implement `hook_civicrm_post`. It sounds simple, but it doesn't work quite as expected - because running within the transaction can have some special implications: 1. Performing subsequent SQL queries within the same transaction be wonky 2. Errors in your notification bubble-up and break the transaction. You can resolve this by deferring your work until the transaction completes. The technique has been discussed in various media over the years; e.g. here's a mention in the dev docs: https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_post/#example-with-transaction-callback However, I think the problem may be that the default is basically backwards: there are more use-cases for 'post' hook in which you prefer to run *after the transaction commits*, but doing that case requires the special incantation. This patch is a proof-of-concept in which the system provides two hooks: * `hook_civicrm_post`: Runs immediately after the change is *sent* to the DB. If there's a SQL transaction, then it runs *within* the transaction. * `hook_civicrm_postCommit`: Runs after the change is *committed* to the DB. Runs outside of any SQL transactions. My theory is that more developers would run their logic at the correct time if they had `postCommit` available as a hook (and, of course, the downstream code would look tidier). This isn't really a pain-point for me, so I'm not super-motivated to push it, and I haven't looked very hard for systemic side-effects of buffering more posts. However, I think it could provide better DX (making it easier for more folks to get the right timing), so I wanted to share the POC. Before ------ ```php /** * Hook fired after the INSERT/UPDATE is sent to the DB */ function hook_civicrm_post($op, $objectName, $objectId, &$objectRef) ``` After ------ ```php /** * Hook fired after the INSERT/UPDATE is sent to the DB */ function hook_civicrm_post($op, $objectName, $objectId, &$objectRef); /** * Hook fired after the record is committed to the DB. * This may be immediate (for non-transactional work) or it may be * delayed a few milliseconds (for transactional work). */ function hook_civicrm_postCommit($op, $objectName, $objectId, $objectRef); ``` --- CRM/Core/Transaction.php | 20 ++++++++++++++++++++ CRM/Utils/Hook.php | 30 ++++++++++++++++++++++++++++++ Civi/Core/Container.php | 1 + 3 files changed, 51 insertions(+) diff --git a/CRM/Core/Transaction.php b/CRM/Core/Transaction.php index e23e9f4315..d42197f9de 100644 --- a/CRM/Core/Transaction.php +++ b/CRM/Core/Transaction.php @@ -252,4 +252,24 @@ class CRM_Core_Transaction { $frame->addCallback($phase, $callback, $params, $id); } + /** + * Whenever hook_civicrm_post fires, schedule an equivalent + * call to hook_civicrm_postCommit. + * + * @param \Civi\Core\Event\PostEvent $e + * @see CRM_Utils_Hook::post + */ + public static function addPostCommit($e) { + // Do we want to dedupe post-commit hooks for the same txn? Setting an ID + // would allow this. + // $id = $e->entity . chr(0) . $e->action . chr(0) . $e->id; + $frame = \Civi\Core\Transaction\Manager::singleton()->getBaseFrame(); + if ($frame) { + $frame->addCallback(self::PHASE_POST_COMMIT, ['CRM_Utils_Hook', 'postCommit'], [$e->action, $e->entity, $e->id, $e->object]); + } + else { + \CRM_Utils_Hook::postCommit($e->action, $e->entity, $e->id, $e->object); + } + } + } diff --git a/CRM/Utils/Hook.php b/CRM/Utils/Hook.php index 76887f2bc3..e0e7a9eec1 100644 --- a/CRM/Utils/Hook.php +++ b/CRM/Utils/Hook.php @@ -393,6 +393,36 @@ abstract class CRM_Utils_Hook { return $event->getReturnValues(); } + /** + * This hook is equivalent to post(), except that it is guaranteed to run + * outside of any SQL transaction. The objectRef is not modifiable. + * + * This hook is defined for two cases: + * + * 1. If the original action runs within a transaction, then the hook fires + * after the transaction commits. + * 2. If the original action runs outside a transaction, then the data was + * committed immediately, and we can run the hook immediately. + * + * @param string $op + * The type of operation being performed. + * @param string $objectName + * The name of the object. + * @param int $objectId + * The unique identifier for the object. + * @param object $objectRef + * The reference to the object if available. + * + * @return mixed + * based on op. pre-hooks return a boolean or + * an error message which aborts the operation + */ + public static function postCommit($op, $objectName, $objectId, $objectRef = NULL) { + $event = new \Civi\Core\Event\PostEvent($op, $objectName, $objectId, $objectRef); + \Civi::dispatcher()->dispatch('hook_civicrm_postCommit', $event); + return $event->getReturnValues(); + } + /** * This hook retrieves links from other modules and injects it into. * the view contact tabs diff --git a/Civi/Core/Container.php b/Civi/Core/Container.php index 0dfebb73ed..c9232136f0 100644 --- a/Civi/Core/Container.php +++ b/Civi/Core/Container.php @@ -335,6 +335,7 @@ class Container { $dispatcher->addListener(SystemInstallEvent::EVENT_NAME, ['\Civi\Core\InstallationCanary', 'check']); $dispatcher->addListener(SystemInstallEvent::EVENT_NAME, ['\Civi\Core\DatabaseInitializer', 'initialize']); $dispatcher->addListener(SystemInstallEvent::EVENT_NAME, ['\Civi\Core\LocalizationInitializer', 'initialize']); + $dispatcher->addListener('hook_civicrm_post', ['\CRM_Core_Transaction', 'addPostCommit'], -1000); $dispatcher->addListener('hook_civicrm_pre', ['\Civi\Core\Event\PreEvent', 'dispatchSubevent'], 100); $dispatcher->addListener('hook_civicrm_post', ['\Civi\Core\Event\PostEvent', 'dispatchSubevent'], 100); $dispatcher->addListener('hook_civicrm_post::Activity', ['\Civi\CCase\Events', 'fireCaseChange']); -- 2.25.1