(REF) Move CIVICRM_MAIL_LOG logic from patch-files to wrapper-class

[totten]

Overview

This aims to improve maintainablity and deployment workflows by reduing the
scope of the needed patch-files.

It is an alternative approach for #16481.

Before

civicrm-core included patch files for pear/mail. These files modified
the behavior of Mail_sendmail, Mail_smtp, and Mail_mail to ensure that
the send() function would abide by the CIVICRM_MAIL_LOG constant.

After

civicrm-core includes a wrapper class LoggingMailer. It takes an
instance of Mail_sendmail, Mail_smtp, or Mail_mail and mixes-in the
CIVICRM_MAIL_LOG logic. The resulting object is still an instance of Mail.

Technical Details

There is a hook_civicrm_alterMailer, for which consumers could potentially
see a change in behavior (because the concrete-class of $mailer may
change and support different properties). Mitigating factors:

• There is only one public method in the Mail contract: send().
• The only example of alterMailer in the documentation suggests
  that you wholly replace the object with your own implementation.
  This was the original purpose. If you use it this way, then it doesn't
  matter what's happening inside the old object.
  https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_alterMailer/
• In public universe, there is only one extension (Cividesk's Sparkpost)
  which uses this hook. And it does swap in that expected way.
• Just in case... the LoggingMailer passes-through all calls to read/write properties.
  You can see this in action in a specific configuration, e.g.

  ## Before patch
  $ cv ev '$m=Civi::service("pear_mail"); return [get_class($m), $m->host];'
  [
      "Mail_smtp",
      "127.0.0.1"
  ]
  
  ## After patch
  $ cv ev '$m=Civi::service("pear_mail"); return [get_class($m), $m->host];'
  [
      "CRM_Utils_Mail_LoggingMailer",
      "127.0.0.1"
  ]
  

Comments

Ping @seamuslee001

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

@totten this test failure api_v3_JobProcessMailingTest.testConcurrency looks like it might relate, I like the idea of this

[totten]

jenkins, test this please

Yeah, at a first glance, it does seem plausible. I tried running the test locally (both in isolation and in the full suite), and it didn't reproduce. It could also be coincidence in a test that's a little time-sensitive.

Consider _createMailer() circa line 122 - the tests for simulated mail-blast are supposed to write to a log-table instead of sending messages, which means they're supposed to go through the CRM_Mailing_BAO_Spool path (without using Mail_smtp or Mail_sendmail or Mail_mail).

Fingers crossed 🤞 for another run to give better result.

[seamuslee001]

Test fail is unrelated

[seamuslee001]

@totten I did an r-run of this by clearing out my local vendor folder and reinstalling packages and clearing cache then submitting the SMTP settings to send a test email. I had to make one additional patch to make it work. However once done it seemed to work correctly

diff --git a/CRM/Admin/Form/Setting/Smtp.php b/CRM/Admin/Form/Setting/Smtp.php
index 87d30761cf..a3590b3ca2 100644
--- a/CRM/Admin/Form/Setting/Smtp.php
+++ b/CRM/Admin/Form/Setting/Smtp.php
@@ -165,7 +165,7 @@ class CRM_Admin_Form_Setting_Smtp extends CRM_Admin_Form_Setting {
           'Subject' => $subject,
         ];

-        $mailer = Mail::factory($mailerName, $params);
+        $mailer = CRM_Utils_Mail::_createMailer($mailerName, $params);

         $errorScope = CRM_Core_TemporaryErrorScope::ignoreException();
         $result = $mailer->send($toEmail, $headers, $message);

[seamuslee001]

@totten i think we also will need to modify this https://github.com/civicrm/civicrm-core/pull/16497/files#diff-6c43a3f8cc4e99bd04af92421c03179eR275

[totten]

Good points @seamuslee001 ! Some follow-ups:

• Code updates:
  • For the first point (CRM/Admin/Form/Setting/Smtp.php), I added the fix for Administer => System => Outbound Mail UI.
  • For the second point (quirk-handling for CC/BCC via CRM_Utils_Mail::send()), I made some changes to the wrapper class. The newer FilteredPearMailer allows a similar conditional ($mailer->getDriver() === 'mail') so that the quirk-handling doesn't have to change much. (It also might allow future cleanup by extracting the quirk-handler to a separate function, but I didn't want the scope-of-work/review to expand even further, so I kept the patch minimal and left a TODO.)
• Testing:
  • I ran with mailcatcher and manually checked each permutation of:
    • (1a) Send email via "View Contact" screen and (1b) Send email via Administer => System => Outbound Mail UI
    • (2a) With SMTP settings, (2b) With CIVICRM_MAIL_LOG pointed to a file, and (2c) With CIVICRM_MAIL_LOG and CIVICRM_MAIL_LOG_AND_SEND.
  • Added a small unit-test for the FilteredPearMail utility

[seamuslee001]

Changes look good to me adding merge on pass

[yashodha]

test this please

[totten]

There were a large number of failures like this...

CRM_Event_Form_ParticipantTest::testParticipantOfflineReceipt with data set #1 (',')
Invalid argument supplied for foreach()

/home/jenkins/bknix-dfl/build/core-16497-6bzhv/web/sites/all/modules/civicrm/CRM/Utils/Mail/FilteredPearMailer.php:61
/home/jenkins/bknix-dfl/build/core-16497-6bzhv/web/sites/all/modules/civicrm/CRM/Utils/Mail.php:299

... because the filters list is empty (NULL) in headless environment.

It should be empty ([]). Patched/squashed/pushed.