Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.In some cases, it would be useful for a module to override the default logic for deciding whether to send an e-mail notification to a user for a particular message.
Use cases include: enforcing an enabled or disabled state for particular roles, sending messages only if a user has not been active on the site, etc.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | privatemsg-1313808-8-email_notify_hook.patch | 5.36 KB | arithmetric |
| #5 | privatemsg-1313808-5-email_notify_hook.patch | 3.89 KB | arithmetric |
| #1 | privatemsg-1313808-email_notify_hook.patch | 1.67 KB | arithmetric |
Comments
Comment #1
arithmetric CreditAttribution: arithmetric commentedAttached is a patch that adds a hook from _pm_email_notify_send_check().
Comment #2
arithmetric CreditAttribution: arithmetric commentedComment #4
BerdirThe test fails are unrelated, you can ignore them. Testbot started to test tests with dependencies recently and it looks like either the integration or the tests or services.module is broken..
Patch looks good. Just wondering if it would make sense to call the hook a bit later so that more context can be provided.
Also, hooks like hook_node_access() introduced IGNORE/DENY/ALLOW constants to make the difference between NULL/TRUE/FALSE more explicit, might be a good idea here too?
Comment #5
arithmetric CreditAttribution: arithmetric commented@Berdir,
Thanks for the review. I updated the patch to introduce the constants PM_EMAIL_NOTIFY_SEND_ACCESS_ALLOW, PM_EMAIL_NOTIFY_SEND_ACCESS_DENY, and PM_EMAIL_NOTIFY_SEND_ACCESS_IGNORE (like node.module in D7). Also I moved the hook invocation and other permissions checks inside of the caching code.
I'm not sure about moving the hook later, because I want to let modules override all of the default logic.
What do you think about this approach?
Comment #6
BerdirThat works for me. Note that when you move it inside the static cache, you also need to set $notifications[$uid][$mid] correctly (aka, when it's not FALSE as that's the default) or follow-up checks would always return FALSE.
- It would be nice to have a usage example in the hook documentation, for example check against a specific role id and then return DENY.
- Tests for this would of course be awesome if you know how to write these. basically what we'd need is is a privatemsg_test.module that implements this hook in a way that allows us to DENY/ALLOW according to something (you could use variable_set('privatemsg_test_mail_deny', $user->mail) inside the test code, then trigger the notification and afterwards check if the mail wasn't sent. There are already quite nice tests for the whole notification thing, so extending that shouldn't require a huge amount of work.
Comment #8
arithmetric CreditAttribution: arithmetric commentedThanks for the review, Berdir.
Attached is an updated version of the patch that fixes the code to correctly set the static cache for the access decision. Also, I added an example in the hook documentation.
I haven't yet had a chance to write the test.
Comment #9
BerdirComment #10
oadaeh CreditAttribution: oadaeh as a volunteer commentedThis issue is being closed because it is against a branch for a version of Drupal that is no longer supported.
If you feel that this issue is still valid, feel free to re-open and update it (and any possible patch) to work with the 7.x-1.x branch (bug fixes only) or the 7.x-2.x branch.
Thank you.