WordPress.org

Make WordPress Core

Opened 9 years ago

Last modified 13 days ago

#761 reopened enhancement

Add hook to conditionally disable comment notifications

Reported by: coffee2code Owned by: matt
Milestone: Future Release Priority: normal
Severity: normal Version: 1.2.2
Component: Comments Keywords: has-patch needs-testing 3.6-early
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 (5)

notifyhooks_comment-functions.diff (1.1 KB) - added by coffee2code 9 years ago.
comment.php.r19734.diff (3.3 KB) - added by jeffstieler 2 years ago.
761.diff (2.0 KB) - added by coffee2code 2 years ago.
Refresh of jeffstieler's patch to apply cleanly against trunk (with minor tweaks)
761.2.diff (2.6 KB) - added by coffee2code 22 months ago.
Patch to defer checks until wp_notify_postauthor()
761.3.diff (2.6 KB) - added by coffee2code 22 months ago.
Minimal change to introduce filter (and not touch the pluggable wp_notify_postauthor())

Download all attachments as: .zip

Change History (21)

comment:1 coffee2code9 years ago

  • Patch set to No

comment:2 matt9 years ago

  • Status changed from new to assigned

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

comment:3 coffee2code9 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.

comment:4 matt9 years ago

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

comment:5 jeffstieler2 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.

comment:6 scribu2 years ago

  • Milestone set to Awaiting Review

Epic revival.

comment:7 follow-up: johnbillion2 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.

coffee2code2 years ago

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

comment:8 coffee2code2 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.

comment:9 scribu2 years ago

  • Milestone changed from Awaiting Review to 3.5

comment:10 in reply to: ↑ 7 scribu2 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'] )

comment:11 jane23 months ago

  • Keywords needs-testing added

comment:12 nacin22 months 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.

coffee2code22 months ago

Patch to defer checks until wp_notify_postauthor()

comment:13 coffee2code22 months 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 (prior to this patch, the notification gets sent before the update is attempted).

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.

Last edited 22 months ago by coffee2code (previous) (diff)

coffee2code22 months ago

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

comment:14 coffee2code22 months 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.

comment:15 nacin20 months 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.

comment:16 ocean905 months ago

  • Summary changed from Add hook to conditionally disable comment notifications [w/ patch] to Add hook to conditionally disable comment notifications
Note: See TracTickets for help on using tickets.