WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 4 months ago

Last modified 4 months ago

#761 closed enhancement (fixed)

Add hook to conditionally disable comment notifications

Reported by: coffee2code Owned by: DrewAPicture
Milestone: 4.4 Priority: normal
Severity: normal Version: 1.2.2
Component: Comments Keywords: has-patch dev-feedback
Focuses: Cc:

Description

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.

Attachments (13)

notifyhooks_comment-functions.diff (1.1 KB) - added by coffee2code 11 years ago.
comment.php.r19734.diff (3.3 KB) - added by jeffstieler 4 years ago.
761.diff (2.0 KB) - added by coffee2code 4 years ago.
Refresh of jeffstieler's patch to apply cleanly against trunk (with minor tweaks)
761.2.diff (2.6 KB) - added by coffee2code 3 years ago.
Patch to defer checks until wp_notify_postauthor()
761.3.diff (2.6 KB) - added by coffee2code 3 years ago.
Minimal change to introduce filter (and not touch the pluggable wp_notify_postauthor())
761.4.diff (3.6 KB) - added by adamsilverstein 5 months ago.
761.5.diff (3.5 KB) - added by adamsilverstein 5 months ago.
761.6.diff (9.8 KB) - added by adamsilverstein 5 months ago.
761.7.diff (9.6 KB) - added by adamsilverstein 5 months ago.
761.8.diff (9.1 KB) - added by adamsilverstein 4 months ago.
761.9.diff (9.5 KB) - added by DrewAPicture 4 months ago.
Docs fixes
761.10.diff (9.9 KB) - added by DrewAPicture 4 months ago.
Split test assertions
761.11.diff (10.6 KB) - added by adamsilverstein 4 months ago.

Download all attachments as: .zip

Change History (48)

#1 @coffee2code
11 years ago

  • Patch set to No

#2 @matt
11 years ago

  • Status changed from new to assigned

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

#3 @coffee2code
11 years ago

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.

#4 @matt
11 years ago

  • Owner changed from anonymous to matt
  • Resolution changed from 10 to 90
  • Status changed from assigned to closed

#5 @jeffstieler
4 years ago

  • Cc jeff@… added
  • Component changed from General to Comments
  • Keywords has-patch added
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Summary changed from Consider adding hook to facilitate conditionally disabling notifications [w/ patch] to Add hook to conditionally disable comment notifications [w/ patch]
  • Version 1.5 deleted

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

#6 @scribu
4 years ago

  • Milestone set to Awaiting Review

Epic revival.

#7 follow-up: @johnbillion
4 years ago

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
4 years ago

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

#8 @coffee2code
4 years ago

  • Version set to 1.2.2

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.

#9 @scribu
4 years ago

  • Milestone changed from Awaiting Review to 3.5

#10 in reply to: ↑ 7 @scribu
4 years ago

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'] )

#11 @jane
3 years ago

  • Keywords needs-testing added

#12 @nacin
3 years ago

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
3 years ago

Patch to defer checks until wp_notify_postauthor()

#13 @coffee2code
3 years ago

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.

Version 0, edited 3 years ago by coffee2code (next)

@coffee2code
3 years ago

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

#14 @coffee2code
3 years ago

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.

#15 @nacin
3 years ago

  • Keywords 3.6-early added
  • Milestone changed from 3.5 to Future Release

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

#16 @ocean90
2 years ago

  • Summary changed from Add hook to conditionally disable comment notifications [w/ patch] to Add hook to conditionally disable comment notifications

#17 @Chair Hire London
15 months ago

  • Keywords reporter-feedback added

#18 @chriscct7
9 months ago

  • Keywords needs-refresh dev-feedback added; 3.6-early reporter-feedback removed

#19 @adamsilverstein
5 months ago

  • Keywords needs-refresh removed

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!

#20 follow-up: @DrewAPicture
5 months ago

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.
 */
Last edited 5 months ago by DrewAPicture (previous) (diff)

#21 in reply to: ↑ 20 @adamsilverstein
5 months ago

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.
 */

#22 @adamsilverstein
5 months ago

  • Keywords needs-unit-tests added

In 761.5.diff:

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

#23 @adamsilverstein
5 months ago

  • Keywords needs-testing needs-unit-tests removed

In 761.6.diff:

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

#24 @adamsilverstein
5 months ago

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
Last edited 5 months ago by adamsilverstein (previous) (diff)

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


5 months ago

#26 @SergeyBiryukov
5 months ago

  • Milestone changed from Future Release to 4.4
  • Owner changed from matt to SergeyBiryukov
  • Status changed from reopened to assigned

#27 in reply to: ↑ description @thomaswm
5 months ago

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.

#28 @adamsilverstein
5 months ago

@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.

This ticket was mentioned in Slack in #core by sergey. View the logs.


4 months ago

#30 @adamsilverstein
4 months ago

761.8.diff:

Refresh against trunk.

@DrewAPicture
4 months ago

Docs fixes

#31 @DrewAPicture
4 months ago

  • Owner changed from SergeyBiryukov to DrewAPicture
  • Status changed from assigned to reviewing

Working with @adamsilverstein to troubleshoot two failing tests.

@DrewAPicture
4 months ago

Split test assertions

#32 @adamsilverstein
4 months ago

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

#33 @DrewAPicture
4 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


4 months ago

#35 @johnbillion
4 months ago

In 35347:

Correctly use WP_TESTS_EMAIL in email tests.

See #761, #34000

Note: See TracTickets for help on using tickets.