DefaultMailSystem implements MailSystemInterface::format() incorrectly

Created on 20 February 2012, over 12 years ago
Updated 31 May 2024, 27 days ago

The current implementation is:

public function format(array $message) {
  // Join the body array into one string.
  $message['body'] = implode("\n\n", $message['body']);
  // Convert any HTML to plain-text.
  $message['body'] = drupal_html_to_text($message['body']);
  // Wrap the mail body for sending.
  $message['body'] = drupal_wrap_mail($message['body']);
  return $message;
}

However, according to the description of drupal_html_to_text(), the call to drupal_wrap_mail() is unnecessary and should be removed since it converts all soft breaks into hard breaks, making format=flowed meaningless.

🐛 Bug report
Status

Needs review

Version

7.0 ⚰️

Component
Mail 

Last updated about 8 hours ago

No maintainer
Created by

🇰🇷South Korea C. Lee

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇮🇹Italy apaderno Brescia, 🇮🇹
    -    // Convert any HTML to plain-text.
    +    // Convert any HTML to plain-text and wrap the mail body for sending.
         $message['body'] = drupal_html_to_text($message['body']);
    -    // Wrap the mail body for sending.
    -    $message['body'] = drupal_wrap_mail($message['body']);
         return $message;
    

    Since the call to drupal_wrap_mail() is removed, the comment cannot says and wrap the mail body for sending, as that is done by drupal_wrap_mail().

    -  $line = wordwrap($line, 77 - $values['length'], $values['soft'] ? " \n" : "\n");
    +  $line = wordwrap($line, 77 - $values['length'], $values['soft'] ? "  \n" : "\n");
       // Break really long words at the maximum width allowed.
       $line = wordwrap($line, 996 - $values['length'], $values['soft'] ? " \n" : "\n", TRUE);
    

    The changed line uses " \n" (two spaces and a new line) instead of " \n" (a space and a new line), but the other line uses " \n" (a space and a new line). I am not sure this is a typo or a inconsistent change.

  • Status changed to Needs review 2 months ago
  • 🇺🇸United States mfb San Francisco

    Since the call to drupal_wrap_mail() is removed, the comment cannot says and wrap the mail body for sending, as that is done by drupal_wrap_mail().

    drupal_html_to_text() wraps the mail for sending. As the function documentation states: "The output will be suitable for use as 'format=flowed; delsp=yes' text (RFC 3676) and can be passed directly to drupal_mail() for sending." Within drupal_html_to_text(), drupal_wrap_mail() will be called on each chunk.

    The changed line uses " \n" (two spaces and a new line) instead of " \n" (a space and a new line), but the other call to wordwrap() uses " \n" (a space and a new line). I am not sure this is a typo or a inconsistent change.

    The trailing space will be deleted when format=flowed "delsp" email is formatted for display. When wrapping at a space, you want to preserve the space, so you end up with two spaces at the end of the line. For extremely long words, there should be no space when the word is reassembled for display, so you have just one space at the end of the line.

    With this change, I am simply reverting 273c78ded2005cac80e2fa965f9131a85be16f0b, so this code goes back to it's original logic in #154218: Add HTML to text convertor for email (and wrap mails properly) (mostly, there was apparently an additional bug fix in there meanwhile)

    If this seems too unclear, I could add additional verbiage to the code comments?

  • Status changed to RTBC 2 months ago
  • 🇮🇹Italy apaderno Brescia, 🇮🇹

    I apologize: That comment is correct because drupal_html_to_text() calls drupal_wrap_mail(). This issue should be sufficient to document that calling drupal_wrap_mail() after drupal_html_to_text() is not necessary.

    With what explained in comment #14, I think this issue is R&TBC.

  • 🇮🇳India Pravesh_Poonia

    Applied Patch, Issue is looking fixed now

  • Status changed to Needs review 28 days ago
  • 🇸🇰Slovakia poker10

    Thanks for working on this. I reviewed the patch and compared it with the D10. I am curious, if there is an issue for this for D10? Because it seems like the code in D10 is the same as the D7 code:

    https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

    https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

    Do we need to change this in D10 first?

    Thanks!

  • 🇺🇸United States mfb San Francisco

    Filed 🐛 Fix the format=flowed; delsp=yes encoding of email messages Active to fix this on Drupal 11

Production build 0.69.0 2024