WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 9 months 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-unit-tests dev-feedback
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 (7)

33735.diff (8.2 KB) - added by dshanske 4 years ago.
33735.2.diff (8.6 KB) - added by dshanske 4 years ago.
Updated Patch
33735.3.diff (9.4 KB) - added by dshanske 4 years ago.
Address issue in simplest manner possible.
33735.4.diff (9.4 KB) - added by dshanske 4 years ago.
Correction of Typo
33735.4.2.diff (9.4 KB) - added by dshanske 4 years ago.
Fix reference pointed out by @wonderboymusic
33735.5.diff (10.3 KB) - added by dshanske 16 months ago.
Refreshed patch
33735.6.diff (11.2 KB) - added by dshanske 16 months ago.

Download all attachments as: .zip

Change History (40)

#1 @johnbillion
4 years ago

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

#2 @dshanske
4 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.


4 years ago

@dshanske
4 years ago

#4 @dshanske
4 years ago

  • Keywords has-patch added; needs-patch removed

#5 @dshanske
4 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.


4 years ago

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

Updated Patch

#9 @dshanske
4 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.


4 years ago

#11 @kitchin
4 years ago

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

#12 @dshanske
4 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.


4 years ago

#14 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 4.4

#15 @SergeyBiryukov
4 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
4 years ago

Address issue in simplest manner possible.

@dshanske
4 years ago

Correction of Typo

#16 @SergeyBiryukov
4 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.


4 years ago

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


4 years ago

#19 @wonderboymusic
4 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
4 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
4 years ago

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

@dshanske
4 years ago

Fix reference pointed out by @wonderboymusic

#22 @wonderboymusic
4 years ago

  • Milestone changed from 4.4 to Future Release

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


4 years ago

#24 @rachelbaker
3 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.

#25 @dshanske
16 months ago

I think I'm going to get back to this and redo it completely.

@dshanske
16 months ago

Refreshed patch

#26 follow-up: @dshanske
16 months ago

  • Keywords needs-refresh removed
  • Milestone changed from Future Release to 5.0

I refreshed this. I still, two years later, can't figure out how to unit test this. Always been a bit of a hard issue for me. I've manually tested this patch. But I can't find unit tests for the original wp_notify functions to based it on.

I'd like to get this in after all this time.

#27 in reply to: ↑ 26 @birgire
16 months ago

Replying to dshanske:

I refreshed this. I still, two years later, can't figure out how to unit test this. Always been a bit of a hard issue for me. I've manually tested this patch. But I can't find unit tests for the original wp_notify functions to based it on.

I'd like to get this in after all this time.

Great. The patch in pluggable.php does not apply cleanly with trunk, for me though:

patching file src/wp-includes/comment-template.php
Hunk #1 succeeded at 2536 (offset 71 lines).
patching file src/wp-includes/pluggable.php
Hunk #1 FAILED at 1452.
Hunk #2 FAILED at 1618.
Hunk #3 FAILED at 1670.
3 out of 3 hunks FAILED -- saving rejects to file src/wp-includes/pluggable.php.rej

ps: I could try to look at the unit tests.

#28 @dshanske
16 months ago

@birgire That's odd. It did for me. It has been a while since I did a patch as I've been using git. Maybe I need to refresh my development environment. I'll try redoing it again tonight.

@dshanske
16 months ago

#29 @dshanske
16 months ago

@birgire Should apply cleanly now. I refreshed the entire environment.

#30 @birgire
16 months ago

Thanks for the refresh @dshanske, that applies cleanly now.

Regarding 33735.6.diff:

  • As get_comment_notify_text() is for internal use, what about _wp_get_comment_notify_text() with the private _ prefix?
  • There's a typo in get_comment_notify_text() with $comment_id instead of $comment.

Some general remarks that comes to mind:

  • The get_comment_notify_text() function modifies some of the older strings.
  • Possible confusion between the existing filter comment_notification_text and the new filter comment_notify_text?
  • The new comment_notify_text filter applies to only part of the notify message and the get_comment_notify_text() function outputs part of the notify message. Would it be possible/suitable to have the function handle all of the notify message?

Do you see these as a concern here? If so how could these be addressed?

#31 @dshanske
16 months ago

I can fix the $comment_id. But before I do, want to look at the other ones before I refresh

  • The older strings aren't consistent. Why is the moderation text different than the notification text? The information provided should be the same.
  • Any ideas for the name? I'm not tied to the name of the filter
  • No, it shouldn't handle the entire message. The purpose of the ticket is to split the duplicative parts of the two functions. The details of the comment, pingback, etc are duplicated. As the difference between the moderation and the notification message is the top and bottom portion.

#32 @pento
11 months ago

  • Milestone changed from 5.0 to 5.1

#33 @pento
9 months ago

  • Keywords dev-feedback added
  • Milestone changed from 5.1 to Future Release

This ticket needs reviewing.

Note: See TracTickets for help on using tickets.