simplenews_html_to_text() strips off newlines

See #299138: Improve \Drupal\Core\Utility\Mail::htmlToText(). I think we all have these issues...
If somebody finds a way to have it working, I'd eventually be open to commit it, although it's not contrib task to "fix" core.

subscribe

I'm actually closing this one, which is clearly a duplicate of the "core" one. Please subscribe to the other one, so everybody can follow the same thread.

As far as I can see after a brief look over the code, there will be a problem regardless of whether the core bug is fixed or not...

simplenews_mail (in simplenews.module) calls simplenews_build_node_mail which calls simplenews_html_to_text (in simplenews.mail.inc) which calls drupal_html_to_text (in mail.inc). The email is then sent using drupal_mail (in mail.inc) which calls drupal_html_to_text again.

The fact that drupal_html_to_text is called twice creates a problem, because the first call strips out all the HTML and the second call screws up the line breaks (drupal_html_to_text requires HTML, as mentioned in the initial report).

So, if simplenews_html_to_text were to return just $text rather than running it through drupal_html_to_text (and the function were renamed accordingly) then we'd be left with only the problems from the core bug, but as it stands there will be a problem regardless of whether the core bug is fixed or not.

Removing the drupal_html_to_text call from the end of simplenews_html_to_text and patching mail.inc with the core patch in http://drupal.org/node/299138 seems to fix the problem (ignoring problems that the core patch doesn't fix _yet_).

When is drupal_mail() calling drupal_html_to_text() ?

drupal_mail() doesn't call it directly, but the default mail system calls it in its format() function which is called by drupal_mail(). See lines 144 - 148 in includes/mail.inc for calls to mail system and line 26 in modules/system/system.mail.inc for call to drupal_html_to_text().

Here's a workaround until the following issue is resolved:

#299138: Improve \Drupal\Core\Utility\Mail::htmlToText()

Thanks.

The attached patch is identical to yours except I've removed your call to htmlspecialchars() from simplenews_text() as it should return plain text rather than HTML.

Emails now look a lot better, but not quite right - I'm investigating some odd spacing with lists.

Crosslinked from #299138-149: Improve \Drupal\Core\Utility\Mail::htmlToText()

If you do make changes to the added html_to_text.inc file, please submit those changes back to the above issue; thanks.

Will do, though that's not necessary with this...

With the last patch, indented content has incorrect spacing when on Drupal 7.2. This patch fixes this by incorporating the latest drupal_wrap_mail() function as simplenews_wrap_mail(). Tests have also been added. Both changes based on code from #299138-148: Improve \Drupal\Core\Utility\Mail::htmlToText().

Edit: Well I shouldn't really say "the latest drupal_wrap_mail()" - it's the latest in #299138-148: Improve \Drupal\Core\Utility\Mail::htmlToText() as of now.

Edit 30/05/2011: Use the patch in comment#13 instead of this one

Here's that patch again with the correct function name this time (simplenews_wrap_mail() as opposed to simpletext_wrap_mail())! As before it's for Drupal 7.2.

I've noticed an oddity where preg_replace() behaves differently on different servers, probably due to the use of /m, the PCRE_MULTILINE modifier, so I'll be looking into that via #299138: Improve \Drupal\Core\Utility\Mail::htmlToText() and will update this issue when appropriate.

Note that there's a new patch in #299138-155: Improve \Drupal\Core\Utility\Mail::htmlToText()

Updated #13 to track recent changes to #299138-157: Improve \Drupal\Core\Utility\Mail::htmlToText()

Same patch with _drupal_html_to_text() call corrected to _simplenews_html_to_text() for Drupal 7.2.

I've found that the use of the PCRE_MULTILINE modifier m in preg_replace() resulted in different behaviour on different servers, possibly due to the difference in the PCRE version though I can't be sure at this stage.

Server 1 (tests pass):

php > $s = "one\ntwo\n";
php > echo preg_replace('/^/m','FOO',$s);
FOOone
FOOtwo

Server 2 (tests fail):

php > $s = "one\ntwo\n";
php > echo preg_replace('/^/m','FOO',$s);
FOOone
FOOtwo
FOO

Note how on server 2 (which has an older PCRE library), the replacement is also made on the trailing new line.

I've added a three line workaround to the _simplenews_wrap_mail() function, so tests are now green on both servers. The attached patch is the same as the last but with the workaround.

Attached is the same except with the simpler solution by pillarsdotnet in core issue http://drupal.org/node/299138#comment-4544368

@stephandale: You beat me to it! :-)

Bump to trigger testbot and see if this still applies.

Updated the patch with the following changes:

- moved tests into existing tests/simplenews.test file instead of creating a new one
- Updated the call to simplenews_text()
- renamed html_to_text.inc to includes/simplenews.html_to_text.inc to be more in line with everything else.

This should allow to test bot to pick up the new tests. Just to see if it works..

I haven't actually reviewed the patch yet.

Question:

It looks like that fun html_to_text.inc is part of mailsystem.module as well. Instead of adding it yet another time, can't we call it from mailsystem.module if it's enabled? Then we actually don't need all that code/tests. Something like

if (module_exists('mailsystem')) {
  // call stuff from mailsystem
}

inside simplenews_text().

I'm also open to discuss to depend on mailsystem completely, especially if we can get support for the mail theme setting in it, see #1382036: Add a common way to configure the theme that will render the email and/or other advantages of doing so. After all, mimemail now does, so most probably have it anyway.

Erm, and now with patch.