PhpMail : broken mail headers in PHP 8.0+ because of LF characters

Error: Call to undefined function Drupal\Core\Mail\Plugin\Mail\mail() in Drupal\Core\Mail\Plugin\Mail\PhpMail->mail() (line 120 of core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php).

I applied the solution in #40, no change, the error persists.

PHP 8.1
Drupal 9.5.3

I assume this issue is still present in Drupal 10 (since it's very similar to Drupal 9.5), but I haven't actually tested Drupal 10 yet. Has anyone confirmed yet whether this is still an issue?

I believe so... My drupal 10 webform handler is set to use html, but in my emails I see all the html tags.

Patch in #40 works for me using mime_mail but with htmlmail it's the same as without the patch. Not clear why.

Drupal core 9.5.5
Mime Mail 8.x-1.0-alpha5
HTML Mail 8.x-3.0-alpha3

Here's another patch, which addresses the concern of breaking the current behaviour for PHP < 8 raised by @alexpott in #34. Since Drupal 10 doesn't support PHP versions lower than 8, the check can be removed again in the 10.x branch.

I've tested the patch on a Drupal 9.5.8 site with PHP 8.1 on Debian and it works: the webform module is able to send HTML mails again.

It was also brought up that this needs a unit test, but I could use some help here, as I'm not really a PHP developer anymore these days. When I look at \Drupal\Tests\Core\Mail\Plugin\Mail\PhpMailTest.php, I see how the doMail() method was mocked and I could probably verify the arguments passed to the mock somehow, but since the code now checks the constant PHP_VERSION to determine how to behave, this makes unit testing hard or impossible. Is there some way of injecting the PHP version as a dependency of the PhpMail class? It would seem weird to me write a unit test that makes different assertions based on which PHP version the test runs on, right? If anyone could give me some pointers here, I'd be happy to give it another try. (For Drupal 10, a test would be more straighforward.)

Patch in #40 works for me using mime_mail but with htmlmail it's the same as without the patch. Not clear why.

As a co-maintainer of HTML Mail, I don't think this observation should be a reason to block or postpone the current issue. The HTML Mail module will not be supported going forward because it relies on a very old and unsupported PECL package. HTML Mail is being abandoned in favor of Mime Mail, and Mime Mail *relies* on this current issue being fixed in core.

Thanks all for the work on this so far, here's a first go at a test.

And again, slightly more verbose :p

Thanks!

The last submitted patch, 52: 3270647-52-d9-tests-only.patch, failed testing. View results →

Just wanted to add:

• The test doesn't verify the Reply-To because the behavior varies based on sendmail_path which is a PHP_INI_SYSTEM config, and I'm not sure the best way to test that, and I think it's out of scope. (Whereas just adding checks for the other static values seems fine imho.)
• I don't know for sure that the subject is correct, I just took what's currently being output. I did double-check that the CRLF + space combo makes sense, and it looks like it might be legit (not enough of an expert to say for sure!):
  

  If it is desirable to encode more text than will fit in an 'encoded-word' of 75 characters, multiple 'encoded-word's (separated by CRLF SPACE) may be used.

  http://www.faqs.org/rfcs/rfc2047.html

• Is it worth a follow-up issue to expand the tests further for the shell escaping and OS/PHP ini dependent behavior? (Not sure how to tackle the sendmail_path stuff.)

Thanks!

Not sure the expanding test coverage is needed as this fix is only needed for 9.5.

I don't see how this is only needed for 9.5.x:

    // Note: email uses CRLF for line-endings. PHP's API requires LF
    // on Unix and CRLF on Windows. Drupal automatically guesses the
    // line-ending format appropriate for your system. If you need to
    // override this, adjust $settings['mail_line_endings'] in settings.php.
    $mail_body = preg_replace('@\r?\n@', $line_endings, $message['body']);
    // For headers, PHP's API suggests that we use CRLF normally,
    // but some MTAs incorrectly replace LF with CRLF. See #234403.
    $mail_headers = str_replace("\r\n", "\n", $headers->toString());
    $mail_subject = str_replace("\r\n", "\n", $mail_subject);

Doesn't some of this code need to be removed now we only support PHP 8+? I what #32 does?

We should get a correct patch into 10.x first.

> I don't see how this is only needed for 9.5.x:

It's not. The fix in the patch is also very relevant for Drupal 10+. The backwards compatibility for PHP < 8 is only relevant for Drupal 9.5 (which is still supported, I think), though. Since Drupal 10 doesnt support PHP 7. That concerns these lines:

    if (version_compare(PHP_VERSION, '8.0.0') < 0) {
      // For headers, PHP's API suggests that we use CRLF normally,
      // but some MTAs incorrectly replace LF with CRLF. See #234403.
      // PHP 8+ requires headers to be separated by CRLF,
      // so we'll replace CRLF by LF only when using PHP < 8. See:
      // - https://bugs.php.net/bug.php?id=81158
      // - https://github.com/php/php-src/commit/6983ae751cd301886c966b84367fc7aaa1273b2d#diff-c6922cd89f6f75912eb377833ca1eddb7dd41de088be821024b8a0e340fed3df
      $mail_headers = str_replace("\r\n", "\n", $mail_headers);
      $mail_subject = str_replace("\r\n", "\n", $mail_subject);
    }

In Drupal 10 and onwards, this particular block can be removed, but the rest of the patch is still needed.

@P44T it's needs work because issues need to go into Drupal 10 before they can go into Drupal 9, i.e. we should commit a version like "in Drupal 10 and onwards, this particular block can be removed, but the rest of the patch is still needed." which I don't think is on this issue yet and certainly not with a recent test run.

@catch, thank your for clarifying. I've added new patch files:

• 3270647-59-d10.patch: based against 10.0.x, including the block for Drupal 9.5.
• 3270647-59-d10-only.patch: minimal patch based against 10.0.x, without backward compatiblity for Drupal 9.5.
  • 3270647-59-d9.patch: based against 9.5.x.

    Please let me know if there's anything else I can do!

Patches have been updated to address coding standard issue.

Got my wires crossed but just to reiterate/confirm we want

D9 = 3270647-52-d9.patch from #52
D10/11.x = 3270647-60-d10-only.patch from #60

Committed/pushed the patches from #60 and 59 to 11.x/10.1.x/10.0.x/9.5.x. Did my best with issue credits but a lot of people on this issue so apologies for any ommisions. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.

Unfortunate this was closed as what ended up in 9.5 does not work. The solution from #20 does work for us.

Hello,
has it not been fixed on v9.5.10 → - Issue #3270647 ???