fix pluggable smtp/mail framework

Oh man, this is awesome.

Marked #28604: Separate mail backend as duplicate.

For tests, what are your thoughts on something like this? The test, unfortunately, isn't passing and I'm not too sure why....

The test now works.... Should DrupalSmtpInterface be renamed to DrupalMailInterface because sometimes you're not using SMTP to send mail?

The problem is that you're running a different instance, so you're using the wrong $sent_message. Making it static, and referencing the static variable makes us reference the correct value.

Umm, that's what I said after fixing it in both #4 and #7 :-P .......

The only thing I'm not too fond of in this patch, or the Drupal mail system in general, is the $module and $key mentality....

function drupal_send($module, $key) {
  static $instances = array();

  $id = $module . '_' . $key;
  $configuration = variable_get('mail_sending_system', array('default' => 'DrupalMailSend'));

  // Look for overrides for the default class, starting from the most specific
  // id, and falling back to the module name.
  if (isset($configuration[$id])) {
    $class = $configuration[$id];
  }
  elseif (isset($configuration[$module])) {
    $class = $configuration[$module];
  }
  else {
    $class = $configuration['default'];
  }

  if (empty($instances[$class])) {
    $interfaces = class_implements($class);
    if (isset($interfaces['DrupalMailSendingInterface'])) {
      $instances[$class] = new $class;

Although it might be out of scope for this patch, there must be a better way of using this $module and $key implementation. Also, it would be nice to have some documentation saying how to use the mail system through variable get and DrupalMailSendingInterface..... That's a much better name then DrupalSmtpInterface, by the way.

:) Note there's an "incorrecly" in there. Déjà vu.

Looked great! Tests pass, lots of documentation is available. Setting this to RTBC.... This patch simply adds a code doc description to the DrupalMailSend class.

We need this to properly implement mail catch-up in simpletest (the test framework currently try to send mail outsite... not that great). Raising to critical.

DamZ: I think testing the mail system itself is out of scope for this issue and can live nicely in a post-patch here: #276409: Tests needed mail.inc. This issue is just to make the mail framework pluggable.

Ah, cool. Thanks guys!

The function "drupal_send" feels too ambiguous. Does it send mail? http requests? kittens? :)

1. drupal_mail_send()->mail()
2. drupal_send_mail()->mail()
3. drupal_send_mail()->send()
4. drupal_mail()->send() - Conflicts with existing function name

Numbers 2 or 3 seem easiest to figure out what it's doing and follows the drupal_verb_noun syntax.

I think drupal_send()->mail() is good. Is there any chance this'll ever be used for non-email?

I suggest drupal_smtp->send().

It is important that all implementors of DrupalMailSendingInterface work in the same way so that callers know exactly how the mailer behaves regardless what implementation is used. PHP's mail() has some rather subtle features that are inherited by DrupalMailSend. These should be properly documented, so that alternative implementations can replicate these.

It should be documented, that mail() looks for Cc and Bcc headers and sends the mail to addresses in these headers too. Note that splitting a Cc header, e.g. "Lorem, ipsum" <foo@example.org>, bar@example.org, into separate addresses is not completely trivial, so perhaps it would be better to make the caller supply all recipients in a structured array, e.g.

array(
  'to' => 'foobar@example.com',
  'cc' => array(
    array('Lorem, ipsum', 'foo@example.org'),
    'bar@example.org'
  )
)

+   *   - to
+   *      The mail address or addresses where the message will be sent to. The
+   *      formatting of this string must comply with RFC 2822. Some examples are:
+   *       user@example.com
+   *       user@example.com, anotheruser@example.com
+   *       User <user@example.com>

According to the PHP manual page, the latter format is not supported on Windows.

+   *   - headers
+   *      Associative array containing all mail headers.

It should be documented whether this array contains all headers or only those not specified by other arguments (to, subject).

It should be documented how to specify the envelope sender, i.e. the address used in the MAIL FROM: SMTP command. This address is used for bounce messages. mail() does this using a fifth parameter (see #131737: Ensure that the Return-Path is set when sending mail on both Windows and non-Windows systems.). The PEAR Mail package (and possibly others?) uses the Return-Path header for this. Note that according to RFC 2821, section 4.4, "[a] message-originating SMTP system SHOULD NOT send a message that already contains a Return-path header field", so if this header is used, it should not actually be sent on the wire.

Note that I used module_invoke(variable_get('field_storage_module', 'field_sql_storage'), $op, $arg1, $arg2); in field module. That's like a Drupal-ish solution...

This is looking RTBC! Well done, Peter. So this is how you spend your theming seminar? Hahaha.... Crell just posted #363787: Plugins: Swappable subsystems for core, so I think we should use that when it goes in. The code changes won't be much from the current patch.

The mail interface seems to be modeled after how PHP's mail() works. I don't think mail() has a very clean interface. It gives the impression that the envelope sender and recipient(s) has to appear in the message headers, and that the subject header has some kind of special status. The requirement that implementations should be able to parse a "Doe, John" <jd@example.net>, foobar@example.org style address in order to get the recipient list seems like a needlessly complex requirement.

I don't expect the pluggable SMTP handler to be called directly by modules, so it doesn't have to be very "user friendly", i.e. the subject header might as well be submitted as part of the header array. Also, the requirement that implementations should accept both LF and CR LF seems like a convenience feature that should be moved to drupal_mail() instead of being duplicated in every implementation.

I suggest that the $message parameter should contain the following entries: envelope_sender (in simple format as accepted by valid_email_address()), envelope_recipients (array of simple addresses), headers (array), and body (string). The headers array should mention all headers, including To, From, Subject etc.

Subscribe.

FYI, #321771: Make drupal_mail_wrapper a hook. is trying to achieve the same goal but via a different solution. It might make sense to select the best approach and close that issue or this one.

+++ includes/mail.inc
@@ -140,43 +140,43 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
+   * More information in the <a href="http://php.net/manual/en/function.mail.php">

Exceeds 80 chars.

+++ includes/mail.inc
@@ -140,43 +140,43 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
+   * PHP function reference for mail()</a>. See drupal_mail() for information on
+   * how $message is composed.

Should use @see in both cases.

+++ includes/mail.inc
@@ -140,43 +140,43 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
+   *   - id
...
+   *   - to
...
+   *   - subject
...
+   *   - body
...
+   *   - headers

Wrong indentation.

Description should follow after a colon of the element property.

(and elsewhere)

+++ includes/mail.inc
@@ -140,43 +140,43 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
+   *      formatting of this string must comply with RFC 2822. Some examples are:
+   *       user@example.com
+   *       user@example.com, anotheruser@example.com
+   *       User <user@example.com>
+   *       User <user@example.com>, Another User <anotheruser@example.com>

Exceeds 80 chars.

Should use the regular list syntax to list stuff.

+++ includes/mail.inc
@@ -195,6 +195,142 @@ function drupal_mail_send($message) {
+   * @param  $message
...
+   * @param  $message

Double space.

+++ includes/mail.inc
@@ -195,6 +195,142 @@ function drupal_mail_send($message) {
+   *   An e-mail message. See drupal_mail() for information on how $message is composed.

Exceeds 80 chars.

+++ includes/mail.inc
@@ -195,6 +195,142 @@ function drupal_mail_send($message) {
+/**
+ * Returns an object that implements DrupalMailSendingInterface.
...
+ * 

Trailing white-space.

+++ includes/mail.inc
@@ -195,6 +195,142 @@ function drupal_mail_send($message) {
+ * name as the value for the key corresponding to the module name.  For example

Double space.

+++ modules/simpletest/tests/mail.test
@@ -0,0 +1,57 @@
+   * Implementation of setUp(). 

Trailing white-space.

15 days to code freeze. Better review yourself.

Moving to needs review so bot can take a crack at it.

What does the test bot thing of this one?

fwiw, i would be happy with a variable_get('mail_inc') solution like hx proposed. thats what we do in d7 for session.inc, cache.inc,

+++ includes/mail.inc	18 Aug 2009 21:23:56 -0000
@@ -61,10 +61,10 @@
  * @param $to
  *   The e-mail address or addresses where the message will be sent to. The
  *   formatting of this string must comply with RFC 2822. Some examples are:
- *    user@example.com
- *    user@example.com, anotheruser@example.com
- *    User <user@example.com>
- *    User <user@example.com>, Another User <anotheruser@example.com>
+ *   - user@example.com
+ *   - user@example.com, anotheruser@example.com
+ *   - User <user@example.com>
+ *   - User <user@example.com>, Another User <anotheruser@example.com>

I see that c960657's comments about structuring the data coming into drupal_mail() so they are more sensible are not taken into account in this patch. I talked to Peter about this and he pointed out that this existing patch just basically takes the mail API we have and makes it pluggable, rather than making it actually make _sense_. Doing that would be a separate issue. I think I agree with this, even though I so very desperately loathe our current mail handling API. ;P

+++ includes/mail.inc	18 Aug 2009 21:23:56 -0000
@@ -127,7 +127,7 @@
-    $message['result'] = drupal_mail_send($message);
+    $message['result'] = drupal_send($module, $key)->mail($message);

This looks a little bit weird. We bothered to abstract the name of this thing to "drupal_send" so it wasn't tied to mailing, but yet the function you call is called "mail()". That makes me wonder why we don't just leave the function the same name.

Except now I think I remember. It's because it's not actually sending anything, it's instantiating an MailSendingThingy object and calling its mail() method.

In that case, I think we should name this to drupal_mail_sending_system() because that would be way more obvious.

+++ includes/mail.inc	18 Aug 2009 21:23:56 -0000
@@ -140,43 +140,38 @@
+class DrupalMailSend implements DrupalMailSendingInterface {
+  /**
+   * Send an e-mail message, using Drupal variables and default settings.
+   * @see http://php.net/manual/en/function.mail.php the PHP function reference
+   *   for mail().
+   * @see drupal_mail() for information on how $message is composed.
+   *

There is no need to duplicate this PHPDoc in two places. Putting it in the interface is fine.

+++ includes/mail.inc	18 Aug 2009 21:23:56 -0000
@@ -195,6 +190,144 @@
+class DrupalTestMailSend implements DrupalMailSendingInterface {
...
+class DrupalDevNullMailSend implements DrupalMailSendingInterface {

These do not belong in core's includes/mail.inc. They belong in mail_test.module or similar.

(Peter pointed out, actually simpletest.module since they need to handle mail in several tests.)

+++ includes/mail.inc	18 Aug 2009 21:23:56 -0000
@@ -195,6 +190,144 @@
+ * is taken from the value corresponding to the 'default-system' key. To use
+ * a different system for all mail sent by one module, specify also a class
+ * name as the value for the key corresponding to the module name.  For example

Though I know what you mean, that sentence is a bit awkward. Could you slightly rephrase?

+++ includes/mail.inc	18 Aug 2009 21:23:56 -0000
@@ -195,6 +190,144 @@
+ * @code
+ * array(
+ *   'default-system' => 'DrupalMailSend',
+ *   'user' => 'DevelMailLog',
+ *   'contact_page_autoreply' => 'DrupalDevNullMailSend',
+ * );
+ * @endcode

I agree with Moshe and chx that this might be overkill, and it's fancier than what we do in any other variable replacement API in Drupal. OTOH, I can see this opening the door for some very interesting things, like sending event notification emails via SMS, but not user registration emails. This gives us additional flexibility without bloating the system too badly, so I support its inclusion.

+++ includes/mail.inc	18 Aug 2009 21:23:56 -0000
@@ -195,6 +190,144 @@
+/**
+ * An interface for pluggable mail back-ends.
+ */
+interface DrupalMailSendingInterface {

There's a lot of knowledge in this issue about use cases around this, and it'd be really nice to have more background as to why a module might implement this interface.

+++ modules/simpletest/tests/mail.test	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,57 @@
+<?php
+// $Id$
+
+/**
+ * Test the Drupal mailing system.
+ */
+class MailTestCase extends DrupalWebTestCase implements DrupalMailSendingInterface {

Missing a /** @file */ thing here.

+++ modules/simpletest/tests/mail.test	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,57 @@
+  /**
+   * Implementation of getInfo().
+   */
...
+  /**
+   * Implementation of setUp().
+   */

No PHPDoc here because they're inherited from the base class.

+++ modules/simpletest/tests/mail.test	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,57 @@
+    return array(
+      'name' => t('Mail system'),
+      'description' => t('Performs tests on the pluggable mailing framework.'),
+      'group' => t('System'),
+    );

No t()s around getInfo strings.

6 days to code freeze. Better review yourself.

The name of the class (DrupalMailSend) and the name of the interface (DrupalMailSendingInterface) are not 100% IMO. I'd prefer to see something like DefaultMailSystem and MailSystemInterface.

That said, what was wrong with a variable_set()/variable_get()? It feels like this solution is a bit over-engineered -- it didn't came across as an elegant solution.

Ok, looks like Dries's feedback was incorporated.

Committed to HEAD! :) Please document this addition in the module upgrade guide.

Looks to me like the wrong class name is being used in mail.inc

added some more doc fixes

Doh. :P Committed.

Back to needs work for docs.

I'm also not quite sure why an implementation of System module uses a filename of mail.sending.inc. That's not following our current standard. We can discuss this standard for D8, but for D7, we should get things consistent.

Attached patch renames the file. No other changes.

Let's get the file rename in, moving this out of criticals queue though.

Renamed in CVS. Thanks!

Still needs docs.

.

I am wondering if this patch doesn't have a fundamental flaw, take this situation. User install smtp module to get that function, which replaces 'default-system' => 'DefaultMailSystem' with 'default-system' => 'SmtpMailSystem' to get the smtp functionallity. Now the user also get the idea of installing HTML Mail module to have some fancy html capabilities, which then set 'default-system' => 'HTMLMailMailSystem' to get access to and change the format functionality, but at the same time it switches Drupal back to use the default mail() function for delivery.

The problem is that the 2 modules need to manipulate 2 different areas of the same mail frame work, so one literally speak kicks the other out. I don't have any ready solution to this, just a reflexion, but obviously this opens up for lots of incompatibilities among mail modules. One thought springing though is that modules like HTML shouldn't create it's own class by implementing the interface but rather extending the base class. Likewise, a module like smtp could also offer html/text to Html Mail to set. I wonder if not some kind of abstraction is needed here so modules like HTML Mail asks for or extends the same class, no matter if it's Drupals default system or smtp providing it. Just thinking laud...

Thanks. #80 summarizes exactly the reasons why PHP OOP is bad and harmful for Drupal on a concrete example. OOP in PHP is not modular. Any attempt to make it modular requires loads of abstractions to work around PHP's limitations. In turn, abstractions lead to loss of context, as well as over-engineered APIs, which, in turn, require expert PHP and OOP skills to work with. Effectively, entirely contradicting Drupal's design goals.

changing tag to new scheme for update page

See related:

• #1135262: drupal_mail() should support using different MailSystemInterface classes for format() and mail()

• #1089956: Provide Administrative UI and Developers API for safely setting the mail_system variable.

Looking at the patch that was committed (we DESPERATELY need issue summaries)... I'm trying to figure out what the API change is that needs to be documented in the 6/7 module update guide.

What I'm seeing:

a) drupal_mail_send($message) ==> drupal_mail_sending_system($module, $key)->mail($message)

b) The new drupal_mail_sending_system() function uses some new classes/interfaces to do its work.

c) The variable governing what to use to send mail changed from variable_set('smtp_library') to variable_set('mail_sending_system').

d) The function drupal_mail_wrapper() went away, but this is a 7.x->7.x change (the function didn't exist in 6.x), so I'm not going to worry about it. There were also some simpletest changes, but simpletest was not core in d6, so we don't have to document them either.

SIGH. Actually, it looks like this changed after this patch was committed (no idea what issue that might have been on, as there's nothing else relevant tagged "needs documentation" or "needs update documentation". So I've made an attempt to document the actual changes at:
http://drupal.org/update/modules/6/7#mail-sending

Marking fixed for now, but if someone would like to review, that would be cool.

... variable_set('mail_sending_system').

Correction: variable_set('mail_system', array($key => $classname)). But to avoid overwriting existing settings, you need something like the following:

$mail_system = variable_get('mail_system', array('default-system' => 'DefaultMailSystem'));
$mail_system = array_diff_key($mail_system, array($key => $classname));
$mail_system += array($key => $classname);
variable_set('mail_system', $mail_system);

Then to clear just your module's setting on hook_disable(), you need:

$mail_system = variable_get('mail_system', array('default-system' => 'DefaultMailSystem'));
$mail_system = array_diff_key($mail_system, array($key => $classname));
if ($key == 'default-system') {
  $mail_system['default-system'] = 'DefaultMailSystem';
}
variable_set('mail_system', $mail_system);

That's why I wrote and advocate using the Mail System module instead of setting that variable directly.

Instead of the above code blocks, simply call mailsystem_set(array($key => $classname)); and mailsystem_clear(array($key => $classname)); respectively.

RE #85 - did you look at what I actually wrote on the 6/7 module update guide page? I think it's correct there...

RE #85 - did you look at what I actually wrote on the 6/7 module update guide page? I think it's correct there...

Correct but incomplete.

RE #85 - I don't think I want to put all of that into the 6/7 module upgrade page. I think the information that is there is enough to get someone (a) to this issue and (b) to the necessary documentation on api.drupal.org. Or is there a doc page on drupal.org that I should link to?

Here's the only doc page I could find; it's better than nothing: http://drupal.org/node/900794

OK, that's related to http://drupal.org/node/224333#email-html , which already has a link to that page.

So I think we're OK here... Really, the point of the update doc for *this* issue is to document that the drupal_send_mail function and variable_get('smtp_library', '') are gone, and to point people towards learning more. I think there are enough pointers.