Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#11 | messaging-1275936-11.patch | 909 bytes | Dane Powell |
#8 | implements-hook_notifications_message_alter-1275936.patch | 2.94 KB | webflo |
Comments
Comment #1
santam CreditAttribution: santam commentedsubscribe
Comment #2
jday CreditAttribution: jday commentedI'm getting the same problem using Notifications 6.x-4.0-beta7 and Notifications Files 6.x-4.0-beta6
Comment #3
Dane Powell CreditAttribution: Dane Powell commentedOkay 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.
Comment #4
webflo CreditAttribution: webflo commentedHmm. I will look into this. I am also messaging maintainer. I can change those things for an easier integration.
Comment #5
webflo CreditAttribution: webflo commented$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?
Comment #6
webflo CreditAttribution: webflo commentedTry this patch. I am still not sure about this approach. What do you think?
Comment #7
Dane Powell CreditAttribution: Dane Powell commentedDoh- 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.
Comment #8
webflo CreditAttribution: webflo commentedI am sorry. Here is the patch.
Comment #9
Dane Powell CreditAttribution: Dane Powell commentedThe 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...
... 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.
Comment #10
Dane Powell CreditAttribution: Dane Powell commentedHey 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...
Comment #11
Dane Powell CreditAttribution: Dane Powell commentedHey 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.
Comment #12
webflo CreditAttribution: webflo commentedThanks for you work! I committed both patches to Notifications (b6ce6cd on 7.x-1.x) and Messaging (719c1e7 on 7.x-1.x).
Comment #13
Dane Powell CreditAttribution: Dane Powell commentedThanks likewise!
Comment #15
donquixote CreditAttribution: donquixote commentedMaybe related:
#1439152: D7: Infinite loop with mailcomment_notifications_message_alter()
on Mailcomment queue.
Comment #16
donquixote CreditAttribution: donquixote commentedIndeed, 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..
Comment #17
donquixote CreditAttribution: donquixote commentedDidn't mean to change the project.
(In fact I did mean to, but then thought it's a bad idea)
Comment #18
Dane Powell CreditAttribution: Dane Powell commentedIf you use Notifications / Messaging 7.x-1.0-alpha2, you should not have this issue.