In notifications.message.inc, the function message_build() is never called during the notification generation process. As a result, implementations of hook_notifications_message_alter() are never called, breaking almost all notifications plugin modules (such as Mail Comment and Notifications Files).

I tried poking through the code but I'm not sure where this function should be called from.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

santam’s picture

subscribe

jday’s picture

I'm getting the same problem using Notifications 6.x-4.0-beta7 and Notifications Files 6.x-4.0-beta6

Dane Powell’s picture

Okay I've looked into this some. It seems that message_build() should be removed from notifications.message.inc, since messaging.message.inc (the parent class) already has a function do_build that invokes hook_messaging_message($op, $message). In other words, if we go with this approach, modules such as mailcomment and notifications_files would need to replace hook_notifications_message_alter with hook_messaging_message.

BUT- the problem with this is that module_invoke_all cannot pass $message by reference, so those modules can't actually modify the message! That's why drupal_alter / hook_notifications_message_alter was used before. The only solutions I can think of are to use drupal_alter instead of module_invoke_all, or implement our own version of module_invoke_all that can pass $message by reference.

webflo’s picture

Assigned: Unassigned » webflo

Hmm. I will look into this. I am also messaging maintainer. I can change those things for an easier integration.

webflo’s picture

$message is a object and objects are passed "by reference" by default. i think module_invoke_all is no problem. But provides hook_messaging_message all information you need?

webflo’s picture

Status: Active » Needs review

Try this patch. I am still not sure about this approach. What do you think?

Dane Powell’s picture

Doh- you are right, $message is passed by reference by default. Apparently I don't understand something about PHP- I was confused because I implemented hook_messaging_message as

function mailcomment_messaging_message($op, &$message)

...and I was getting a PHP error...

Warning: Parameter 2 to mailcomment_messaging_message() expected to be a reference, value given in module_invoke_all()

If I take out the ampersand before $message, it still gets passed by reference and I don't get an error.

So I guess the only thing left to do for this issue is to document somewhere that hook_notifications_message_alter has been replaced with hook_messaging_message. Sorry to bother you with what seems to be a misunderstanding of PHP on my part!

By the way- your patch wasn't attached.

webflo’s picture

I am sorry. Here is the patch.

Dane Powell’s picture

The patch itself looks good, but I am still having trouble converting a hook_notifications_message_alter() from D6 to D7. The problem is that mailcomment needs to set the message subject, as well as the header and footer of the body. Similarly, notifications_files would need to add attachments. However, I can't figure out how to edit message parts. If I use code similar to the following...

<?php
function mailcomment_notifications_message_alter(&$message) {
  $footer = $message->get_template()->get_element('footer') . 'add this text to the footer';
  $message->get_template()->add_element('footer', $footer);
}
?>

... the script goes into a loop and hits the memory limit, because get_template() calls render() calls do_build() calls mailcomment_notifications_message_alter() calls get_template()... repeat ad nauseum.

Dane Powell’s picture

Status: Needs review » Needs work

Hey webflo, any thoughts on the problem in #9? If we can just figure that out, then we can release compatible versions of Notifications Files and Mailhandler...

Dane Powell’s picture

Status: Needs work » Needs review
FileSize
909 bytes

Hey webflo- I think I have this nailed. I think we should apply your patch in #8, plus this patch to messaging (which I have maintainer access to, if you'd like for me to do it myself). This patch simply sets the message template in the build process rather than the render process, which solves the loop condition mentioned above and shouldn't cause any other problems.

webflo’s picture

Status: Needs review » Fixed

Thanks for you work! I committed both patches to Notifications (b6ce6cd on 7.x-1.x) and Messaging (719c1e7 on 7.x-1.x).

Dane Powell’s picture

Thanks likewise!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

donquixote’s picture

donquixote’s picture

Project: Notifications » Messaging

Indeed, the patch in #11 fixes the mailcomment issue.

The issue settings for this one are a bit misleading, as there are patches both to messaging and to notifications.
What versions of messaging and notifications will have this patch included? Don't say -dev, that's a moving target..

donquixote’s picture

Project: Messaging » Notifications

Didn't mean to change the project.
(In fact I did mean to, but then thought it's a bad idea)

Dane Powell’s picture

If you use Notifications / Messaging 7.x-1.0-alpha2, you should not have this issue.