Add hook to conditionally disable comment notifications

[coffee2code]

E-mail notifications for posted comments are controlled by the 'comments_notify' setting. E-mail notifications for comments needing modification are controlled by the 'moderation_notify' setting. Each is an all or nothing setting, i.e. if 'on', ALL post authors will receive notifications when appropriate. AFAIK, there isn't a clean way for a plugin to insert itself into the notification process.

[matt]

These are already wrapped in function_exists, could you just rewrite them.

[coffee2code]

Hmm, well, at your suggestion I tried to simply implement a version of wp_notify_postauthor() in a plugin but it didn't like me redeclaring the function since it had already been declared in comment-functions.php. I don't doubt that I may be overlooking something.

However, even if it did work, I am not really attempting to reimplement the whole notification function (which as an approach wouldn't be upgrade-friendly and wouldn't play well with other plugins). Mostly I just envision the aforementioned hook to allow an easy yea/nay on notification after WP has otherwise concluded the notification could go through (at a point when we also know the $comment_id of the already posted comment). And with the hook, other plugins could chain themselves into the notification approval/denial check.

[jeffstieler]

The "just rewrite them" response needs to be reconsidered as the wp_notify_postauthor function is about 100 lines long now.

[scribu]

Epic revival.

[johnbillion]

This is an elegantly simple patch, but the main issue at hand is that the settings for comment notifications are inadequate. This filter should be taken into consideration in #7835 and #6286.

[coffee2code]

Refresh of jeffstieler's patch to apply cleanly against trunk (with minor tweaks)

[coffee2code]

Attached 761.diff​ as a refresh of jeffstieler's patch. It removes the whitespace removals (which have since been trimmed in core), added some parentheses spacing, and removed single-use variable.

[scribu]

Replying to johnbillion:

This is an elegantly simple patch, but the main issue at hand is that the settings for comment notifications are inadequate. This filter should be taken into consideration in #7835 and #6286.

I disagree; this filter should make sense no matter what settings are in place in the UI.

Regarding the patch from coffee2code, I'm wondering why the following check is only done in one place:

&& ( ! isset( $commentdata['user_id'] ) || $post->post_author != $commentdata['user_id'] )

[nacin]

This filter is clever but I wonder if all of that context should be passed to the filter, rather than happening after $do_notify. So:

$maybe_notify = get_option( 'comments_notify' ) && $commentdata['comment_approved'];
if ( isset( $commentdata['user_id'] ) && $post->post_author == $commentdata['user_id'] )
    $maybe_notify = false;
$maybe_notify = apply_filters( 'notify_post_author', $maybe_notify, $comment_ID );

I could go either way on this, though. It can be argued that only sending emails for approved comments and never sending emails to the comment author don't need to be filtered. It would however be weird for that filter to run for a comment ID that these checks later prevent. So perhaps the comment_approved and user_id checks take place, then the filter, then the function call.

It should also be noted that wp_notify_postauthor() also checks $comment->user_id == $post->post_author already (and has for some time), so maybe we should just be always calling wp_notify_postauthor() and let the function later in the stack handle it.

[coffee2code]

Patch to defer checks until wp_notify_postauthor()

[coffee2code]

I actually really like deferring all the post author notification checks until wp_notify_postauthor(). I pursued that approach in 761.2.diff​.

The main benefit is that it centralizes all post author notification logic, which:

• Removes code duplication currently found in wp_new_comment() and wp_set_comment_status() (the only two core functions to call wp_notify_postauthor(); both functions perform similar checks prior to calling wp_notify_postauthor())
• Removes code/effort duplication with certain checks that occur in wp_new_comment()/wp_set_comment_status() that are repeated in wp_notify_postauthor()
• Fixes inconsistency between wp_new_comment() and wp_set_comment_status() (as pointed out by @scribu, the latter didn't include a check to compare the post author with the commenter to prevent self-notifications, which turns out to be unnecessary because wp_notify_postauthor() performs that check anyhow)
• Brings wp_notify_postauthor() into closer alignment with wp_notify_moderator() which checks its notification setting value (get_option( 'moderation_notify' )) itself.

And of course, as per the original ticket, the patch introduces a 'wp_notify_postauthor' filter to allow overriding the get_option('comments_notify') and comment_approved checks for any $comment.

The only risk I see for breaking existing compatibility would be if someone was calling wp_notify_postauthor() directly and not expecting get_option('comments_notify') and comment_approved checks to be performed. They can bypass those using the newly introduced 'wp_notify_postauthor' hook and do a __return_true.

Additionally, the patch does change wp_set_comment_status() such that the call to wp_notify_postauthor() happens after the $wpdb->update(). This ensures that the comment is in the approved state prior to wp_notify_postauthor() acts on the comment, and also prevents the notification from being triggered if the update fails for some reason.

Tangentially, the $comment_type argument to wp_notify_postauthor() is superfluous, but I'll open a separate ticket to address that.

Another thought: remove the wp_notify_postauthor() call from both wp_new_comment() and wp_set_comment_status() and instead hook wp_notify_postauthor() to the 'comment_post' and 'wp_set_comment_status' (or 'transition_comment_status') actions respectively.

[coffee2code]

Minimal change to introduce filter (and not touch the pluggable wp_notify_postauthor())

[coffee2code]

As @nacin ​pointed out in IRC, the problem with pushing the logic into a pluggable function is that any "plugged" versions of this function out there can't be guaranteed to have the proper checks in them. The existing checks in wp_new_comment() and wp_set_comment_status() may be currently preventing emails from being sent that would otherwise get sent if these custom-defined but non-checking versions of wp_notify_postauthor() are called. It's unfortunate that pluggable functions limit us in what we can implement.

Therefore, 761.3.diff​ is attached as a change-minimizing and more backwards compatible approach, though less centralized and de-duplicative of effort as previously put forth.

Other than introducing the 'wp_notify_post_author' filter, this patch does not provide for any of the benefits I mentioned in my previous comment.

However, in addition to the new filter, I carried over one related change from the previous patch:

Additionally, the patch does change wp_set_comment_status() such that the call to wp_notify_postauthor() happens after the $wpdb->update(). This ensures that the comment is in the approved state prior to wp_notify_postauthor() acting on it, and also prevents the notification from being triggered if the update fails for some reason (prior to this patch, the notification gets sent before the update is attempted).

I still favor 761.2.diff​, but 761.3.diff​ is sufficient to satisfy this ticket.

[nacin]

Let's take a little more time to make sure we get this right.

[adamsilverstein]

761.4.diff​:

• Refresh last patch against trunk
• Add some hook docs
• Introduces similar wp_notify_moderator filter (including a later check inside wp_notify_moderator() if you want to override a disabled setting)
• In all cases, passes the current setting and comment id for context

Appreciate feedback! and unit tests!

[DrewAPicture]

On 761.4.diff​, let's not use @uses in the context of hooks. I would prefer for those notes to be in the DocBlock descriptions, and for hooks particularly, you can simply use the inline @see tag with single quotes syntax.

Example:

<?php
/**
 * Fires immediately after a comment is inserted into the database.
 *
 * Calls {@see 'wp_notify_post_author'} to determine if the post author should be
 * notified. Calls {@see 'wp_notify_moderator'} to determine if the site moderator
 * should be notified.
 *
 * @since 1.2.0
 *
 * @param int $comment_ID       The comment ID.
 * @param int $comment_approved 1 (true) if the comment is approved, 0 (false) if not.
 */

[adamsilverstein]

Thanks for the correct docs syntax, appreciate the feedback. I copied the @uses format from the old patch; will fix.

Replying to DrewAPicture:

On 761.4.diff​, let's not use @uses in the context of hooks. I would prefer for those notes to be in the DocBlock descriptions, and for hooks particularly, you can simply use the inline @see tag with single quotes syntax.

Example:

<?php
/**
 * Fires immediately after a comment is inserted into the database.
 *
 * Calls {@see 'wp_notify_post_author'} to determine if the post author should be
 * notified. Calls {@see 'wp_notify_moderator'} to determine if the site moderator
 * should be notified.
 *
 * @since 1.2.0
 *
 * @param int $comment_ID       The comment ID.
 * @param int $comment_approved 1 (true) if the comment is approved, 0 (false) if not.
 */

[adamsilverstein]

In 761.5.diff​:

• Clean up hook docs as per @DrewAPicture's recommendations

[adamsilverstein]

In 761.6.diff​:

• Add some unit tests, testing filters and notifications along the way.

[adamsilverstein]

761.7.diff​:

• refresh against trunk, after comments code changes
• don't include 'comment_approved' in filtered condition for wp_new_comment_notify_postauthor
• Doc block language fixes from Drew

[thomaswm]

Replying to coffee2code:

AFAIK, there isn't a clean way for a plugin to insert itself into the notification process.

There is a way now. In #33587, the calls to the notification functions were replaced by hooks. Since [34106], the functions wp_notify_postauthor and wp_notify_moderator are called using the comment_post hook.

[adamsilverstein]

@thomaswm - thanks for pointing out that change, it is closely related.

Although the new method of firing the notifications does enable better filtering of notifications, for example disabling them, it does not override the settings in the same way the hooks on this ticket.

With the hooks here, your filter can force an email to be sent even if it would normally not be, or the opposite. For example, you could have a plugin that always sends you an email when certain keywords are used in a post, even if your normal settings are no notifications.

In any case, the new hook triggering likely means my patch needs a refresh :) thanks again for pointing it out.

[adamsilverstein]

761.8.diff​:

Refresh against trunk.

[DrewAPicture]

Docs fixes

[DrewAPicture]

Working with @adamsilverstein to troubleshoot two failing tests.

[DrewAPicture]

Split test assertions

[adamsilverstein]

In 761.11.diff​ :

• Fix some logic
• Add a missing filter repeat application in wp_notify_moderator() after recent shuffling

All unit tests pass now

[DrewAPicture]

In 35339:

Comments: Introduce two new filters, notify_moderator and notify_post_author, both of which make it possible to selectively override site notification email settings for new comments.

The notify_moderator filter makes it possible to override the value for the moderation_notify option, which controls whether to send new comment emails to "site moderators", that is to say, the owner of the admin email for the site and the post author if they have the ability to modify the comment.

The notify_post_author filter likewise makes it possible to override the value for the comments_notify option, which controls whether to send new comment emails to the post author. If the post author is the comment author, default behavior is not to send the notification. Note: enabling or disabling notifications via this hook could also affect other recipients added via the 'comment_notification_recipients' filter in wp_notify_postauthor(), if hooked.

Passing a falsey value to either of the new filters will prevent notifications from being sent, regardless of their corresponding option values.

Adds tests.

Props coffee2code, adamsilverstein, DrewAPicture.
Fixes #761.

[johnbillion]

In 35347:

Correctly use WP_TESTS_EMAIL in email tests.

See #761, #34000