From 39eb69ec7d483b7d10abffb6ebd679d66d6b8446 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Tue, 10 Aug 2021 17:11:00 -0700 Subject: [PATCH 1/4] (REF) ActionSchedule - Remove unused `try`/`catch(TokenException $e)` This try/catch block purports to catch any exceptions of type `TokenException`. However, if you grep the source tree, you will find that `TokenException` is never thrown. --- CRM/Core/BAO/ActionSchedule.php | 55 +++++++++++++++------------------ 1 file changed, 25 insertions(+), 30 deletions(-) diff --git a/CRM/Core/BAO/ActionSchedule.php b/CRM/Core/BAO/ActionSchedule.php index d77feae9331b..014978230e83 100644 --- a/CRM/Core/BAO/ActionSchedule.php +++ b/CRM/Core/BAO/ActionSchedule.php @@ -269,40 +269,35 @@ public static function sendMailings($mappingID, $now) { $multilingual = CRM_Core_I18n::isMultilingual(); while ($dao->fetch()) { $errors = []; - try { - $tokenProcessor = self::createTokenProcessor($actionSchedule, $mapping); - $row = $tokenProcessor->addRow() - ->context('contactId', $dao->contactID) - ->context('actionSearchResult', (object) $dao->toArray()); - - // switch language if necessary - if ($multilingual) { - $preferred_language = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Contact', $dao->contactID, 'preferred_language'); - $row->context('locale', CRM_Core_BAO_ActionSchedule::pickLocale($actionSchedule->communication_language, $preferred_language)); - } - - foreach ($tokenProcessor->evaluate()->getRows() as $tokenRow) { - // It's possible, eg, that sendReminderEmail fires Hook::alterMailParams() and that some listener use ts(). - $swapLocale = empty($row->context['locale']) ? NULL : \CRM_Utils_AutoClean::swapLocale($row->context['locale']); + $tokenProcessor = self::createTokenProcessor($actionSchedule, $mapping); + $row = $tokenProcessor->addRow() + ->context('contactId', $dao->contactID) + ->context('actionSearchResult', (object) $dao->toArray()); + + // switch language if necessary + if ($multilingual) { + $preferred_language = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Contact', $dao->contactID, 'preferred_language'); + $row->context('locale', CRM_Core_BAO_ActionSchedule::pickLocale($actionSchedule->communication_language, $preferred_language)); + } - if ($actionSchedule->mode === 'SMS' || $actionSchedule->mode === 'User_Preference') { - CRM_Utils_Array::extend($errors, self::sendReminderSms($tokenRow, $actionSchedule, $dao->contactID)); - } + foreach ($tokenProcessor->evaluate()->getRows() as $tokenRow) { + // It's possible, eg, that sendReminderEmail fires Hook::alterMailParams() and that some listener use ts(). + $swapLocale = empty($row->context['locale']) ? NULL : \CRM_Utils_AutoClean::swapLocale($row->context['locale']); - if ($actionSchedule->mode === 'Email' || $actionSchedule->mode === 'User_Preference') { - CRM_Utils_Array::extend($errors, self::sendReminderEmail($tokenRow, $actionSchedule, $dao->contactID)); - } - // insert activity log record if needed - if ($actionSchedule->record_activity && empty($errors)) { - $caseID = empty($dao->case_id) ? NULL : $dao->case_id; - CRM_Core_BAO_ActionSchedule::createMailingActivity($tokenRow, $mapping, $dao->contactID, $dao->entityID, $caseID); - } + if ($actionSchedule->mode === 'SMS' || $actionSchedule->mode === 'User_Preference') { + CRM_Utils_Array::extend($errors, self::sendReminderSms($tokenRow, $actionSchedule, $dao->contactID)); + } - unset($swapLocale); + if ($actionSchedule->mode === 'Email' || $actionSchedule->mode === 'User_Preference') { + CRM_Utils_Array::extend($errors, self::sendReminderEmail($tokenRow, $actionSchedule, $dao->contactID)); } - } - catch (\Civi\Token\TokenException $e) { - $errors['token_exception'] = $e->getMessage(); + // insert activity log record if needed + if ($actionSchedule->record_activity && empty($errors)) { + $caseID = empty($dao->case_id) ? NULL : $dao->case_id; + CRM_Core_BAO_ActionSchedule::createMailingActivity($tokenRow, $mapping, $dao->contactID, $dao->entityID, $caseID); + } + + unset($swapLocale); } // update action log record From c06f174feaa0e82fac89c3709e6fe0ca36f3b9b4 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Tue, 10 Aug 2021 17:52:13 -0700 Subject: [PATCH 2/4] ActionSchedule - Fill TokenProcessor will real batches Background: `ActionSchedule::sendMailings()` fetches a batch of pending reminders (per some specific schedule/rule). Then it composes and sends a message for each. Before: For each item in the batch, it makes a new `TokenProcessor`. This means that the `TokenProcessor` only sees one pending reminder. Therefore, the `TokenProcessor` cannot meaningfully fetch batched data. After: It creates one `TokenProcessor` and adds rows for each pending reminder. This means that `TokenProcessor` can fetch batched data.# On branch master-action-batch Comments: This part of a longer plot to simplify the `CRM_*_Tokens`. Currently, `CRM_*_Tokens` implements duplicate prefetch mechanisms -- the general-purpose `prefetch()` and the special-purpose `alterActionScheduleQuery()`. This patch makes it possible to use `prefetch()` for all the real work of prefetching (and phasing-out `alterActionScheduleQuery()`). --- CRM/Core/BAO/ActionSchedule.php | 37 ++++++++++++++++++--------------- Civi/Token/TokenProcessor.php | 2 +- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/CRM/Core/BAO/ActionSchedule.php b/CRM/Core/BAO/ActionSchedule.php index 014978230e83..5821fced0653 100644 --- a/CRM/Core/BAO/ActionSchedule.php +++ b/CRM/Core/BAO/ActionSchedule.php @@ -267,9 +267,8 @@ public static function sendMailings($mappingID, $now) { ); $multilingual = CRM_Core_I18n::isMultilingual(); + $tokenProcessor = self::createTokenProcessor($actionSchedule, $mapping); while ($dao->fetch()) { - $errors = []; - $tokenProcessor = self::createTokenProcessor($actionSchedule, $mapping); $row = $tokenProcessor->addRow() ->context('contactId', $dao->contactID) ->context('actionSearchResult', (object) $dao->toArray()); @@ -279,27 +278,31 @@ public static function sendMailings($mappingID, $now) { $preferred_language = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Contact', $dao->contactID, 'preferred_language'); $row->context('locale', CRM_Core_BAO_ActionSchedule::pickLocale($actionSchedule->communication_language, $preferred_language)); } + } - foreach ($tokenProcessor->evaluate()->getRows() as $tokenRow) { - // It's possible, eg, that sendReminderEmail fires Hook::alterMailParams() and that some listener use ts(). - $swapLocale = empty($row->context['locale']) ? NULL : \CRM_Utils_AutoClean::swapLocale($row->context['locale']); + $tokenProcessor->evaluate(); + foreach ($tokenProcessor->getRows() as $tokenRow) { + $dao = $tokenRow->context['actionSearchResult']; + $errors = []; - if ($actionSchedule->mode === 'SMS' || $actionSchedule->mode === 'User_Preference') { - CRM_Utils_Array::extend($errors, self::sendReminderSms($tokenRow, $actionSchedule, $dao->contactID)); - } + // It's possible, eg, that sendReminderEmail fires Hook::alterMailParams() and that some listener use ts(). + $swapLocale = empty($row->context['locale']) ? NULL : \CRM_Utils_AutoClean::swapLocale($row->context['locale']); - if ($actionSchedule->mode === 'Email' || $actionSchedule->mode === 'User_Preference') { - CRM_Utils_Array::extend($errors, self::sendReminderEmail($tokenRow, $actionSchedule, $dao->contactID)); - } - // insert activity log record if needed - if ($actionSchedule->record_activity && empty($errors)) { - $caseID = empty($dao->case_id) ? NULL : $dao->case_id; - CRM_Core_BAO_ActionSchedule::createMailingActivity($tokenRow, $mapping, $dao->contactID, $dao->entityID, $caseID); - } + if ($actionSchedule->mode === 'SMS' || $actionSchedule->mode === 'User_Preference') { + CRM_Utils_Array::extend($errors, self::sendReminderSms($tokenRow, $actionSchedule, $dao->contactID)); + } - unset($swapLocale); + if ($actionSchedule->mode === 'Email' || $actionSchedule->mode === 'User_Preference') { + CRM_Utils_Array::extend($errors, self::sendReminderEmail($tokenRow, $actionSchedule, $dao->contactID)); + } + // insert activity log record if needed + if ($actionSchedule->record_activity && empty($errors)) { + $caseID = empty($dao->case_id) ? NULL : $dao->case_id; + CRM_Core_BAO_ActionSchedule::createMailingActivity($tokenRow, $mapping, $dao->contactID, $dao->entityID, $caseID); } + unset($swapLocale); + // update action log record $logParams = [ 'id' => $dao->reminderID, diff --git a/Civi/Token/TokenProcessor.php b/Civi/Token/TokenProcessor.php index 7ad2ec7b5842..d434a1a13919 100644 --- a/Civi/Token/TokenProcessor.php +++ b/Civi/Token/TokenProcessor.php @@ -256,7 +256,7 @@ public function getRow($key) { * Each row is presented with a fluent, OOP facade. */ public function getRows() { - return new TokenRowIterator($this, new \ArrayIterator($this->rowContexts)); + return new TokenRowIterator($this, new \ArrayIterator($this->rowContexts ?: [])); } /** From 3b191d7d7a24835a9cd19de49d6577484d5c4182 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Mon, 9 Aug 2021 22:59:54 -0700 Subject: [PATCH 3/4] (REF) ActionSchedule - Pass through SQL fields named "tokenContext_*" This will help us to consolidate the prefetching logic in `CRM_*_Tokens`. Currently, `CRM_*_Token::alterActionScheduleQuery()` updates the query to select all fields for the entity. This is regardless of how many fields there are, whether they are needed, etc. This is also prone to naming conflicts. With this patch, `CRM_*_Token::alterActionScheduleQuery()` only needs to select one field (`tokenContext_fooId`). This will then propagate to the `TokenProcessor` which can do its own optimized data-loading. --- CRM/Core/BAO/ActionSchedule.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CRM/Core/BAO/ActionSchedule.php b/CRM/Core/BAO/ActionSchedule.php index 5821fced0653..5b6fbf84cf38 100644 --- a/CRM/Core/BAO/ActionSchedule.php +++ b/CRM/Core/BAO/ActionSchedule.php @@ -278,6 +278,15 @@ public static function sendMailings($mappingID, $now) { $preferred_language = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Contact', $dao->contactID, 'preferred_language'); $row->context('locale', CRM_Core_BAO_ActionSchedule::pickLocale($actionSchedule->communication_language, $preferred_language)); } + + foreach ($dao->toArray() as $key => $value) { + if (preg_match('/^tokenContext_(.*)/', $key, $m)) { + if (!in_array($m[1], $tokenProcessor->context['schema'])) { + $tokenProcessor->context['schema'][] = $m[1]; + } + $row->context($m[1], $value); + } + } } $tokenProcessor->evaluate(); From 9ae8e0ffd7804b1e7f52b6955dbc954e1b57ea42 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Tue, 10 Aug 2021 18:13:48 -0700 Subject: [PATCH 4/4] CRM_Activity_Tokens - Simplify prefetch. Remove special-cases for `actionSearchResult`. Before: There are two distinct ways in which `CRM_Activity_Tokens` can be called, eg * For scheduled reminders, it's given `$row->context[actionSearchResult]`. The query is inspected for data. * For everything else, it's given `$row->context[activityId]`. The value is After: `CRM_Activity_Tokens` always receives activities as `$row->context[activityId]`. Comment: There are pre-existing tests for activity-based reminders and tokens in `CRM_Core_BAO_ActionScheduleTest` and `CRM_Activity_ActionMappingTest`. --- CRM/Activity/Tokens.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/CRM/Activity/Tokens.php b/CRM/Activity/Tokens.php index 66663dd7c497..bfcb4e76e5c0 100644 --- a/CRM/Activity/Tokens.php +++ b/CRM/Activity/Tokens.php @@ -84,6 +84,7 @@ public function alterActionScheduleQuery(\Civi\ActionSchedule\Event\MailingQuery // Multiple revisions of the activity. // Q: Could we simplify & move the extra AND clauses into `where(...)`? $e->query->param('casEntityJoinExpr', 'e.id = reminder.entity_id AND e.is_current_revision = 1 AND e.is_deleted = 0'); + $e->query->select('e.id AS tokenContext_' . $this->getEntityContextSchema()); } /** @@ -91,9 +92,7 @@ public function alterActionScheduleQuery(\Civi\ActionSchedule\Event\MailingQuery */ public function prefetch(TokenValueEvent $e) { // Find all the entity IDs - $entityIds - = $e->getTokenProcessor()->getContextValues('actionSearchResult', 'entityID') - + $e->getTokenProcessor()->getContextValues($this->getEntityContextSchema()); + $entityIds = $e->getTokenProcessor()->getContextValues($this->getEntityContextSchema()); if (!$entityIds) { return NULL; @@ -144,8 +143,7 @@ public function evaluateToken(TokenRow $row, $entity, $field, $prefetch = NULL) 'activity_id' => 'id', ]; - // Get ActivityID either from actionSearchResult (for scheduled reminders) if exists - $activityId = $row->context['actionSearchResult']->entityID ?? $row->context[$this->getEntityContextSchema()]; + $activityId = $row->context[$this->getEntityContextSchema()]; $activity = $prefetch['activity'][$activityId];