Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#25699 closed defect (bug) (fixed)

When filtering comment notification email recipients, notifications are not sent for author comments

Reported by: chipbennett's profile chipbennett Owned by: markjaquith's profile markjaquith
Milestone: 3.8 Priority: normal
Severity: minor Version: 3.7
Component: Comments Keywords: has-patch
Focuses: Cc:

Description

Reference support thread:
http://wordpress.org/support/topic/no-notifications-when-post-author-comments?replies=9

Core filters 'comment_notification_recipients' for wp_notify_postauthor() and 'comment_moderation_recipients' for wp_notify_moderator() were added in WordPress 3.7. When 'comment_notification_recipients' is filtered to add additional recipients, no email notification is sent for post-author comments.

This issue occurs because of this conditional check in the wp_new_comment() function in wp-includes/comment.php:

if ( get_option('comments_notify') && $commentdata['comment_approved'] && ( ! isset( $commentdata['user_id'] ) || $post->post_author != $commentdata['user_id'] ) )
	wp_notify_postauthor($comment_ID, isset( $commentdata['comment_type'] ) ? $commentdata['comment_type'] : '' );

Specifically:

$post->post_author != $commentdata['user_id']

Obviously, this conditional is in place for good reason (post authors don't need to receive notification for their own comments); but it interferes with email notification for multiple recipients.

I'd be happy to patch, but I'm not sure of the most elegant solution.

Attachments (7)

25699.patch (7.7 KB) - added by ethitter 11 years ago.
25699.2.patch (7.6 KB) - added by ethitter 11 years ago.
25699.3.patch (7.3 KB) - added by markjaquith 11 years ago.
some cleanup
25699.4.patch (5.9 KB) - added by ethitter 11 years ago.
25699.5.patch (5.9 KB) - added by ethitter 11 years ago.
25699.inline-docs.diff (2.6 KB) - added by markjaquith 11 years ago.
Inline docs changes.
25699.6.patch (2.6 KB) - added by DrewAPicture 11 years ago.

Download all attachments as: .zip

Change History (19)

#1 @nacin
11 years ago

  • Milestone changed from Awaiting Review to 3.8

#2 in reply to: ↑ description @DaveAl
11 years ago

Replying to chipbennett:

Reference support thread:
http://wordpress.org/support/topic/no-notifications-when-post-author-comments?replies=9

Core filters 'comment_notification_recipients' for wp_notify_postauthor() and 'comment_moderation_recipients' for wp_notify_moderator() were added in WordPress 3.7. When 'comment_notification_recipients' is filtered to add additional recipients, no email notification is sent for post-author comments.

This issue occurs because of this conditional check in the wp_new_comment() function in wp-includes/comment.php:

if ( get_option('comments_notify') && $commentdata['comment_approved'] && ( ! isset( $commentdata['user_id'] ) || $post->post_author != $commentdata['user_id'] ) )
	wp_notify_postauthor($comment_ID, isset( $commentdata['comment_type'] ) ? $commentdata['comment_type'] : '' );

Specifically:

$post->post_author != $commentdata['user_id']

Obviously, this conditional is in place for good reason (post authors don't need to receive notification for their own comments); but it interferes with email notification for multiple recipients.

I'd be happy to patch, but I'm not sure of the most elegant solution.

There appear to be two problems to solve - both in similar fashion - and a question.

  1. In wp_new_comment(), a check for multiple recipients needs to be added to the if condition, something along the lines of:
    $recipients = 0;
    $recipients = count( apply_filters( 'comment_notification_recipients', $recipients, $comment_id ) );
    ...
    ...
    if ( get_option('comments_notify') && $commentdata['comment_approved'] && ( ! isset( $commentdata['user_id'] ) || $post->post_author != $commentdata['user_id'] || $recipients > 0 ) )
     	wp_notify_postauthor($comment_ID, isset( $commentdata['comment_type'] ) ? $commentdata['comment_type'] : '' );
    
  1. A similar check would need to be added to wp_notify_postauthor in pluggable.php to prevent it from returning in the case of multiple recipients:
    $recipients = 0;
    $recipients = count( apply_filters( 'comment_notification_recipients', $recipients, $comment_id ) );
    ...
    ...
        // The comment was left by the author
        if ( $comment->user_id == $post->post_author && $recipients == 0 )
            return false;
    

Finally, the question: If there are multiple recipients and the comment is from the post author, should the post author be removed from the list of recipients? This could actually be done in the email loop by simply skipping that one email address:

    foreach ( $emails as $email ) {
        if ( $email != $author->user_email || $comment->user_id != $post->post_author ) {
             @wp_mail( $email, $subject, $notify_message, $message_headers );
        }
    }

Personally, I would include the author in the case of multiple recipients, if for no other reason than to alert the author that notifications were sent, not to mention it would be a little less code to mess with.

Last edited 11 years ago by DaveAl (previous) (diff)

@ethitter
11 years ago

#3 @ethitter
11 years ago

  • Cc erick@… added

25699.patch allows wp_notify_postauthor() to send an email for a comment by the post author. The logic to check if an email should be sent is moved entirely into wp_notify_postauthor(), rather than checking in both wp_new_comment() and wp_notify_postauthor() as was done previously.

To do so, I moved the comment_notification_recipients filter towards the top of wp_notify_postauthor(), so we can bail as early as possible if only the post author would be notified, or no recipients are set. The recipients array is still subjected to various checks that previously caused the function to return false: the author of the comment is the post author, the post author moderated his/her own comment, or the post author can no longer read the post.

As noted above, the post author still isn't notified for his/her own comment, but any additional addresses will receive the notifications for post author's comments. New filters allow the post author to receive a notification if desired; while these filters can bypass the capabilities check, the filter would already allow a user to do so, so the permissions burden has always fallen to the individuals using the provided filters.

@ethitter
11 years ago

@markjaquith
11 years ago

some cleanup

@ethitter
11 years ago

#4 @ethitter
11 years ago

25699.4.patch cleans up the patch a bit further, correcting the phpdoc implementation, simplifying the new author notification override filter, and using the comment type from the comment object already available within wp_notify_postauthor().

#5 @ethitter
11 years ago

  • Keywords has-patch added

@ethitter
11 years ago

#6 @ethitter
11 years ago

25699.5.patch refreshes for the changes made in r26358.

#7 @markjaquith
11 years ago

  • Owner set to markjaquith
  • Resolution set to fixed
  • Status changed from new to closed

In 26367:

Fix comment_notification_recipients filter behavior so that it is still respected even on comments left by the post author

The code was bailing on this-is-a-comment-on-your-own-post detection, ignoring additional recipients. Now:

  • Logic check is done within wp_notify_postauthor()
  • Logic check is overridable via comment_notification_notify_author filter (default still false)
  • The code doesn't bail on comment-on-own-post detection, but just removes the author from the array
  • The code instead now bails if the recipients list is empty, so comment_notification_recipients works properly

props ethitter.
fixes #25699

#8 @nacin
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

We should document these filters. :-)

We can also remove any generic @uses references. They are of limited use (it's easy to parse raw function calls for any future code reference site and it is of limited benefit anyway). Things that are unusual and/or interesting can stay.

#9 @markjaquith
11 years ago

Looking at the inline documentation handbook, it says we shouldn't use @uses at all. That's certainly easier than determining "unusual and/or interesting". Agree that it is of limited benefit.

@markjaquith
11 years ago

Inline docs changes.

#10 @markjaquith
11 years ago

There. Docs for the two hooks (3.7- and 3.8-added), and removal of @uses blocks that were inserted, plus existing ones.

#11 @DrewAPicture
11 years ago

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

In 26388:

Inline documentation for the following filter hooks in wp-includes/pluggable.php:

  • comment_notification_recipients
  • comment_notification_notify_author

Also removes some generic @uses tags from various related doc blocks.

Props markjaquith.
Fixes #25699.

#12 @chriscct7
10 years ago

#16995 was marked as a duplicate.

Note: See TracTickets for help on using tickets.