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
- Remove the second space. One space + CRLF is sufficient for flowed lines.
Comment | File | Size | Author |
---|---|---|---|
#19 | fix-mail-wrapping-2078917-19.patch | 1.35 KB | mfb |
#9 | drupal.mail-wrap-line.9.patch | 656 bytes | sun |
#2 | drupal8.mail-wrap-line.2.patch | 1.38 KB | sun |
drupal8.mail-wrap-line.0.patch | 653 bytes | sun | |
Comments
Comment #2
sunAdjusted the unit test accordingly.
Comment #3
sunSimple enough. This change is complete and ready to be committed.
Will backport to D7 and D6 afterwards.
Comment #4
sunStill looking good.
Comment #5
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedIn line with RFC 2646 so there is no reason that I can think of that this could cause any havoc.
Comment #6
sun#2: drupal8.mail-wrap-line.2.patch queued for re-testing.
Comment #7
sunPeople may be scared by the issue title. Let's see whether a simplification helps.
—
Apparently not. Looks like the RTBC queue is very long.
Comment #8
alexpottCommitted 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.
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.
Comment #9
sunBackported to D7.
The unit test in D8 does not appear to exist in D7.
Comment #10
sunIdentical patch, so I do not realistically see what would be wrong with moving it to RTBC.
Comment #11
tstoecklerRTBC++
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.
Comment #11.0
tstoecklerUpdated issue summary.
Comment #12
sunIs anything blocking this?
Comment #13
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/273c78d
Moving to Drupal 6 for possible backport.
Comment #14
maximpodorov CreditAttribution: maximpodorov commentedBTW, is it proper to use wordwrap(), strlen() and other byte level function for UTF-8 texts?
Comment #15
sunThanks! 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.
Comment #17
kenneth.venken CreditAttribution: kenneth.venken commentedI 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...
And RFC 3676 describing delsp=yes
and
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)
Comment #18
maximpodorov CreditAttribution: maximpodorov commentedAnd my comment is also unanswered:
BTW, is it proper to use wordwrap(), strlen() and other byte level function for UTF-8 texts?
Comment #19
mfbYes I'd agree this patch should be reverted... Here's a patch to fix the format=flowed delsp encoding in case anyone's interested.
I also posted this patch @ #1447236: DefaultMailSystem implements MailSystemInterface::format() incorrectly