Remove use of drupal_html_to_text() from system mails

Right. As explained at #298708: drupal_html_to_text() removes line endings, drupal_html_to_text() requires that you pass it HTML (duh), not plain text or rich text. In the case of core actions, if you configure the "Send e-mail" action, you just get a text area to enter plain text (with a few tokens). Since we pass this text field without any check_markup() directly as the body of the message, we're basically handling it as plain text. So, we can't call drupal_html_to_text() since there's no HTML, and therefore all the whitespace in the message gets collapsed.

This same patch applies (with offsets) to DRUPAL-6, too.

Looks valid to me. :)

Note: the alternative would be to run the text area from the core email action through check_markup() and keep the drupal_html_to_text() in place, but that text area doesn't currently have a notion of a text format, etc.

This issue is tagged for drupal 7. What do we need to do to get it applied to Drupal 6? Do we need to open a separate issue?

We need to get it committed to HEAD, and then we can set the version back to 6.x-dev and get it committed to D6. The patch applies cleanly and passes all tests. Dries says he was happy with the approach. If anyone else wants to review and set to RTBC, that'd help speed this up (by convention, we're not supposed to mark our own patches RTBC)...

Hm. The side-effect of this is we would no longer using the drupal_html_to_text function anywhere in core. It also seems like this might break things for people who were using RTEs to edit their mails, or for those who were sending node bodies or similar out through this function.

Would it be possible to do some sort of checking to see if the incoming $body is HTML and either pass it along in plaintext or via HTML if required?

Also in #298708-2: drupal_html_to_text() removes line endings Dries recommended we write some tests for this. I assume this is why he didn't commit this back in #3?

Probably needs a re-roll.

I agree with this patch as a first measure. Still needs tests.

Separate issue: Add an argument/property to mail messages to let the entire system know that it's either dealing with HTML or plain-text. Most probably, there is already an issue floating around for this though.

Here's a rerolled version of this patch, a starting point at least

Tagging for feature freeze.

I discussed this issue quite intensively with smk-ka in the past hour.

- http://api.drupal.org/api/function/system_mail/7 is the only hook_mail() implementation that invokes drupal_html_to_text(), and that has a reason.

- The reason is that system_mail() is only invoked for the "Send e-mail" action in http://api.drupal.org/api/function/system_send_email_action/7

- The "Send e-mail" action is a configurable action, which allows you to enter plain-text combined with tokens, which may generate HTML or not.

- By default, Drupal's default mail format is plain-text, so Drupal core has to assume that any HTML in a (configurable, token-replaced) message needs to be converted into plain-text.

- A module having the purpose of HTML-enabling some or all e-mails will alter the edit form where the e-mail text (template) is entered, so the user is able to enter HTML. Such a module has to process the body for all or certain/configured e-mails to either auto-convert text to HTML, or to apply a custom text format or sanitisation to a body that already contains HTML.

- Currently, the problem is that such a module is only invoked after system_mail() generated the message. Thus, even when the original body was supposed to contain HTML, it will be converted to plain-text.

- Hence, what we need is a way to inform a hook_mail() implementation (like system_mail()) whether it shall or shall not apply drupal_html_to_text() to the message text.

This is, what we call hook_mail_prepare(). :)

Another approach: why not deciding that all mail bodies (send to drupal_mail(), by one route or another) are HTML, and do the drupal_html_to_text() in the mailer implementation itself?

@Damien: The hook_mail() implementation as well as hook_mail_alter() implementations that generate the message need to know whether it has to convert and sanitize the message. While an all-or-nothing approach would certainly work, too, it would remove flexibility.

This time with the required documentation.

In addition, $message['html'] has another purpose:

- $message['html'] = FALSE indicates that the original message does not contain HTML. A HTML-enabling module can simply perform a text-to-HTML conversion for all these messages, because it can presume that anything in the message is plain-text.

- $message['html'] = TRUE indicates that the original message potentially contains HTML. A HTML-enabling module will have to perform additional steps to convert and potentially sanitize such messages.

ok, discussed Damien's suggestion once again with smk-ka:

• We can't attach the message format to the MailSystemInterface, because that interface is only dealing with the mail client. Even when using PHP's mail() function, you can send HTML mails, if you properly process them.
• True is that we could convert mail messages "globally" (the entire message, not just one body array chunk) to plain-text or HTML. But that cannot happen in the MailSystemInterface, and it can't happen in hook_mail_alter(), because hook_mail_alter() is supposed to add data to the message, just like hook_mail() adds to the message initially. And if it doesn't happen in hook_mail(), then we would need a new facility for it.
• Actually, the facility already exists: it is http://api.drupal.org/api/function/drupal_wrap_mail/7 - which is entirely bound to plain-text messages, but is invoked for all messages currently.
• Hence, if we want to follow Damien's suggestion, then we would make drupal_wrap_mail() pluggable - in the very same way and pattern as the MailSystemInterface. That approach would also make sense to me.

So we either go with the existing patch - or we add a new MailFormatInterface and convert the existing drupal_wrap_mail() into DefaultTextMailFormat.

In the meantime I'd actually prefer the latter. :)

ok, this is my first OOP patch. So who always wanted to call me stupid? 8)

Better one.

We can't attach the message format to the MailSystemInterface, because that interface is only dealing with the mail client. Even when using PHP's mail() function, you can send HTML mails, if you properly process them.

This is just a naming issue. In Drupal 6, the drupal_mail_wrapper() function was used for more thing then just choosing the mail sending interface. MimeMail, for example, used it to wrap the Mail in a Mime envelope.

So either:

• we rename MailSystemInterface to accept the fact that there is more then just sending the mail about it
• we add an additional, explicit, MailPrepareInterface stage, and dumb down MailSystemInterface

@Damien: That's more or less what the new patch is doing! :)

The past implementation, however, is not a good comparison. We need to differentiate between mail sending and mail preparation/formatting. PHP's mail() is able to send HTML mails as long as you prepare the mail properly. So the mail sending engine is detached from the mail formatting engine.

The new patch accounts for this, and like you proposed, would also allow for a single implementation to provide both formatting and sending in a single class:

- MailSystemInterface is a pluggable system for mail sending engines, extending MailInterface.

- MailFormatInterface is a pluggable system for formatting messages, extending MailInterface.

- Both can be assigned separately to individual message keys (like the new, existing MailSystemInterface we have).

- A single implementation would be able to plug into both, i.e. provide formatting and sending in one class. (PHPMailer module might be a possible example for that)

- By default, Drupal core now assumes that all messages may contain HTML and invokes drupal_html_to_text() in the DefaultTextMailFormat implementation, which could be re-used by other implementations.

This one should pass.

The full 248 Site-wide contact form tests pass for me. Re-test.

At the very least the naming for this new parameter makes this very confusing.

This usage is very non-intuitive:

drupal_mail_class('system')->mail()

I'm imagining something more like the DB layer might be appropriate like:

drupal_mail_sending_system()->format('html_to_text')->mail()

@pwolanin: I'm not sure I understand your proposal. The problem with that is that both ->format() AND ->mail() need to be pluggable and the caller should not decide which formatting or which mail engine to apply. Instead we want to assign an implementation for both to each message that goes through drupal_mail().

Of course, what we can do is to introduce helper functions for the arbitrator, so you get the "more intuitive" functions back:

function drupal_mail_sending_system($module, $key) {
  return drupal_mail_class('system', $module, $key);
}

function drupal_mail_formatter($module, $key) {
  return drupal_mail_class('format', $module, $key);
}

Why should the caller not decide?

@pwolanin: This entire issue is about the fact that Drupal core decides about the format of the message before it is handed off to anyone. There is no way to prevent drupal_html_to_text() from running, and there is absolutely no way to prevent drupal_wrap_mail() from running. What you introduced with the MailSystemInterface was a first baby step to get to a pluggable system -- however, the real nightmare for people who want to send mails other than plain-text lives in aforementioned issues.

It took some crazy in-depth discussions to understand the real problem of that html-to-text conversion in system_mail(). Based on that initial draft, Damien proposed to make it pluggable for real (instead of a static $message option), stopping core from making wild assumptions on the format "we" want to send and just treat it like pure HTML -- always.

That's the only way to pass on the original content to implementations that really want to send HTML (and not forcing them into a html-to-text-to-html conversion, which is what we currently do). The latest patch accounts for all of that.

Though I can see the issue needs fixing this solution seems overly complex to me. Do we really need all these classes and interfaces?

Can't we just add (either of them or both):

- Add a format() function to the MailSystemInterface, so we can implement and override it
- Add a 'format' string or a 'formatter' callback to the $mail array that can be altered by modules (and defaults to drupal_wrap_mail()

?

@Jose: I had my very own reasons to start dealing with this patch (PHPMailer) for which the solution you propose would probably be sufficient. However, I specifically took highly flexible implementations like Messaging module into account. Your module allows messages to be sent via arbitrary engines in an arbitrary format. Even if a message may end up as private message within the same system, you still want to allow to configure whether it's sent as raw HTML or converted into plain-text. But to accomplish this flexibility, modules have to implement Messaging's API instead of Drupal core's API. The new MailSystemInterface in core already abstracts the mail engine, but still takes away the decision that the message has to be plain-text. This patch basically adds the flexibility of Messaging to core. Hence, all modules can simply use core's API, and Messaging can plug into that to add the flexibility.

Your proposal of a $message['formatter callback'] would make sense to me if hook_mail_alter() wasn't supposed to let modules add or alter the message itself. To provide some context to the design of other APIs: We first build, then alter, then render, then output a thing. We do not define or presume the output format during building or altering, because that is up to the renderer. And, of course, we do not have a prototype for the output engine at all, because everything else we render ends up as HTML (AJAX/JSON would count, if we wouldn't build completely different data for it). Hence, instead of presuming and limiting the format Drupal can send messages in, we want to defer that decision to the rendering engine - which is entirely decoupled from the actual sending engine, because the sending engine is supposed to just send; just like PHP's native mail() function could send much more than we currently pass to it.

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

The only chance to get this in is to agree to a way forward *now*, so please speak up, re-roll this patch, and help to make it RTBC.

Re-rolled against HEAD.

This one should pass again.

Damien additionally requested to rename "system" to "send", so we get the nice pair of "format" and "send".

oh boy... something is broken with the testbots... it's always the same test that sometimes fails and sometimes not. Can't replicate this locally at all.

ugh - this makes no sense to me.

+  if ($type == 'format') {
+    $configuration = variable_get('mail_formatter', array('default-class' => 'DefaultMailFormat'));
+  }
+  elseif ($type == 'send') {
+    $configuration = variable_get('mail_sending_system', array('default-class' => 'DefaultMailSystem'));
+  }
+  else {
+    throw new Exception(t('Mail interface %interface does not exist.', array('%interface' => 'Mail' . ucfirst($type) . 'Interface')));
+  }

Thanks! Improved that exception message.

sorry - but this is introducing a total DX WTF imho

@pwolanin: Wow. I'm trying to help with your issues, but what you're doing here is totally not constructive. Would you mind to elaborate a bit more on what you don't like and how you see ways to improve it?

I had the chance to discuss this a bit more with pwolanin in IRC.

To get his approval as well, we will just enhance the existing MailSystemInterface with a new format() method.

I really hope this one is good to go.

An extended class is always dependent on a single base class, that is, multiple inheritance is not supported.

PHP crash course. ;)

Even more backtalk on the return values of the methods.

Renaming drupal_mail_sending_system() only to drupal_mail_system(). Nicely maps to MailSystemInterface and DefaultMailSystem.

I'm liking this much better, though not sure if we should leave all the key doxygen in the hook definition.

Well, I just wanted to improve, and can certainly live without that. Point is, that's one of the major problems in our API documentation: if more than one doxygen documents the same thing, we have a 100% probability that all others will be outdated very soon. There's no way out of that unless we point to a single point of failure. Since hook_mail_alter() is the most public function of all here, the documentation contained in there is, apparently, the most recent and most correct one, because almost no one on this earth really touches the internals of the mail sub-system.

Let's defer that decision to core maintainers... I think I incorporated all of your points now?

.

This one should pass again.

Looks good - need to test locally that mail actually sends.

tested locally.

I can't help but think that the approach taken in this patch is a little awkward. It effectively forces people to create special classes with special names and implementations. I'd expect this to be a configuration issue, rather than a programming task. On the flip side, it is very flexible/powerful, fixes a real shortcoming in the system, and is probably needed for some.

Your basically summarized the idea: By default, Drupal core, like now, will still presume all messages to be sent as plain-text. This, however, is a big enabler for contrib modules like Messaging module, which provides a very granular interface to configure the format and mailing engine for every single message $id in the system.

Hummm.... this has been committed, but apparently we forgot to update the issue.

Tagging appropriately.

I don't see any commit message for this - are you sure it went in?

Strange, yeah. http://api.drupal.org/api/function/drupal_mail/7 shows the new code, so perhaps it was mixed into another commit...

Can this be set back to Drupal 6.x or does it still need work in HEAD?

It still needs documentation in the module upgrade guide. Since I'm not sure when I'll have time to work on the documentation for all the issues I tackled and that will take some time, I'm unassigning myself for now.

Note that this patch cannot be back-ported, and the way I see it, there is no way to achieve a similar fix for D6 (for the reasons outlined in the analysis above).

Added docs to http://drupal.org/node/727352

Just a note, Dries mixed up his commit messages, and this patch actually went in as commit 275520.

Cross-reference: The following issue fixes the bug that this one introduced:

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

The page where the docs where added (http://drupal.org/node/727352) gives me an access denied page.

The page got closed down and everything in it was supposedly moved to the main upgrade page. I think the one for this issue is http://drupal.org/update/modules/6/7#email-html ?