WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 4 months ago

#16365 new enhancement

Comment transition for new comments

Reported by: MattyRob Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 3.1
Component: Comments Keywords: has-patch dev-feedback needs-refresh needs-docs
Focuses: Cc:

Description

As far as I can tell wp_transitions_comment_status() does not get called for new 'comments' based on my testing and review of comment.php in wp-includes.

There is a similar transition for posts that gets called for new 'posts' including hooks like 'new_to_publish' and 'new_to_private'.

I feel that there should be a similar hook to this form comments so that plugins can hook into new comments differently from comments moved from one existing status to another (like comment_unapproved_to_approved'.

Attachments (5)

16365.diff (584 bytes) - added by MattyRob 4 years ago.
16365v2.diff (555 bytes) - added by MattyRob 4 years ago.
16365v3.diff (591 bytes) - added by MattyRob 4 years ago.
16365v4.diff (1.0 KB) - added by MattyRob 4 years ago.
16365v5.diff (1.6 KB) - added by MattyRob 4 months ago.

Download all attachments as: .zip

Change History (29)

comment:1 @MattyRob4 years ago

  • Keywords has-patch needs-testing added

I think my attached patch will accomplish this. It adds to the wp_new_comment() function in the wp-includes/comment.php file.

It needs to cast the $commentdata array as an object to ensure the transition function works as expected. There may be a more elegant way of doing this but I've tested this patch and it works on my testing platform.

@MattyRob4 years ago

comment:2 @markjaquith4 years ago

  • Milestone changed from Awaiting Review to Future Release

Seems reasonable.

comment:3 @MattyRob4 years ago

If this "seems reasonable" is there any chance of it making 3.2?

comment:4 @MattyRob4 years ago

  • Keywords dev-feedback added; needs-testing removed

I've tested out my changes with a plugin containing the following code:

function comment_transition($commentdata) {
	// put something in here to email when comments are made and see if we can report the hook used somehow
	$recipient = '<my email address here>';
	$subject = 'New to Spam hook';
	$mailtext = 'The "comment_new_to_spam" hook has been called';
	@wp_mail($recipient, $subject, $mailtext);
}

add_action('comment_new_to_spam', 'comment_transition');

On every new spam made on my site I got an email.

Can we commit now please?

comment:5 @MattyRob4 years ago

The silence is deafening. How does one get traction on a ticket? Anyone, Please?!?

I've attached a one line patch also.

@MattyRob4 years ago

comment:6 follow-up: @ocean904 years ago

  • Keywords dev-feedback removed
  • Type changed from defect (bug) to enhancement

It's marked as Future Release and isn't a bug, so no, not for 3.2.

Also, please make your patch relative to the WordPress root dir.

comment:7 in reply to: ↑ 6 ; follow-up: @MattyRob4 years ago

Replying to ocean90:

It's marked as Future Release and isn't a bug, so no, not for 3.2.

I can see it's marked as a future release; it has been for 3 months. It's had a patch that works for 4 months and it's a single line change!

I know the core devs are busy but honestly when someone contributes code that improves the core you really know how to ignore people on here!

What about 3.3 then?

Also, please make your patch relative to the WordPress root dir.

How? Not sure what you mean. And is it likely to make any difference to the speed it gets committed?

comment:8 in reply to: ↑ 7 ; follow-ups: @nacin4 years ago

Replying to MattyRob:

I can see it's marked as a future release; it has been for 3 months. It's had a patch that works for 4 months and it's a single line change!

No one noticed a valid patch until after feature freeze. So it needs to wait.

I know the core devs are busy but honestly when someone contributes code that improves the core you really know how to ignore people on here!

This is one of more than two thousand tickets. I alone make somewhere around 600 comments a month to Trac. It's not like I'm deliberately avoiding your patches.

What about 3.3 then?

Sure, maybe.

How? Not sure what you mean. And is it likely to make any difference to the speed it gets committed?

Yes, actually. There's at least three different comment.php files. Which one is this? I'm not inclined to guess. Diff from the root, rather than from the directory.

So here's why I said maybe regarding 3.3, and definite no regarding 3.2: I'm not sure I like it. wp_transition_comment_status() is for transitioning a comment status from one to another, not for the comment creation to begin with. This might cause backwards compatibility issues due to the generic hooks in wp_transition_comment_status(). It should possibly be a new hook.

comment:9 in reply to: ↑ 8 @MattyRob4 years ago

Replying to nacin:

Replying to MattyRob:

I know the core devs are busy but honestly when someone contributes code that improves the core you really know how to ignore people on here!

This is one of more than two thousand tickets. I alone make somewhere around 600 comments a month to Trac. It's not like I'm deliberately avoiding your patches.

As I said - I know you are busy and this was not a personal attack on you. But, this ticket was commented on when it was new, moved from pending review to future release and noted as 'seems reasonable' - and then apparently ignored!

Yes, actually. There's at least three different comment.php files. Which one is this? I'm not inclined to guess. Diff from the root, rather than from the directory.

So here's why I said maybe regarding 3.3, and definite no regarding 3.2: I'm not sure I like it. wp_transition_comment_status() is for transitioning a comment status from one to another, not for the comment creation to begin with. This might cause backwards compatibility issues due to the generic hooks in wp_transition_comment_status(). It should possibly be a new hook.

So are you saying you don't think this is reasonable? The is a new_to_{new_status} hook for posts already in core - has been for ages. I cannot imagine of a scenario where this might cause an issue.

But, hey I'm all ears. If this doesn't get committed I will not be able to add a new feature to my plugin because I'm certainly not going to write reams of code to check comment transition status from 'new' to approved when I can already simply hook into unapproved to approved.

New root referenced patch to follow, thanks for your time.

@MattyRob4 years ago

comment:10 @MattyRob4 years ago

Root reference patch updated so that is also updates the inline documentation.

@MattyRob4 years ago

comment:11 in reply to: ↑ 8 @MattyRob4 years ago

  • Keywords dev-feedback added
  • Severity changed from normal to minor

Replying to nacin:

So here's why I said maybe regarding 3.3, and definite no regarding 3.2: I'm not sure I like it. wp_transition_comment_status() is for transitioning a comment status from one to another, not for the comment creation to begin with. This might cause backwards compatibility issues due to the generic hooks in wp_transition_comment_status(). It should possibly be a new hook.

I've been thinking about this. Compare and contrast the wp_transition_post_status() function with the wp_transition_comment_status() function.

Apart from some code that renames some instances of comment statuses that are inconsistently implemented through the code (like approved being the same a 1 and unapproved being the same as 0 and hold) the do_action hook calls are all virtually identical to the post transition calls.

I have had this patch running on my 3.1.3 main site for the last 3 weeks. I've had over 100 comments made in this time - mainly spam but some genuine comments and also pingbacks. I have seen no unusual behaviours or issues that would lead me to think there are compatibility issues with other hooks but can you give me any indication as to what I could or should look for?

comment:12 @johnbillion3 years ago

  • Cc johnbillion added

comment:13 @jdgrimes2 years ago

  • Cc jdg@… added

comment:14 @eflister9 months ago

please add this! i need a plugin that can email subscribers about new comments, and it's waiting on this issue:
https://wordpress.org/support/topic/need-way-for-users-to-sign-up-for-email-noticedigests-of-all-comments-1?replies=3#post-6050890

comment:15 follow-up: @helen4 months ago

Is wp_insert_comment not sufficient for what's needed? There are definitely plugins that send notifications for all new comments.

comment:16 in reply to: ↑ 15 @DrewAPicture4 months ago

Replying to helen:

Is wp_insert_comment not sufficient for what's needed? There are definitely plugins that send notifications for all new comments.

I think there's a reasonable case to be made for consistency with how post transitions are handled. If you flip the argument on its head, shouldn't the wp_insert_post hook be enough for new posts? And yet we still use wp_transition_post_status() which affords use of those hooks for new posts.

comment:17 @SergeyBiryukov4 months ago

  • Milestone changed from Future Release to 4.2

comment:18 @slackbot4 months ago

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

comment:19 @slackbot4 months ago

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

comment:20 @DrewAPicture4 months ago

  • Keywords needs-refresh needs-docs added

As discussed in the above-linked Slack discussion, the positive consensus is that this would be worth considering as it more closely mimics the behavior of creating new posts. This with the caveat that comments don't get the auto-draft status.

The patch needs a refresh, and I would recommend adding a changelog entry to wp_transition_comment_status() DocBlock to mention that a new comment status, 'new', was added in 4.2.0. This would be with the implication that new permutations of the comment_{$old_status}_to_{$new_status} and comment_{$new_status}_{$comment->comment_type} hooks would also be in play. Happy to assist with those documentation additions.

comment:21 @MattyRob4 months ago

You mean something like the new patch to follow?

@MattyRob4 months ago

comment:22 @slackbot4 months ago

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

comment:23 @slackbot4 months ago

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

comment:24 @helen4 months ago

  • Milestone changed from 4.2 to Awaiting Review

There appears to be some prior art, but no real consensus about whether or not to introduce this right now, so punting.

Note: See TracTickets for help on using tickets.