drupal_mail, allow hook_mail_alter implementation to cancel mail

I agree with this! Faced this issue today when aiming to prevent Register Welcome emails being sent and found there is no easy way, had to use this send to nowhere option in hook_mail_alter which is not at all elegant. Definitely I think an "alter" option for email should be to not send that email at all. If there is a reason why it cannot be done this way (too much power for an alter function) then how about a new function such as hook_mail_nosend() ?

The idea and principle is good, but the key 'send' sounds a bit poorly named.

how about we rephrase the key as 'cancel' and reverse the logic?

Re-rolled according to suggestions from @sun and @larowlan.

Patch looks good but I think we need tests as well as updates to the documentation. There is an extensive example in the documentation for drupal_mail and also for the hook_mail_alter function in system.api.php.
We need to update the documentation so those writing hook_mail_alter implementations know that they can set the cancel key.

Not sure about the backport to D6/D7 - is it too late for that?
Tagging appropriately for the tests/documentation.

Added tests and documentation as requested.

+++ b/includes/mail.inc
@@ -57,6 +57,9 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
+ *         if (example_user_notices_optout($data['user'])) {
+ *           break;
+ *         }

Not sure what this does?

Powered by Dreditor.

Oops. Corrected the example code and added comments.

Looks good to me.

d7 and d6 backports.

The d6 patch is necessarily in two parts, one against code and one against documentation.

Thanks for adding docs and tests. And - ouch - given that, only now it's apparent that the function argument to drupal_mail() is already named "send"... although it still sounds a bit fuzzy (within the context of a mail sending function), it's better to keep the argument and property identically named.

Additionally, as a side-note, a property with negated meaning and logic ("cancel") should always ring alarm bells - never do that; it almost always leads to implementation and/or understanding problems in the long run. In some other places, Drupal core still uses properties with negated meaning (e.g., "hidden") -- almost no one understands their meaning, and even if you understand them, it's extremely weird to code against these values.

Furthermore:

+++ b/includes/mail.inc
@@ -57,6 +57,11 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
+ *         // If the recipient has opted out of such notices, cancel sending.
+ *         if (example_user_notices_optout($data['user'])) {
+ *           $message['cancel'] = TRUE;
+ *           break;
+ *         }

+++ b/modules/system/system.api.php
@@ -1824,11 +1824,17 @@ function hook_image_toolkits() {
+    // If the recipient has not opted to receive such messages, cancel sending.
+    if (!modulename_messagekey_optin($message['to'])) {
+      $message['cancel'] = TRUE;
+    }

1) If a module knows upfront that it's not going to send a mail, then I don't think it should attempt to send it in the first place. Therefore, the added example for hook_mail() doesn't make sense to me. In turn, this means we should clarify this in drupal_mail() docs... whereas I believe the current $send function argument has a different point in that the calling module wants to prepare a mail for sending, but take over the sending on its own, or do something in between formatting and sending, or whatever... In turn, it might be wrong to presume that the $send function argument and the 'send' message property are functionality-wise and logically the same. A module that tries to do the aforementioned thing will set $send to FALSE upfront, but will have to check whether $message['send'] has been set to FALSE after invoking drupal_mail(), so it doesn't send a mail that's not supposed to be sent.

2) The example snippet in hook_mail_alter() looks slightly more understandable to me than the one in the hook_mail() phpDoc (optout vs. optin, etc)

+++ b/includes/mail.inc
@@ -87,7 +92,8 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
+ *   manually.  Modules implementing hook_mail_alter() may cancel sending by

No double spaces between sentences in phpDoc.

+++ b/modules/simpletest/simpletest.module
@@ -502,3 +502,16 @@ function simpletest_clean_results_table($test_id = NULL) {
+ * Aborts sending of messages with module='simpletest' and key='cancel_test'.

Replace the '=' with spaces?

+++ b/modules/simpletest/tests/mail.test
@@ -41,6 +41,24 @@ class MailTestCase extends DrupalWebTestCase implements MailSystemInterface {
+    self::$sent_message = NULL;

Any reason for not using $this instead of self:: here?

+++ b/modules/simpletest/tests/mail.test
@@ -41,6 +41,24 @@ class MailTestCase extends DrupalWebTestCase implements MailSystemInterface {
+    $message = drupal_mail('simpletest', 'cancel_test', 'cancel@drupal.org', $language);

All mails in tests need to be addressed to @example.com. drupal.org is a real domain name and has a mail server.

+++ b/modules/simpletest/tests/mail.test
@@ -41,6 +41,24 @@ class MailTestCase extends DrupalWebTestCase implements MailSystemInterface {
+    // Assert that the message was not actually sent.
+    $this->assertNull(self::$sent_message, 'Message cancellation works: <pre>' . var_export(self::$sent_message,1) . '</pre>');

Exporting a variable into an assertion message, which is supposed to be NULL doesn't make much sense to me.

Powered by Dreditor.

Thanks Sun for such a comprehensive review. It is this level of attention to detail that makes Drupal what it is today.

... it's better to keep the argument and property identically named.

Additionally, as a side-note, a property with negated meaning and logic ("cancel") should always ring alarm bells - never do that ...

I thought so too, but decided (just this once!) to simply make the requested changes rather than argue with a senior developer.

Corrected.

1) If a module knows upfront that it's not going to send a mail, then I don't think it should attempt to send it in the first place. Therefore, the added example for hook_mail() doesn't make sense to me.

Like many examples, it is a bit contrived. Realistically, the module implementing hook_mail_alter() would be different from the one sending. I have updated the example in system.api.php to make this clear. Unfortunately, this also highlights a problem. If the sending module only uses drupal_mail() for formatting, not for sending, then the $message['to'] value may not be reliable. I would welcome suggestions for more appropriate examples.

No double spaces between sentences in phpDoc.

Corrected.

+++ b/modules/simpletest/simpletest.module
@@ -502,3 +502,16 @@ function simpletest_clean_results_table($test_id = NULL) {
+ * Aborts sending o,f messages with module='simpletest' and key='cancel_test'.

Replace the '=' with spaces?

Corrected.

+++ b/modules/simpletest/tests/mail.test
@@ -41,6 +41,24 @@ class MailTestCase extends DrupalWebTestCase implements MailSystemInterface {
+    self::$sent_message = NULL;

Any reason for not using $this instead of self:: here?

Yes. The variable is static. Using $this instead of self:: should trigger an error, just as using self:: to refer to a non-static method would trigger an error.

+++ b/modules/simpletest/tests/mail.test
@@ -41,6 +41,24 @@ class MailTestCase extends DrupalWebTestCase implements MailSystemInterface {
+    $message = drupal_mail('simpletest', 'cancel_test', 'cancel@drupal.org', $language);

All mails in tests need to be addressed to @example.com. drupal.org is a real domain name and has a mail server.

Corrected.

+++ b/modules/simpletest/tests/mail.test
@@ -41,6 +41,24 @@ class MailTestCase extends DrupalWebTestCase implements MailSystemInterface {
+    // Assert that the message was not actually sent.
+    $this->assertNull(self::$sent_message, 'Message cancellation works: <pre>' . var_export(self::$sent_message,1) . '</pre>');

Exporting a variable into an assertion message, which is supposed to be NULL doesn't make much sense to me.

Corrected. It was helpful during troubleshooting, i.e., when I was trying to figure out why the variable was not NULL.

Updated 8.x patch passes tests.

Subscribing, don't mind me…

#21: drupal_mail-800434-21.patch queued for re-testing.

Re-rolled for current 8.x checkout.

Fixed and re-rolled.

+++ b/includes/mail.inc
@@ -65,6 +71,23 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
+ * to multiple recipients.
+ *
+ * @code
+ *   function example_notify($accounts) {
+ *     global $language;
...
+ *     }
+ * @endcode

1) Trailing dot should be a period here.

2) Let's remove the function declaration and also the $language line - both unrelated to the example. In turn, the indentation can be decreased.

+++ b/includes/mail.inc
@@ -65,6 +71,23 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
+ *     $message = drupal_mail('example', 'notice', '<>', $language, $accounts, 'reply@example.com', FALSE);
+ *     // Make sure that message sending was not canceled.
+ *     if ($message['send']) {

@@ -108,6 +133,7 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
+    'send'     => $send,

The $send argument cannot be assigned to 'send' by default. As visible in the example code snippet, the code would pass FALSE, so 'send' would always be FALSE; in turn, the calling code intending to send the mail on its own would never send the mail.

This means the inner logic of drupal_mail() concerning $send and 'send' needs to take both values separately into account. Both $send and 'send' default to TRUE, but drupal_mail() only sends the mail, if both are still TRUE. The example for manual sending stays the same; i.e., checking for 'send'.

Lastly, in the example code itself:

- The code should pass a proper e-mail $to address.

- The comment should read "Only send the mail if sending was not canceled."

+++ b/includes/mail.inc
@@ -148,12 +174,16 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
+    if (empty($message['send'])) {
+      $message['result'] = FALSE;
+    }

I think we should assign NULL instead of FALSE here. Not a huge improvement, but in the sending code path, FALSE denotes an mail sending engine error... so FALSE would and should also technically indicate an error here.

Overall, drupal_mail() implements poor error handling though, something we should look into for D8.

+++ b/modules/simpletest/tests/mail.test
@@ -34,10 +34,28 @@ class MailTestCase extends DrupalWebTestCase implements MailSystemInterface {
+    $this->assertNull(self::$sent_message, 'Message cancellation works:');

Missing t() in assertion message, which should also be "Message was cancelled."

+++ b/modules/system/system.api.php
@@ -1826,11 +1826,24 @@ function hook_image_toolkits() {
 function hook_mail_alter(&$message) {
...
+    if ($message['send']) {
+      // If this message is actually being sent, not just formatted for
+      // sending, check the recipient.
+      if (!example_notifications_optin($message['to'], $message['id'])) {

I don't think the logic + comment is a proper and valid example. hook_mail_alter() implementations will have to unconditionally override 'send', regardless of whether 'send' is already FALSE or not.

Powered by Dreditor.

1) Trailing dot should be a period here.

I'm sorry, but I don't understand the distinction between a "trailing dot" and a "period".

2) Let's remove the function declaration and also the $language line - both unrelated to the example. In turn, the indentation can be decreased.

Done.

The $send argument cannot be assigned to 'send' by default. As visible in the example code snippet, the code would pass FALSE, so 'send' would always be FALSE; in turn, the calling code intending to send the mail on its own would never send the mail.

This means the inner logic of drupal_mail() concerning $send and 'send' needs to take both values separately into account. Both $send and 'send' default to TRUE, but drupal_mail() only sends the mail, if both are still TRUE. The example for manual sending stays the same; i.e., checking for 'send'.

Had to build a logic table to understand what you were saying, but on reflection I agree.

Done.

The code should pass a proper e-mail $to address.

You're right; '<>' is valid as a $from address, but not as a $to address. On the other hand, the point of this example is that a message is being formatted once, then sent to multiple recipients.

Replaced by 'user@example.com', but a real-world example is more likely to use NULL or an empty string here.

The comment should read "Only send the mail if sending was not canceled."

Agreed; that sounds better. Fixed.

I think we should assign NULL instead of FALSE here. Not a huge improvement, but in the sending code path, FALSE denotes an mail sending engine error... so FALSE would and should also technically indicate an error here.

Agreed; fixed.

Missing t() in assertion message,

I've stopped using t() in assertion messages since reading #500866: [META] remove t() from assert message

assertion message, which should also be "Message was cancelled."

No argument; changed as requested.

I don't think the logic + comment is a proper and valid example. hook_mail_alter() implementations will have to unconditionally override 'send', regardless of whether 'send' is already FALSE or not.

Yes, if we initialize $message['send'] = TRUE, then the example logic becomes faulty. Fixed.

+++ b/includes/mail.inc
@@ -65,6 +71,29 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
+ * to multiple recipients.

Sorry I meant: colon, not period.

+++ b/includes/mail.inc
@@ -65,6 +71,29 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
+ *   $message = drupal_mail(
+ *     'example', // Module sending mail.
+ *     'notice', // Sending key.
+ *     'user@example.com', // "To" address.
+ *     $language,
+ *     $accounts, // Parameters for sending.
+ *     'reply@example.com',
+ *     FALSE
+ *   );

Please revert to a single line.

Additionally, $params is supposed to be a string-keyed array containing contextual information, so it should be array('accounts' => $accounts) here.

That said, however, it looks like the example is trying to formulate a concept of mass-mailing via drupal_mail(), which is 1) not the topic of this issue, and 2) not really supported by drupal_mail() currently. That is, because implementors still most likely want to have personalized mails in a mass-mail construct.

But again, that's a completely different and separate issue. To get this patch done, we need to focus and limit changes on the currently supported API; i.e., support both $send and 'send', but not a single word related to mass-mailing in the docs.

+++ b/includes/mail.inc
@@ -148,12 +180,16 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
   // Optionally send e-mail.
   if ($send) {
+    if (empty($message['send'])) {
+      $message['result'] = NULL;
+    }
+    else {

When looking at this without contextual background information from this issue, people will ask themselves "WTF?" -- the correlation and intended meaning of $send and 'send' could use one or two inline comments, clarifying that one was set by the caller, the other one potentially overridden by others.

Thanks!

Powered by Dreditor.

Sorry I meant: colon, not period.

Okay; done.

Please revert to a single line.

Doesn't fit on a single line. Changed to two, but I don't like it, because the opening and closing parentheses don't match.

EDIT: Revised example in #34 should meet both our standards.

Additionally, $params is supposed to be a string-keyed array containing contextual information, so it should be array('accounts' => $accounts) here.

Well, then the documentation for the $params argument should say so. Changed in both places.

That said, however, it looks like the example is trying to formulate a concept of mass-mailing via drupal_mail(), which is 1) not the topic of this issue, and 2) not really supported by drupal_mail() currently. That is, because implementors still most likely want to have personalized mails in a mass-mail construct.

Can you suggest another example in which it would make sense to call drupal_mail() with the $send argument set to FALSE?

... the correlation and intended meaning of $send and 'send' could use one or two inline comments...

Comments added.

Can you suggest another example in which it would make sense to call drupal_mail() with the $send argument set to FALSE?

Never mind; I thought of one. You might want to format a message, then add it to a spool for batch-sending later.

Revised example code to fit this new scenario; no other changes.

Slightly better example code.

Thanks, I think this is good to go now.

Closed duplicate: #854304-3: Allow hook_mail_alter to affect sending of mails

While this patch continues development is there a way to implement this functionality now in D6.x? I need to disable the default mail sent by the invite.module so that I can send a custom message through my Campaign Intelligence API call.

-johnny;j

@#37: Sure. Just backport the patch like I did in #15.

#34: drupal_mail-allow_mail_alter_to_cancel-800434-34.patch queued for re-testing.

#34: drupal_mail-allow_mail_alter_to_cancel-800434-34.patch queued for re-testing.

Unless I've missed something, I think this was set to RTBC at #35 - so if the bot's happy too...

#34: drupal_mail-allow_mail_alter_to_cancel-800434-34.patch queued for re-testing.

#4: allownomail-800434-2.diff queued for re-testing.

#34: drupal_mail-allow_mail_alter_to_cancel-800434-34.patch queued for re-testing.

The patch in comment #34 fails because the 8th line of the 5th chunk (the watchdog line) uses the wrong severity. It should be WATCHDOG_ERROR.

Re-rolled.

#48: hook_mail_alter-cancel-800434-48.patch queued for re-testing.

Looks like this addresses sun's concerns. So apart from the minor thing below I'd say this is RTBC, provided that the bot is still happy.

Attaching a D6 backport for people needing it, I ain't sure this API addition can actually be backported.

+++ b/includes/mail.inc
@@ -148,12 +171,20 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
+      // Log errors

Missing trailing dot.

Re-rolled, and change notice written.

Wonderful :)

+++ b/includes/mail.inc
@@ -82,12 +101,15 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
+ *   {$module}_mail() function, if it exists.

hook_mail() or actually MODULE_mail()

+++ b/includes/mail.inc
@@ -82,12 +101,15 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
+ *   If TRUE, drupal_mail will deliver the message by calling

drupal_mail()

--

Otherwise, this looks fine to me. Not sure whether this is backportable though ('cos of the NULL 'result' value), but let's make sure to try.

@sun:

hook_mail() or actually MODULE_mail()

The particular hook_mail() function implemented by the $module module. To quote:

  // Build the e-mail (get subject and body, allow additional headers) by
  // invoking hook_mail() on this module. We cannot use module_invoke() as
  // we need to have $message by reference in hook_mail().
  if (function_exists($function = $module . '_mail')) {
    $function($key, $message, $params);
  }

Re-rolled.

Sorry; missed one.

Right, no need to quote. {$module}_mail() is an unknown documentation pattern to me. hook_mail() is API-documentation-wise correct, and MODULE_mail() would technically be even more correct, since hook_mail() is actually a callback, not a hook.

Since it's documented as hook_mail() currently, I guess it's fine to go with that for now.

That said, those contextual parameters are also passed to hook_mail_alter(), if you want to be nitty gritty nitpicky.

@sun:

hook_mail() is API-documentation-wise correct, and MODULE_mail() would technically be even more correct, since hook_mail() is actually a callback, not a hook.

Whatever. My point is that $params is not passed to every hook_mail() implementation, and it is certainly not passed to a function called MODULE_mail(). It is passed to the particular function whose name is exactly $module . '_mail', where $module is the first parameter to the drupal_mail() function.

That said, those contextual parameters are also passed to hook_mail_alter(), if you want to be nitty gritty nitpicky.

Only if they are copied into the $message array by the $module . '_mail' function. This is typical behavior, but not required by hook_mail() documentation.

An exhaustive search of Drupal 8.x source code reveals no instance where MODULE was used as a placeholder for a named parameter to the function being documented.

Searching for examples of your other cited standard, where hook_FOO() refers to a function whose hook name comes from a passed parameter, yields one concrete example:

_theme_process_registry()

 * Process a single implementation of hook_theme().

...

 * @param $name
 *   The name of the module, theme engine, base theme engine, theme or base theme implementing hook_theme().

The doc header does not conform to current coding standards, but I'll follow its example anyway, as I can't find another.

Re-rolled, with interdiff from #51.

Slightly clearer wording on the $send parameter.

... if you want to be nitty gritty nitpicky.

I do. That, combined with stubborn determination, is what we have in common. ;-)

I hope this is ok now.

I see a lot of revisions spent on existing doxygen wordsmithing which, while awesome is another issue. I would love to see them separated but that's a minor problem.

The important issue here is, where did we accept to have a $send and a send key? Where did we get over the horrible confusion this causes? I did try to skim the whole issue but this simply stopped being a problem apparently?

Well, personally the reason why it was introduced looked sensible to me: this way one can decide to cancel a mail that was intended to be sent, but not send a mail that was intended to be just previewed/built. OTOH this might be too restricitve, no strong preference on this.

Of course the goal of the patch is great! The functionality is good. But the key choice is poor.

So if you don't like the choice of $message['send'] as a flag that hook_mail_alter() implementations can change to abort sending, what choice would you approve?

Saying "I don't like that" without suggesting an alternative is ... less than helpful.

Skimming the comments, I see:

#7

The key 'send' sounds a bit poorly named.

#8

"submit" ? "process" ?

#9

How about we rephrase the key as 'cancel' and reverse the logic?

#19

The function argument to drupal_mail() is already named "send"... although it still sounds a bit fuzzy (within the context of a mail sending function), it's better to keep the argument and property identically named.

Additionally, as a side-note, a property with negated meaning and logic ("cancel") should always ring alarm bells - never do that; it almost always leads to implementation and/or understanding problems in the long run

#62

Where did we accept to have a $send and a send key? Where did we get over the horrible confusion this causes?

Question answered; back to RTBC.

Please don't RTBC your own patch.

#60: hook_mail_alter-cancel-800434-60.patch queued for re-testing.

We have a change record for a non-committed patch here. I don't think this situation is sane. I hope that @catch can shed some light on this and possibily give his feedback about the issue raised by @chx in #62. IMHO the key name is not so confusing, if I'm not missing something we have other examples of this pattern in core, e.g. http://api.drupal.org/api/drupal/modules--field--field.attach.inc/functi....

Rerolled #60 after the /core migration. Since we are getting no feedback here I'll RTBC this again, provided that the tests pass.

See #69.

I had my say in #62 catch will decide.

@chx:

I understand your point, although a suggestion would help, but IMO #69 should be took into consideration.

Edit: I mean, this might be RTBC after all. As you say, let committers decide.

Adding anything to drupal_mail() makes me wince since hook_mail() is one of the worst hooks in all of core, so I opened #1346036: Add an alternative to hook_mail() and deprecate it to see if we can kill it.

+++ b/core/includes/mail.incundefined
@@ -65,13 +71,26 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
+ *   // Only add to the spool if sending was not canceled.

Shouldn't it be cancelled? Or is this US vs. UK spelling?

+++ b/core/includes/mail.incundefined
@@ -82,12 +101,15 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
- *   Optional parameters to build the e-mail.
+ *   An associative array with string keys, which will be passed to the

None of this is to do with the patch, and it should just go in a separate docs clean-up issue I think.

+++ b/core/includes/mail.incundefined
@@ -288,7 +319,7 @@ interface MailSystemInterface {
-   *      defined by one of the other parameters.  PHP's mail() looks for Cc

Same with things like this.

+++ b/core/modules/simpletest/simpletest.moduleundefined
@@ -504,3 +504,16 @@ function simpletest_clean_results_table($test_id = NULL) {
+ * Aborts sending of messages with id 'simpletest_cancel_test'.
+ *

Should be 'ID', not 'id'.

+++ b/core/modules/system/system.api.phpundefined
@@ -1451,11 +1451,20 @@ function hook_image_toolkits() {
+      // sending. This kind of check can be done once in a hook_mail_alter()

If we're adding a feature, we don't need to talk about possible approaches to implementing that feature from before it was committed that are now being replaced.

11 days to next Drupal core point release.

@catch:

Since IMO this is a good candidate for backport, can we move it to Drupal 7 and figure out how to propery address this for D8 in the other issue? This is a badly needed feature, and one that can be implemented only by patching core at the moment.

@plach I'm fine with committing the $send stuff here to Drupal 8 if we can just trim the patch a bit to remove the comment cleanup. webchick would likely want that removed if it is backported anyway - the mass comment cleanup issues are not being backported.

The $send key does not really make things any worse it just doesn't make it better either, which just reminded me how much I don't like it, but I don't think this patch should suffer for that - seems like a useful API addition.

Implemented suggestions from #76.

Shouldn't it be cancelled? Or is this US vs. UK spelling?

Yes, I think "canceled" is legal. I've done a quick core search and got 47 occurrences of "canceled" vs 17 of "cancelled", moreover http://en.wikipedia.org/wiki/Canceled_Apollo_missions :) However I at least ensured that the patch uses the more common form consistently.

Since I only removed comment lines I think it should be safe to mark this back to RTBC. Let's wait for the bot however.

+++ b/core/includes/mail.inc
@@ -57,6 +57,12 @@ define('MAIL_LINE_ENDINGS', isset($_SERVER['WINDIR']) || strpos($_SERVER['SERVER
+ *         // If the recipient can receive such notices by instant-message,
+ *         // do not send by email.

Wrong comment wrapping.

12 days to next Drupal core point release.

@plach: please review #66.

The patch addresses the concerns of a committer. In this case it's fine to RTBC one's own patch, since otherwise the committer might not notice it for a long time. Also note that I did not write any line of it, just rerolled it and removed some hunks as per catch's request.

Let's skip the status ping-pong and get this in, catch already said he's fine with it.

+++ b/core/includes/mail.inc
@@ -148,12 +170,20 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
+    // The original caller requested sending.
+    if (empty($message['send'])) {
+      // Sending was canceled by one or more hook_mail_alter() implementations.
+      // We set 'result' to NULL, because FALSE indicates an error in sending.

Can we append the inner comment to the outer, please? At this point in the function it's really not trivial to follow the flow of the logic, and I got heavily confused why the comment states that the original caller requested sending while the condition checks for FALSE, while it defaults to TRUE.

Appending the inner comment to the outer clarifies the flow.

+++ b/core/includes/mail.inc
@@ -148,12 +170,20 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
+    else {
+      // Sending was originally requested and was not canceled.

Likewise and to stay consistent, move this inner comment also out, right above the else.

5 days to next Drupal core point release.

et voila

@sun:

Anything holding this back left?

Don't think so. Thanks!

Looks good to me, and there is even a patch in progress at #1346036: Add an alternative to hook_mail() and deprecate it. Committed/pushed to 8.x, moving to 7.x for webchick to consider.

Here it is the D7 version.

Just applied the patch with -p2 and rerolled, hence setting back to RTBC.

This functionality actually looks really cool, and would allow for a centralized "Opt-out" contrib module to cancel *all* email coming from Drupal, which seems super handy. Thanks for implementing it in a backwards-compatible way, and for the extra added tests.

We are currently under issue thresholds, and this is the last issue in the D7 RTBC queue, so I'd like to sneak this in. :) However, it no longer applies to D7. :( Could I get a quick re-roll?

Rerolled. It was just a stupid documentation typo fix committed after #88 was rolled, thus back to RTBC.

#90: hook_mail_alter-cancel-800434-90.patch queued for re-testing.

+++ b/modules/simpletest/tests/mail.testundefined
@@ -35,10 +35,28 @@ class MailTestCase extends DrupalWebTestCase implements MailSystemInterface {
+  /**
+   * Test that message sending may be canceled.
+   *

So this is supposed to have a silly s. Since this already went into D8 and got stuck on rerolls for D7, I'm not going to screw with the patch here. Besides, simpletest.module still hasn't been claimed for #1310084: [meta] API documentation cleanup sprint, so I guess we can count on it getting fixed there rather than opening a one-letter followup patch. :P

Committed and pushed to 7.x. Thanks!

(I left the S the way it was to keep the two in sync)

Oops. I got this patch and another one confused! Sorry. I had to roll back because we're not under thresholds yet. ;(

Looks to me like this wasn't actually rolled back? Setting to "fixed" for the time being.

This can easily be backported to D6 too.

D6 backport and documentation backport.

@Albert Volkman:

Thanks!

Would you please roll a single patch containing all the changes and name it something like "hook_mail_alter-cancel-800434-100.D6.patch" this way the bot won't try to automatically test it (see also the attachment notes below).

The first patch is for core, the second is for the documentation project.

Indeed, but ending the filename in -d6.patch is helpful to keep testbot from trying to test it. :)

Ah, very true. Do I need to repost them then? The first as it is for testing, and the second with -d6?

I'd suggest maybe attaching only the core patch here, and opening a separate issue against the API project with the docs patch.

You got it.

http://drupal.org/node/1443112 for documentation patch.

#104: hook_mail_alter_cancel-800434-d6-104.patch queued for re-testing.

/me puts on dunce hat.

Looks good, thanks.

#109: hook_mail_alter-cancel-800434-109.patch queued for re-testing.

Needed a very minor re-roll to apply to HEAD.

P.S. I am completely confused about how to attach a patch to an issue after the D7 upgrade. Apologies if I break things.