From 999d9be579acf3e340992ced0fc9dbf32fcb7bcb Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Fri, 22 Sep 2023 13:14:29 +1200 Subject: [PATCH] Update code creating queue entries to save mailing_id This covers all the places I found except the forward action I will tackle that separately --- CRM/Mailing/BAO/Mailing.php | 10 ++-- CRM/Mailing/BAO/MailingJob.php | 53 ++++++++++--------- CRM/Mailing/Event/BAO/MailingEventQueue.php | 29 ++++++++++ CRM/Utils/SQL/Insert.php | 22 +++++++- ext/flexmailer/src/Listener/BasicHeaders.php | 2 +- .../phpunit/api/v3/JobProcessMailingTest.php | 6 +-- 6 files changed, 87 insertions(+), 35 deletions(-) diff --git a/CRM/Mailing/BAO/Mailing.php b/CRM/Mailing/BAO/Mailing.php index 82e4010d09..5d984633c5 100644 --- a/CRM/Mailing/BAO/Mailing.php +++ b/CRM/Mailing/BAO/Mailing.php @@ -816,10 +816,11 @@ class CRM_Mailing_BAO_Mailing extends CRM_Mailing_DAO_Mailing implements \Civi\C * * @param array $testParams * Contains form values. + * @param int $mailingID * - * @return void + * @throws \Civi\Core\Exception\DBQueryException */ - public function getTestRecipients($testParams) { + public static function getTestRecipients(array $testParams, int $mailingID): void { if (!empty($testParams['test_group']) && array_key_exists($testParams['test_group'], CRM_Core_PseudoConstant::group())) { $contacts = civicrm_api('contact', 'get', [ 'version' => 3, @@ -852,6 +853,8 @@ ORDER BY civicrm_email.is_bulkmail DESC 'job_id' => $testParams['job_id'], 'email_id' => $dao->email_id, 'contact_id' => $groupContact, + 'mailing_id' => $mailingID, + 'is_test' => TRUE, ]; CRM_Mailing_Event_BAO_MailingEventQueue::create($params); } @@ -908,7 +911,7 @@ ORDER BY civicrm_email.is_bulkmail DESC $fields = []; $fields[] = 'Message-ID'; // CRM-17754 check if Resent-Message-id is set also if not add it in when re-laying reply email - if ($prefix == 'r') { + if ($prefix === 'r') { $fields[] = 'Resent-Message-ID'; } foreach ($fields as $field) { @@ -916,7 +919,6 @@ ORDER BY civicrm_email.is_bulkmail DESC $headers[$field] = '<' . implode($config->verpSeparator, [ $localpart . $prefix, - $job_id, $event_queue_id, $hash, ] diff --git a/CRM/Mailing/BAO/MailingJob.php b/CRM/Mailing/BAO/MailingJob.php index ce90b95a67..ed18b5ca37 100644 --- a/CRM/Mailing/BAO/MailingJob.php +++ b/CRM/Mailing/BAO/MailingJob.php @@ -407,23 +407,17 @@ VALUES (%1, %2, %3, %4, %5, %6, %7) } /** - * @param array $testParams + * @param ?array $testParams */ - public function queue($testParams = NULL) { - $mailing = new CRM_Mailing_BAO_Mailing(); - $mailing->id = $this->mailing_id; + public function queue(?array $testParams = NULL) { if (!empty($testParams)) { - $mailing->getTestRecipients($testParams); + CRM_Mailing_BAO_Mailing::getTestRecipients($testParams, (int) $this->mailing_id); } else { // We are still getting all the recipients from the parent job // so we don't mess with the include/exclude logic. $recipients = CRM_Mailing_BAO_MailingRecipients::mailingQuery($this->mailing_id, $this->job_offset, $this->job_limit); - // FIXME: this is not very smart, we should move this to one DB call - // INSERT INTO ... SELECT FROM .. - // the thing we need to figure out is how to generate the hash automatically - $now = time(); $params = []; $count = 0; // dev/core#1768 Get the mail sync interval. @@ -434,31 +428,40 @@ VALUES (%1, %2, %3, %4, %5, %6, %7) if (empty($recipients->email_id) && empty($recipients->phone_id)) { continue; } - - if ($recipients->phone_id) { - $recipients->email_id = "null"; - } - else { - $recipients->phone_id = "null"; - } - $params[] = [ - $this->id, - $recipients->email_id, - $recipients->contact_id, - $recipients->phone_id, + 'job_id' => $this->id, + 'email_id' => $recipients->email_id ? (int) $recipients->email_id : NULL, + 'phone_id' => $recipients->phone_id ? (int) $recipients->phone_id : NULL, + 'contact_id' => $recipients->contact_id ? (int) $recipients->contact_id : NULL, + 'mailing_id' => (int) $this->mailing_id, + 'is_test' => !empty($testParams), ]; $count++; - // dev/core#1768 Mail sync interval is now configurable. - if ($count % $mail_sync_interval == 0) { - CRM_Mailing_Event_BAO_MailingEventQueue::bulkCreate($params, $now); + /* + The mail sync interval is used here to determine how + many rows to insert in each insert statement. + The discussion & name of the setting implies that the intent of the + setting is the frequency with which the mailing tables are updated + with information about actions taken on the mailings (ie if you send + an email & quickly update the delivered table that impacts information + availability. + + However, here it is used to manage the size of each individual + insert statement. It is unclear why as the trade offs are out of sync + ie. you want you insert statements here to be 'big, but not so big they + stall out' but in the delivery context it's a trade off between + information availability & performance. + https://github.com/civicrm/civicrm-core/pull/17367 */ + + if ($count % $mail_sync_interval === 0) { + CRM_Mailing_Event_BAO_MailingEventQueue::writeRecords($params); $count = 0; $params = []; } } if (!empty($params)) { - CRM_Mailing_Event_BAO_MailingEventQueue::bulkCreate($params, $now); + CRM_Mailing_Event_BAO_MailingEventQueue::writeRecords($params); } } } diff --git a/CRM/Mailing/Event/BAO/MailingEventQueue.php b/CRM/Mailing/Event/BAO/MailingEventQueue.php index 056e159db3..4ca0bee954 100644 --- a/CRM/Mailing/Event/BAO/MailingEventQueue.php +++ b/CRM/Mailing/Event/BAO/MailingEventQueue.php @@ -287,10 +287,39 @@ SELECT DISTINCT(civicrm_mailing_event_queue.contact_id) as contact_id, } /** + * Bulk save multiple records. + * + * For performance reasons hooks are not called here. + * + * @param array[] $records + * + * @return array + */ + public static function writeRecords(array $records): array { + $rows = []; + foreach ($records as $record) { + $record['hash'] = self::hash(); + $rows[] = $record; + if (count($rows) >= CRM_Core_DAO::BULK_INSERT_COUNT) { + CRM_Utils_SQL_Insert::into('civicrm_mailing_event_queue')->rows($rows)->execute(); + $rows = []; + } + } + if ($rows) { + CRM_Utils_SQL_Insert::into('civicrm_mailing_event_queue')->rows($rows)->execute(); + } + // No point returning a big array but the standard function signature is to return an array + // records + return []; + } + + /** + * @deprecated * @param array $params * @param null $now */ public static function bulkCreate($params, $now = NULL) { + CRM_Core_Error::deprecatedFunctionWarning('writeRecords'); if (!$now) { $now = time(); } diff --git a/CRM/Utils/SQL/Insert.php b/CRM/Utils/SQL/Insert.php index 0a3c509866..dc256fc95c 100644 --- a/CRM/Utils/SQL/Insert.php +++ b/CRM/Utils/SQL/Insert.php @@ -145,7 +145,12 @@ class CRM_Utils_SQL_Insert { $escapedRow = []; foreach ($this->columns as $column) { - $escapedRow[$column] = $this->escapeString($row[$column]); + if (is_bool($row[$column])) { + $escapedRow[$column] = (int) $row[$column]; + } + else { + $escapedRow[$column] = $this->escapeString($row[$column]); + } } $this->rows[] = $escapedRow; @@ -186,4 +191,19 @@ class CRM_Utils_SQL_Insert { return $sql; } + /** + * Execute the query. + * + * @param bool $i18nRewrite + * If the system has multilingual features, should the field/table + * names be rewritten? + * @return CRM_Core_DAO + * @see CRM_Core_DAO::executeQuery + * @see CRM_Core_I18n_Schema::rewriteQuery + */ + public function execute($i18nRewrite = TRUE) { + return CRM_Core_DAO::executeQuery($this->toSQL(), [], TRUE, NULL, + FALSE, $i18nRewrite); + } + } diff --git a/ext/flexmailer/src/Listener/BasicHeaders.php b/ext/flexmailer/src/Listener/BasicHeaders.php index 025a165e4e..79baa28917 100644 --- a/ext/flexmailer/src/Listener/BasicHeaders.php +++ b/ext/flexmailer/src/Listener/BasicHeaders.php @@ -39,7 +39,7 @@ class BasicHeaders extends BaseListener { $mailParams = array(); $mailParams['List-Unsubscribe'] = ""; - \CRM_Mailing_BAO_Mailing::addMessageIdHeader($mailParams, 'm', $e->getJob()->id, $task->getEventQueueId(), $task->getHash()); + \CRM_Mailing_BAO_Mailing::addMessageIdHeader($mailParams, 'm', NULL, $task->getEventQueueId(), $task->getHash()); $mailParams['Precedence'] = 'bulk'; $mailParams['job_id'] = $e->getJob()->id; diff --git a/tests/phpunit/api/v3/JobProcessMailingTest.php b/tests/phpunit/api/v3/JobProcessMailingTest.php index 197d0408f5..7014fc18a8 100644 --- a/tests/phpunit/api/v3/JobProcessMailingTest.php +++ b/tests/phpunit/api/v3/JobProcessMailingTest.php @@ -100,8 +100,6 @@ class api_v3_JobProcessMailingTest extends CiviUnitTestCase { /** * Test that a contact deleted after the mailing is queued is not emailed. - * - * @throws \CRM_Core_Exception */ public function testDeletedRecipient(): void { $this->createContactsInGroup(2, $this->_groupID); @@ -260,7 +258,7 @@ class api_v3_JobProcessMailingTest extends CiviUnitTestCase { $this->_mut->assertRecipients($this->getRecipients(1, 2)); } - public function concurrencyExamples() { + public function concurrencyExamples(): array { $es = []; // Launch 3 workers, but mailerJobsMax limits us to 1 worker. @@ -489,7 +487,7 @@ class api_v3_JobProcessMailingTest extends CiviUnitTestCase { * * @throws \CRM_Core_Exception */ - public function testBatchActivityTargets($isBulk) { + public function testBatchActivityTargets($isBulk): void { $loggedInUserId = $this->createLoggedInUser(); \Civi::settings()->set('mailerBatchLimit', 2); -- 2.25.1