Reduce Duplication and Improve Comment Notification Email Functions

[dshanske]

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.

[dshanske]

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.

[dshanske]

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.

[dshanske]

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.

[kitchin]

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]

Updated Patch

[dshanske]

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.

[kitchin]

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

[dshanske]

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

[SergeyBiryukov]

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]

Address issue in simplest manner possible.

[dshanske]

Correction of Typo

[wonderboymusic]

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

[dshanske]

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.

[wonderboymusic]

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

[dshanske]

Fix reference pointed out by @wonderboymusic

[rachelbaker]

@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.

[dshanske]

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

[dshanske]

Refreshed patch

[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.

[birgire]

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.

[dshanske]

@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]

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

[birgire]

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?

[dshanske]

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.

[pento]

This ticket needs reviewing.

[dshanske]

I haven't done much Core work in a while, but wanted to revisit a few of my failed attempts. I tried to address the issues here.

The goal of this is multi-faceted and remains unchanged. wp_notify_moderator and wp_notify_postauthor are long and duplicative. I tried to address the old feedback by @birgire but I think I need some more people looking at this mater.

A few issues are noticeable regardless of complexity. In order to add more data, such as comment meta, to the email, you have to completely rewrite the output from both functions. In order to cover custom comment types, accepting such things aren't officially supported in core...you have to rewrite the entire output from both functions.

I'd love to solve this in future by supporting comment type registration, with callbacks for each function like this, but there is no extensive work being done on comments right now. This addresses the issue.

The code in question also adds in calls to the existing comment filters, which allows for additional flexibility if you are already filtering comment text or such,

[davidbaumwald]

This ticket needs a refreshed patch and unit tests. With 5.4 Beta 1 approaching, this is being moved to Future Release. If any maintainer or committer feels this can be worked in before 5.4 Beta 1 or can assume ownership during a specific cycle, feel free to update the milestone accordingly.

[hellofromTonya]

From scrub today, moving this ticket to a Future Release. Why? There's been no activity. 5.7 Beta 1 in less than 6 days.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.