WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 years ago

#33735 reviewing enhancement

Reduce Duplication and Improve Comment Notification Email Functions

Reported by: dshanske Owned by: SergeyBiryukov
Milestone: Future Release Priority: low
Severity: normal Version:
Component: Comments Keywords: has-patch needs-refresh needs-unit-tests
Focuses: Cc:

Description

Had touched on this in #33587.

wp_notify_postauthor and wp_notify_moderator have some duplicative code that could be eliminated and simplified.

The functions for notification also lack a filter similar to the one for displaying the comment text.

Proposing the function to show the comment in text form in the notification be separated out into its own function with a filter, and the default text be improved somewhat.

Attachments (5)

33735.diff (8.2 KB) - added by dshanske 3 years ago.
33735.2.diff (8.6 KB) - added by dshanske 3 years ago.
Updated Patch
33735.3.diff (9.4 KB) - added by dshanske 3 years ago.
Address issue in simplest manner possible.
33735.4.diff (9.4 KB) - added by dshanske 3 years ago.
Correction of Typo
33735.4.2.diff (9.4 KB) - added by dshanske 3 years ago.
Fix reference pointed out by @wonderboymusic

Download all attachments as: .zip

Change History (29)

#1 @johnbillion
3 years ago

  • Keywords needs-patch good-first-bug added
  • Priority changed from normal to low

#2 @dshanske
3 years ago

In order to figure out what would be an improvement, I have to ask what people think. The basic text notification of a comment being received hasn't changed as long as I can remember.

The current display doesn't take into account custom comment types, alternative displays(such as the get_comment_text filter), etc.

The current presentation also has a bug. The moderator version refers to a Trackback/Pingback excerpt, whereas the postauthor version uses Comment for these type.

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


3 years ago

@dshanske
3 years ago

#4 @dshanske
3 years ago

  • Keywords has-patch added; needs-patch removed

#5 @dshanske
3 years ago

Added a patch that has the comment notification text in its own function, with filter. It also uses the WordPress retrieval functions for parts of the comment, as they have their own filters.

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


3 years ago

#7 @dshanske
3 years ago

Since this comes out of my annoyance at having to rewrite an entire function to support custom comment types and/or comment meta I wanted in the notification, knowing that might explain what started this.

#8 @kitchin
3 years ago

Great. Notes:

  • Any reason you're passing $comment_id instead of $comment ?
  • Can delete $comment_author_domain = @gethostbyaddr($comment->comment_author_IP); at 14346 and 1570 in the old code.
  • Need to sprintify $notify_message .= __( 'You can see all comments on this post here:' ) . "\r\n";

@dshanske
3 years ago

Updated Patch

#9 @dshanske
3 years ago

Applied the suggested changes. I originally didn't sprintify the comment line as it provides the same link to the entire comment section regardless, but it would be consistent with the old behavior.

But re $comment vs $comment_id, that was intentionally. I'm proposing switching to using get_comment_text, get_comment_author_link, etc. All of which have filters on them, as opposed to accessing the comment data directly. This opens the door to a variety of enhancements that plugins may already have enabled for this code. They all take $comment_id as a parameter.

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


3 years ago

#11 @kitchin
3 years ago

Makes sense. Not sure how to get traction on committing, but it looks very reasonable and useful to me.

#12 @dshanske
3 years ago

  • Component changed from Mail to Comments

I think this is probably better classified as a Comment, not a Mail issue.

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


3 years ago

#14 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 4.4

#15 @SergeyBiryukov
3 years ago

Unfortunately, sprintf( __( 'New %1s on your post "%2s"' ), get_comment_type( $comment_id ), ... ) is not translatable, see ticket:17609:3 and ticket:19099:1. The comment type should remain in the string.

Same for sprintf( __( 'You can see all %s on this post here:' ), get_comment_type( $comment_id ) ).

Perhaps moving switch ( $comment->comment_type ) with the corresponding strings to get_comment_notify_text() would work.

@dshanske
3 years ago

Address issue in simplest manner possible.

@dshanske
3 years ago

Correction of Typo

#16 @SergeyBiryukov
3 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

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


3 years ago

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


3 years ago

#19 @wonderboymusic
3 years ago

  • Keywords needs-refresh added

I can tell just by reading the code that this hasn't been tested. Variables being accessed that don't exist, etc

#20 @dshanske
3 years ago

Admit I'm something of an amateur at writing as opposed to simply reading code. I think, looking at it, I missed generating $comment, which was in the original code. Other than that, curious what I missed.

#21 @wonderboymusic
3 years ago

$comment is used on the first line of get_comment_notify_text() ... there is no $comment variable. I stopped looking there.

@dshanske
3 years ago

Fix reference pointed out by @wonderboymusic

#22 @wonderboymusic
3 years ago

  • Milestone changed from 4.4 to Future Release

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


3 years ago

#24 @rachelbaker
2 years ago

  • Keywords needs-unit-tests added; good-first-bug removed

@dshanske In your latest 33735.4.2.diff you went back to using $comment_id as the parameter for get_comment_notify_text() instead of passing in $comment. Right now you are doing additional un-needed look-ups. This would also benefit from unit tests to make sure your patch works as intended.

Note: See TracTickets for help on using tickets.