From b2423c39e732d17d15127f959f4bb756f1551797 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Fri, 7 Feb 2020 15:15:14 -0800 Subject: [PATCH] (REF) Move CIVICRM_MAIL_LOG logic from patch-files to wrapper-class Overview -------- This aims to improve maintainablity and deployment workflows by reduing the scope of the needed 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. 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" ] ``` --- CRM/Utils/Mail.php | 4 ++ CRM/Utils/Mail/LoggingMailer.php | 67 +++++++++++++++++++ .../composer/patches/pear-mail.patch.txt | 53 --------------- 3 files changed, 71 insertions(+), 53 deletions(-) create mode 100644 CRM/Utils/Mail/LoggingMailer.php diff --git a/CRM/Utils/Mail.php b/CRM/Utils/Mail.php index b9aa6ef617..c6a982c587 100644 --- a/CRM/Utils/Mail.php +++ b/CRM/Utils/Mail.php @@ -123,6 +123,10 @@ class CRM_Utils_Mail { } else { $mailer = Mail::factory($driver, $params); + // Previously, CiviCRM bundled patches to change the behavior of these three classes. Use a decorator to avoid patching. + if ($mailer instanceof Mail_smtp || $mailer instanceof Mail_mail || $mailer instanceof Mail_sendmail) { + $mailer = new CRM_Utils_Mail_LoggingMailer($mailer); + } } CRM_Utils_Hook::alterMailer($mailer, $driver, $params); return $mailer; diff --git a/CRM/Utils/Mail/LoggingMailer.php b/CRM/Utils/Mail/LoggingMailer.php new file mode 100644 index 0000000000..3f7b59b899 --- /dev/null +++ b/CRM/Utils/Mail/LoggingMailer.php @@ -0,0 +1,67 @@ +delegate = $delegate; + } + + public function send($recipients, $headers, $body) { + if (defined('CIVICRM_MAIL_LOG')) { + CRM_Utils_Mail::logger($recipients, $headers, $body); + if (!defined('CIVICRM_MAIL_LOG_AND_SEND') && !defined('CIVICRM_MAIL_LOG_AND SEND')) { + return TRUE; + } + } + + if (!is_array($headers)) { + return PEAR::raiseError('$headers must be an array'); + } + + return $this->delegate->send($recipients, $headers, $body); + } + + public function &__get($name) { + return $this->delegate->{$name}; + } + + public function __set($name, $value) { + return $this->delegate->{$name} = $value; + } + + public function __isset($name) { + return isset($this->delegate->{$name}); + } + + public function __unset($name) { + unset($this->delegate->{$name}); + } + +} diff --git a/tools/scripts/composer/patches/pear-mail.patch.txt b/tools/scripts/composer/patches/pear-mail.patch.txt index e2f488dd33..23fa638c9d 100644 --- a/tools/scripts/composer/patches/pear-mail.patch.txt +++ b/tools/scripts/composer/patches/pear-mail.patch.txt @@ -17,21 +17,6 @@ diff --git a/Mail/mail.php b/Mail/mail.php index ee1ecef..ae6e2e8 100644 --- a/Mail/mail.php +++ b/Mail/mail.php -@@ -114,6 +114,14 @@ class Mail_mail extends Mail { - */ - public function send($recipients, $headers, $body) - { -+ if (defined('CIVICRM_MAIL_LOG')) { -+ CRM_Utils_Mail::logger($recipients, $headers, $body); -+ // Note: "CIVICRM_MAIL_LOG_AND SEND" (space not underscore) was a typo that existed for some years, so kept here for compatibility, but it should not be used. -+ if (!defined('CIVICRM_MAIL_LOG_AND_SEND') && !defined('CIVICRM_MAIL_LOG_AND SEND')) { -+ return true; -+ } -+ } -+ - if (!is_array($headers)) { - return PEAR::raiseError('$headers must be an array'); - } @@ -145,7 +153,12 @@ class Mail_mail extends Mail { if (is_a($headerElements, 'PEAR_Error')) { return $headerElements; @@ -46,41 +31,3 @@ index ee1ecef..ae6e2e8 100644 // We only use mail()'s optional fifth parameter if the additional // parameters have been provided and we're not running in safe mode. -diff --git a/Mail/sendmail.php b/Mail/sendmail.php -index 7e8f804..e0300a0 100644 ---- a/Mail/sendmail.php -+++ b/Mail/sendmail.php -@@ -132,6 +132,14 @@ class Mail_sendmail extends Mail { - */ - public function send($recipients, $headers, $body) - { -+ if (defined('CIVICRM_MAIL_LOG')) { -+ CRM_Utils_Mail::logger($recipients, $headers, $body); -+ // Note: "CIVICRM_MAIL_LOG_AND SEND" (space not underscore) was a typo that existed for some years, so kept here for compatibility, but it should not be used. -+ if (!defined('CIVICRM_MAIL_LOG_AND_SEND') && !defined('CIVICRM_MAIL_LOG_AND SEND')) { -+ return true; -+ } -+ } -+ - if (!is_array($headers)) { - return PEAR::raiseError('$headers must be an array'); - } -diff --git a/Mail/smtp.php b/Mail/smtp.php -index 5e698fe..5f057e2 100644 ---- a/Mail/smtp.php -+++ b/Mail/smtp.php -@@ -255,6 +255,14 @@ class Mail_smtp extends Mail { - */ - public function send($recipients, $headers, $body) - { -+ if (defined('CIVICRM_MAIL_LOG')) { -+ CRM_Utils_Mail::logger($recipients, $headers, $body); -+ // Note: "CIVICRM_MAIL_LOG_AND SEND" (space not underscore) was a typo that existed for some years, so kept here for compatibility, but it should not be used. -+ if (!defined('CIVICRM_MAIL_LOG_AND_SEND') && !defined('CIVICRM_MAIL_LOG_AND SEND')) { -+ return true; -+ } -+ } -+ - $result = $this->send_or_fail($recipients, $headers, $body); - - /* If persistent connections are disabled, destroy our SMTP object. */ -- 2.25.1