Mails resembling HTML are corrupted

Thanks @mxr576 however that patch is not the same as this issue.

The patch alters MessageViewBuilder.php in the contact module. The issue summary proposes to alter PhpMail::format().

Thanks. However it's more complex than that. The message body is an array, and each entry in the array is either MarkupInterface or a string. So something like this (not tested):

foreach ($message['body'] as &$part) {
  if ($part instanceof MarkupInterface) {
    $part = MailFormatHelper::htmlToText($part);
  }
}

Thanks. Please see MailInterface::format() for a very clear description of what this patch needs to do.

The conversion process consists of the following steps:
   * - If the output is HTML then convert any input line that is a string using
   *   \Drupal\Component\Utility\Html\Html::Escape().
   * - If the output is plain text then convert any input line that is markup
   *   using \Drupal\Core\Mail\MailFormatHelper::htmlToText().
   * - Join the input lines into a single string.
   * - Wrap long lines using \Drupal\Core\Mail\MailFormatHelper::wrapMail().

if(is_array($message['body'])) {

The loop must go before the call to implode. Then you don't need this if line.

This needs a test before it can be committed.

Thanks it is getting better each time. Please delete this line and the else branch.
Html::Escape($part);
The comment says to do this if the output is HTML. However PHPMail always generates plain text output.

Code looks good. I think you don't need this line now.
+use Drupal\Component\Utility\Html;

Next step is to write tests - back to "Needs work" for that.

The patch is good, but it needs a test. The test should fail with an "only tests" patch. The test can send the example text from the issue summary.

Add another case inside mail_html_test_mail(), something like this

function mail_html_test_mail($key, &$message, $params) {
  switch ($key) {
    case 'render_from_message_param':
      $message['body'][] = \Drupal::service('renderer')->renderPlain($params['message']);
      break;

    case 'plain_from_message_param':
      $message['body'][] = $params['message'];
      break;
  }
}

Add another test in MailTest.php like this

    $message = "Today I learnt Greek letters alpha&beta; they are hard to write.\nIn HTML, ampersand must be written as &amp;.\nI saw your house and <wow> it is great.\nIf a<b and b<c then a<c.";
    \Drupal::service('plugin.manager.mail')->mail('mail_html_test', 'plain_from_message_param', 'plain@example.com', $language_interface->getId(), ['message' => $message]);

Great thanks that's good progress. Here are my comments:

1. You added the new test to testRenderedElementsUseAbsolutePaths() but it's not really related. I suggest to make a new test, maybe like this:

public function testResembleHtml() {

2. In MailTest.php this code is not needed - we know that $message is a plain string.

+use Drupal\Component\Render\MarkupInterface;


+    // Convert into plain text and assert.
+    if ($message instanceof MarkupInterface) {
+      $message = MailFormatHelper::htmlToText($message);
+    }

3. I see that there is the same bug with the mail subject - please can we fix that here too? The fix is in MailManager::doMail(). Instead of this code

        if ($message['subject']) {
          $message['subject'] = PlainTextOutput::renderFromHtml($message['subject']);
        }

Do this:

        if ($message['subject'] && ($message['subject'] instanceof MarkupInterface)) {
          $message['subject'] = PlainTextOutput::renderFromHtml($message['subject']);
        }

Then add a test for the subject also. You can take one of the four sentences out of $message and put it in the subject instead.

Thanks to you both.

1.

--- a/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
+++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
@@ -99,6 +99,7 @@ public function testSendPersonalContactMessage() {
+    $mail['subject'] = PlainTextOutput::renderFromHtml($mail['subject']);

No I don't think we can do that - the test is correct, so it means that the code is now broken. The problem is that the contact module is passing markup in something that isn't MarkupInterface. It looks like the problem is in contact_mail(): the concatenation operator (.=) forces the result back to a string.

In contact.module line 142 instead of this line:

      $message['subject'] .= t('[@form] @subject', $variables, $options);

do this:

      $message['subject'] = t('[@form] @subject', $variables, $options);

2. In testResembleHtml(), maybe better to take the sentence that's now in $subject out of $message so it's like this:

$subject = "Today I learnt Greek letters alphaβ they are hard to write.";
$message = "In HTML, ampersand must be written as &amp;.\nI saw your house and <wow> it is great.\nIf a<b and b<c then a<c.";

Thanks, looks good. In contact.module I think we need to change all three cases: lines 142, 151 and 161. You are right, only one is needed to pass the automatic tests, but they all have the same bug.

Then the next step is to upload an "Only tests" patch: it has the changes to tests and nothing else. The automatic tests should fail for this one of course.

Thanks @Berdir

I don't think the subject can ever be HTML? Why would we bother to check for that or expect it to be Markup?

Importantly, there is all the code that realised it needed to pass in Markup to avoid corruption:-) For example see simplenews_token_replace_subject(). Without that code, this patch would be definitely non-BC, and almost impossible for contrib modules to write code that works accurately with multiple core versions. A second category is anyone who calls Token::replace() which outputs HTML (although annoyingly not as a MarkupInterface, which is a separate bug, #2580723: Fix token system confusion, with new function Token::replacePlain()).

People that are sending plain text mails might currently not pass in a proper Markup object and this change means that suddenly, their HTML will be full of HTML tags.

Could be, but the interface documentation is strongly in favour of this patch:

hook_mail() body:
The array may contain either strings or objects implementing \Drupal\Component\Render\MarkupInterface.

MailInterface::format():
The message body is received as an array of lines that are either strings or objects implementing \Drupal\Component\Render MarkupInterface

Furthermore, the popular swiftmailer module contains code exactly like this patch without any obvious sign of complaint. So perhaps there won't be as much broken contrib code as you fear??

Finally, I don't see how we can ever fix this bug without getting the people who are passing markup but not as MarkupInterface to change their code.

Thinking out loud, could we turn it around and instead somehow flag message bodies to explicitly be plain text?

Maybe, but it seems pretty tricky/messy. The body is an array and, at least in theory, each part could separately be markup or not. I guess the flag could be "please trust me and only call htmlToText() if I pass MarkupInterface". Then maybe we deprecate not passing the flag to give people a warning. And in D10 we deprecate the flag and everyone has to change their code again??

I propose that we should try to get comments from more key people. How about if we set this to RTBC with some appropriate issue tags and see what people say?

I am struggling to imagine plasible contrib code that will be hit by this issue - can anyone give an example?

To hit the bug you need a string containing markup but not implementing MarkupInterface. Generally core would not generate such a thing so the contrib code would have to make it - which suggest that the coder has some awareness that they are making such a thing. Then to see the bug, pass this string as the body of a mail message - expecting the markup to be converted to plain.

Why would anyone create such a string containing markup only to pass it to a function that they want to remove the markup??

Thanks Berdir

MHO, that documentation just documents that it is allowed to use strings or Markup objects, not what that specifically means.

OK here's a more specific statement, from the comment on MailInterface::format(). Does this one seem any more convincing?

The conversion process consists of the following steps:
- If the output is HTML then convert any input line that is a string using \Drupal\Component\Utility\Html\Html::Escape().
- If the output is plain text then convert any input line that is markup using \Drupal\Core\Mail\MailFormatHelper::htmlToText().
- Join the input lines into a single string.
- Wrap long lines using \Drupal\Core\Mail\MailFormatHelper::wrapMail().

This is much less of an issue with swiftmailer, because if people are using Swiftmailer then they likely want to send HTML mails anyway.

I believe the same problem exists if using swiftmailer. If some code creates markup in a plain string and passes it to swiftmailer then their mail will be corrupted. If it is an HTML mail, then attributes will be double-escaped. If it is a plain text mail, then markup will appear in the mail.

but going to set back to needs work to update the documentation and have a change record

Makes sense, will do.

I guess if #3165762: Add symfony/mailer into core seems like the direction and the key people are not comfortable with this patch then we can just ignore the bug in this issue and ensure that the replacement system doesn't have the same bug.

Change record done - back to RTBC as per #47.

I think yes, we should close it. The bug is fixed in the new Symfony Mailer @Mail plug-in. PhpMail still contains the bug, but I don't think it makes sense to fix that - it creates BC problems and the code will be removed when Symfony Mailer is adopted.