Problem

  • _drupal_wrap_mail_line() adds two spaces at the end of soft-wrapped lines, causing double spaces to appear at random positions in sentences in some e-mail clients; i.e.:
    $ php -r 'include "mail.inc"; $i = drupal_wrap_mail("_drupal_wrap_mail_line() adds two spaces at the end of soft-wrapped lines, causing double spaces to appear at random positions in sentences in some e-mail clients"); $o = preg_replace("/( +)\\n/", "$1", $i); var_dump($i, $o);'

    string(166) "_drupal_wrap_mail_line() adds two spaces at the end of soft-wrapped lines,
    causing double spaces to appear at random positions in sentences in some
    e-mail clients"

    string(164) "_drupal_wrap_mail_line() adds two spaces at the end of soft-wrapped lines,  causing double spaces to appear at random positions in sentences in some  e-mail clients"

Details

  • RFC 2646 defines the generation of format=flowed as follows:

    A soft line break is a SP CRLF sequence

    If the line ends in one or more spaces, the line is flowed. Otherwise it is fixed. Trailing spaces are part of the line's content, but the CRLF of a soft line break is not.

    If the line ends with one or more space characters, it is flowed.

Proposed solution

  1. Remove the second space. One space + CRLF is sufficient for flowed lines.
Files: 
CommentFileSizeAuthor
#9 drupal.mail-wrap-line.9.patch656 bytessun
PASSED: [[SimpleTest]]: [MySQL] 40,413 pass(es).
[ View ]
#2 drupal8.mail-wrap-line.2.patch1.38 KBsun
PASSED: [[SimpleTest]]: [MySQL] 58,471 pass(es).
[ View ]
drupal8.mail-wrap-line.0.patch653 bytessun
FAILED: [[SimpleTest]]: [MySQL] 58,021 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, drupal8.mail-wrap-line.0.patch, failed testing.

sun’s picture

Assigned:Unassigned» sun
Status:Needs work» Needs review
StatusFileSize
new1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 58,471 pass(es).
[ View ]

Adjusted the unit test accordingly.

sun’s picture

Simple enough. This change is complete and ready to be committed.

Will backport to D7 and D6 afterwards.

sun’s picture

Still looking good.

Tor Arne Thune’s picture

Status:Needs review» Reviewed & tested by the community

In line with RFC 2646 so there is no reason that I can think of that this could cause any havoc.

sun’s picture

#2: drupal8.mail-wrap-line.2.patch queued for re-testing.

sun’s picture

Title:_drupal_wrap_mail_line() causes double spaces in soft-wrapped format=flowed sentences» E-mails contain double spaces in soft-wrapped sentences

People may be scared by the issue title. Let's see whether a simplification helps.


Apparently not. Looks like the RTBC queue is very long.

alexpott’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed 4be4a98 and pushed to 8.x. Thanks!

This was introduced in #154218: Add HTML to text convertor for email (and wrap mails properly) what is interesting is that the second call to wordwrap uses the correct sot-wrapped line break.

  if (!$line_is_mime_header) {
    // Use soft-breaks only for purely quoted or unindented text.
    $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");

There is also the suggestion of an optimisation here as if $line_is_mime_header is FALSE we will be calling wordwrap twice which seems unnecessary.

sun’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new656 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,413 pass(es).
[ View ]

Backported to D7.

The unit test in D8 does not appear to exist in D7.

sun’s picture

Status:Needs review» Reviewed & tested by the community

Identical patch, so I do not realistically see what would be wrong with moving it to RTBC.

tstoeckler’s picture

RTBC++

The green patch verifies that this doesn't break existing tests, but I also checked that there are no tests that could be amended easily to add test coverage for this.

tstoeckler’s picture

Issue summary:View changes

Updated issue summary.

sun’s picture

Is anything blocking this?

David_Rothstein’s picture

Version:7.x-dev» 6.x-dev
Issue summary:View changes
Status:Reviewed & tested by the community» Patch (to be ported)

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/273c78d

Moving to Drupal 6 for possible backport.

maximpodorov’s picture

BTW, is it proper to use wordwrap(), strlen() and other byte level function for UTF-8 texts?

sun’s picture

Version:6.x-dev» 7.x-dev
Status:Patch (to be ported)» Fixed

Thanks! I'm not going to work on a backport to D6 anymore, since my original trigger and use-case has vanished. Thus closing as fixed.

Status:Fixed» Closed (fixed)

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

kenneth.venken’s picture

I think this patch is incorrect and that the issue is invalid.

Quoting documentation of drupal_html_to_text: https://api.drupal.org/api/drupal/includes!mail.inc/function/drupal_html...

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.

And RFC 3676 describing delsp=yes

If the line is flowed and DelSp is "yes", the trailing space immediately prior to the line's CRLF is logically deleted. If the DelSp parameter is "no" (or not specified, or set to an unrecognized value), the trailing space is not deleted

and

Messages generated with DelSp=yes and received by clients which are aware of Format=Flowed but are not aware of the DelSp parameter will have an extra space remaining after removal of soft line breaks.

So it is to be expected that some clients - those that don't know about the DelSp parameter - will show two spaces.
Now that the extra space isn't added, I'm getting mails where some words are stuck together. (In Gmail and Apple Mail)

maximpodorov’s picture

And my comment is also unanswered:
BTW, is it proper to use wordwrap(), strlen() and other byte level function for UTF-8 texts?