WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#25699 closed defect (bug) (fixed)

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

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

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 6 years ago.
25699.2.patch (7.6 KB) - added by ethitter 6 years ago.
25699.3.patch (7.3 KB) - added by markjaquith 6 years ago.
some cleanup
25699.4.patch (5.9 KB) - added by ethitter 6 years ago.
25699.5.patch (5.9 KB) - added by ethitter 6 years ago.
25699.inline-docs.diff (2.6 KB) - added by markjaquith 6 years ago.
Inline docs changes.
25699.6.patch (2.6 KB) - added by DrewAPicture 6 years ago.

Download all attachments as: .zip

Change History (19)

#1 @nacin
6 years ago

  • Milestone changed from Awaiting Review to 3.8

#2 in reply to: ↑ description @DaveAl
6 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 6 years ago by DaveAl (previous) (diff)

@ethitter
6 years ago

#3 @ethitter
6 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
6 years ago

@markjaquith
6 years ago

some cleanup

@ethitter
6 years ago

#4 @ethitter
6 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
6 years ago

  • Keywords has-patch added

@ethitter
6 years ago

#6 @ethitter
6 years ago

25699.5.patch refreshes for the changes made in r26358.

#7 @markjaquith
6 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
6 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
6 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
6 years ago

Inline docs changes.

#10 @markjaquith
6 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
6 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
5 years ago

#16995 was marked as a duplicate.

Note: See TracTickets for help on using tickets.