Wrong implementation of hook_mail_alter()

I agree... I discovered that this is blocker when using Mime Mail with Subscriptions Mail module... You will get a "white screen of death"...

I'm not very familiar with Drupal mail system... Maybe swapping module alteration with mail preparation can solve the issue...

Now...

  foreach (module_implements('mail_alter') as $module) {
    $function = $module .'_mail_alter';
    $function($mailkey, $recipient, $subject, $body, $sender, $headers);
  }

  $engine_prepare = variable_get('mimemail_engine', 'mimemail') .'_prepare';
  if (function_exists($engine_prepare)) {
    $message = $engine_prepare($sender, $recipient, $subject, $body, $plaintext, $headers, $text, $attachments, $mailkey);
  }
  else {
    $message = mimemail_prepare($sender, $recipient, $subject, $body, $plaintext, $headers, $text, $attachments, $mailkey);
  }

and (maybe) we can use it like...

  $engine_prepare = variable_get('mimemail_engine', 'mimemail') .'_prepare';
  if (function_exists($engine_prepare)) {
    $message = $engine_prepare($sender, $recipient, $subject, $body, $plaintext, $headers, $text, $attachments, $mailkey);
  }
  else {
    $message = mimemail_prepare($sender, $recipient, $subject, $body, $plaintext, $headers, $text, $attachments, $mailkey);
  }

  foreach (module_implements('mail_alter') as $module) {
    $function = $module .'_mail_alter';
    $function($message);
  }

Here's an attempted fix which I've not yet fully tested. But without it, mimemail horribly broke my site :)

Here is an interesting comment about which hooks mimemail should implement and which ones it should not.

I dont understand all of it, but it looks like the suggestions made would make mimemail work together with other mailing modules much better.

http://drupal.org/node/132903#comment-1610070

It does sound like there are some architectural issues to work on. In the meantime hopefully we could at least fix this really critical bug (for those of us who need to alter our mail :)

Workaround is to test the number of args passed in to your module's mail alter function.

/**
 * Implementation of hook_mail_alter()
 */
function my_module_mail_alter(&$message, &$to = '', &$subject = '', &$body = '', &$from = '', &$headers = array()) {
  if(count(func_get_args()) == 1) {
    $message['to'] = 'to@example.com';
    $message['from'] = 'from@example.com';
  }
  else {
    $to = 'to@example.com';
    $from = 'from@example.com';
  }
}

See also #443964: Skip style sheets with print media

I got to this issue by searching the queue. Not really because of the same problem explained in this issue but it's definitely related. I agree with some of the comments on the node posted by batje in #3. Basically I have followed this issue historically. That code, the mail_alter hook was added into the module in #263142: support hook_mail_alter for better integration with other modules to overcome missuse of mimemail() function in favor drupal_mail() directly. Basically, what was happening is that simplenews, for instance, was doing something like:

if (module_exists('mimemail)) {
  mimemail(...);
} else {
 drupal_mail(...);
}

Simplenews, at least, has improved the way it sends mail, and now, although calling mimemail directly, it also calls drupal_mail() so that function can render the mail. drupal_mail() has an argument that tells not to actually send the mail, but just process it (including mail_alter). So basically, what you are getting with this code (fixed or not) is that mail_alter is mainly executed twice (once from drupal_mail() and once from mimemail(), which I think is bad behavior). I am working with a site that uses a combination of simplenews, simplenews_template and mimemail to send fancy html newsletter and I just found the calling of mail_alter twice. It's not really something that for now will break my site, but I just think it's completely unnecessary to call mail_alter twice.

Attached is a patch that strips the whole code alltogether, besides, it makes mimemail at least one tiny step to be easier to maintain.

Hopefully, if you agree, you could try out the patch in your configuration and see if that actually works, and if not, why and what modules is actually not working, but hook_mail_alter() should be run by drupal_mail() only, and not by any other module, unless it works completely by itself, which is not currently the case of mimemail (as mimemail actually plugis into drupal_mail()).

Anything happening on this? The patch submitted by mfb in comment #2 could also add support for attachments in the $mesages array. It will allow for using hook_mail_alter to alter add attachments.

It's still implemented as (in the alpha-2):

<?php
foreach (module_implements('mail_alter') as $module) {
    $function = $module .'_mail_alter';
    $function($mailkey, $recipient, $subject, $body, $sender, $headers);
  }
?>

Should I take the dev version or just apply the patch ?

subscribing
I would also like to know if i should apply patch or grab latest dev?

It is still wrong implemented in the alpha3.

And just make crashes my other modules, plans are to fixe this problem ? The patch is fine. Or let the wrong API implementation for the moment ?

should be

<?php
drupal_alter('mail', $message);
?>

instead of

<?php
  foreach (module_implements('mail_alter') as $module) {
    $function = $module .'_mail_alter';
    $function($mailkey, $recipient, $subject, $body, $sender, $headers);
  }
?>

And provide a hook_mimemail_alter for full support.

The implementation is wrong for sure, but it is not just that. Since drupal_mail() calls hook_mail_alter() if we call again in mimemail() it will run twice, which is not so good. IMHO, we should stick with hanoii's patch in #7 and strip out these lines.

#865658: Wrong parameters to hook_mail_alter() is a duplicate of this.

Yes, #7 seems the way to go. If it's really required, then add a proper hook_mimemail_alter as #12 suggested.

Patch in #7 is committed to HEAD.

If needed we can add a hook_mimemail_alter(), just open an other issue for it.