dev/core#85 On some servers mail() fails when 'Cc' or 'Bcc' headers are defined but empty

[mattwire]

Overview

On some servers the PHP mail() function fails to send email if empty 'Cc' or 'Bcc' headers are set.

dev/core#85

Before

Mail cannot be sent, PHP mail() returns false (no error message).

After

Mail is sent, PHP mail() sends.

Technical Details

'Cc' and 'Bcc' headers are optional. Most servers will just ignore them if empty but some seem to fail if they are present but have no email addresses. So we unset them if empty.

[eileenmcnaughton]

@mattwire how can make this as specific as possible to keep the risk as low as possible

I see in the class this ominous block (which @twomice was last to touch)

    // CRM-3795, CRM-7355, CRM-7557, CRM-9058, CRM-9887, CRM-12883, CRM-19173 and others ...
    // The PEAR library requires different parameters based on the mailer used:
    // * Mail_mail requires the Cc/Bcc recipients listed ONLY in the $headers variable
    // * All other mailers require that all be recipients be listed in the $to array AND that
    //   the Bcc must not be present in $header as otherwise it will be shown to all recipients
    // ref: https://pear.php.net/bugs/bug.php?id=8047, full thread and answer [2011-04-19 20:48 UTC]
    if (get_class($mailer) != "Mail_mail") {
      // get emails from headers, since these are
      // combination of name and email addresses.
      if (!empty($headers['Cc'])) {
        $to[] = CRM_Utils_Array::value('Cc', $headers);
      }
      if (!empty($headers['Bcc'])) {
        $to[] = CRM_Utils_Array::value('Bcc', $headers);
        unset($headers['Bcc']);
      }
    }

I think that is the same mailer & we could possibly do this within that block - which has the added benefit we KNOW empty headers are OK for mail_Mail

[monishdeb]

@mattwire how about moving this code up i.e. point where it is been initialised

- $headers['Cc'] = CRM_Utils_Array::value('cc', $params);
- $headers['Bcc'] = CRM_Utils_Array::value('bcc', $params);

+ foreach (['Cc', 'Bcc'] as $fieldPart) {
+  if (!empty($params[$fieldPart])) {
+    $headers[$fieldPart] = $params[$fieldPart];
+  }
+ }

and then we no longer this code

[mattwire]

@monishdeb That makes sense. The only issue, which @eileenmcnaughton has also mentioned, is how we make this low risk as most servers are happy with the empty headers, but clearly some are not! Could we end up in a situation where we apply this patch and find that some servers actually require the empty headers? :-(

[eileenmcnaughton]

right most conservative possible plse

[monishdeb]

@eileenmcnaughton @mattwire I think it's safe to not set these empty attributes to mail headers. Here's more evidence https://www.drupal.org/project/mimemail/issues/960374 https://stackoverflow.com/questions/44884390/laravel-mail-send-even-if-cc-and-bcc-is-null sendgrid/sendgrid-nodejs#263 where they follow the same logic

[eileenmcnaughton]

@monishdeb I think it' safe too - but it still scares me

[eileenmcnaughton]

Also noting some confirmation in the wild - https://issues.civicrm.org/jira/browse/CRM-21801

[mattwire]

@eileenmcnaughton @monishdeb I've updated this PR with the changes suggested by @monishdeb Will post to dev list for feedback.

[nganivet]

shouldn't this be:

if (!empty($params[strtolower($optionalHeader]))) {

since it is $params['cc'] lower case in the lines that you are replacing?

I also do not like moving away from the CRM_Utils_Array functions.

How about doing instead:

foreach (['Cc', 'Bcc'] as $optionalHeader) {
$headers[$optionalHeader] = CRM_Utils_Array::value(strtolower($optionalHeader), $params);
if (empty($headers[$optionalHeader])) {
unset($headers[$optionalHeader])
}
}

[eileenmcnaughton]

@nganivet do you have any opinion about the safety of the fix?

[nganivet]

Right now the fix is non-functional, so needs to be corrected first. Then it would depend on the downstream library's expectations and behavior: does it require $headers['cc'] to be set, even if empty? is it creating the CC header in the outgoing email if the $headers['cc'] is not set? I have not investigated these questions.

[eileenmcnaughton]

OK @nganivet those questions are the key ones we need to answer. This patch has changed a few times since created & I agree the current iteration needs to do a strtolower comparison - but we really need to answer the bigger question - ie. we know some mail servers fail on an empty cc / bcc but we don't know if any servers REQUIRE cc / bcc to be set EVEN IF EMPTY

[mattwire]

@nganivet Thanks for the feedback, I cleaned the patch up and as you spotted it no longer worked because of the lowercase issue. I've now amended it with changes as you suggested.

[monishdeb]

I agree with the final patch and also finds the logic to be safe about not setting cc and bcc attribute. Hence approving this PR

[eileenmcnaughton]

Per @monishdeb comments in gitlab - this seems fairly compelling https://www.drupal.org/project/mimemail/issues/960374
Merging