Make WordPress Core

Opened 6 years ago

Last modified 4 months ago

#16979 new enhancement

Extra hooks needed in comment process

Reported by: dd32 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Comments Keywords: needs-patch good-first-bug
Focuses: Cc:


I'm running into a few commenting issues whilst building a plugin with a custom post type..

  • Duplicate comment check's cannot be bypassed
  • Empty comments are not allowed (In this case, the comment body is empty whilst a set of custom fields acting as metadata are set, meaning, the plugin wants to accept that comment)
  • being able to override the wp_die() in the commenting process would be useful (Currently: Duplicate comments, User not logged in, and not all fields filled in will cause this)

One potential solution would be to move lines 55-84 from wp-comments-post.php to functions hooked to preprocess_comment

Attachments (2)

16979.diff (6.0 KB) - added by dd32 6 years ago.
comment.diff (9.9 KB) - added by sgrant 22 months ago.
Updated patch from @dd32 and added unit tests.

Download all attachments as: .zip

Change History (20)

#1 @aaroncampbell
6 years ago

I ran into the empty comment issue quite a while ago. I think what I ultimately did was make that field a hidden field and populate it with an md5 hash. Actually, I'm pretty sure I populated it with something static until I ran into the duplicate comment issue. It would be nice to make this a little more flexible.

Are you thinking three different functions? One for user logged in, one for email/author required, and one for empty comments?

#2 @dd32
6 years ago

Actually, I'm pretty sure I populated it with something static until I ran into the duplicate comment issue.

I'm appending a <!-- comment --> with a hash in it, that works around the issue, I was planning on striping it out before it went to the database, but there were no filters between the duplicate check and the DB insert either!

In this case, I'd probably do them as 3 functions, one for duplicate comment, one for blank comment, and one for the userloggedin & required fields not set

Last edited 6 years ago by dd32 (previous) (diff)

#3 @solarissmoke
6 years ago

  • Cc solaris.smoke@… added

#4 @dd32
6 years ago

Working through implementing some changes here.

It seems that none of the functions involved here are designed with error handling in mind at all, Every function, be it processing or Inserting is designed to die in event of error rather than to handle it and pass it to a common error handler..

Returning an error code from wp_new_comment() appears to be out, this function has been used in the wild (plugins) a fair bit without any error handling being present unfortunately.

#5 @dd32
6 years ago

returning WP_Error objects through preprocess_comment is also not very friendly to existing plugins.. For now it looks like this is going to simply do (wp_)?die's as it currently does..

6 years ago

#6 @dd32
6 years ago

attachment 16979.diff added

First attempt at moving the ones that Impacted me to hooks.

For example, If you'd like to allow Empty comments, and do not want to prevent duplicate comments from being posted:

	add_action('pre_comment_on_post', array(&$this, 'allow_blank_comments'));

	function allow_blank_comments( $post_id ) {
		if ( 'my_cpt' == get_post_type($post_id) ) {
			remove_filter( 'preprocess_comment', 'disallow_empty_comments', 5);
			remove_action( 'check_comment_flood', 'check_comment_flood_duplicate', 10, 4 );

This doesn't attempt to refactor the entire commenting process to handle passing error objects around, or to allow the theme to handle these errors by displaying that inline. I would prefer the comment process to be refactored, but doing so will require a bit more work than I've patched here.

Additionally, not in the patch, is the fact that this moves the Empty comment check to within the comment processing code, which will cause a problem for XML-RPC as it currently allows blank comments. (It duplicates a few checks to avoid wp_die()'s).

if ( !isset($content_struct['content']) || '' == $content_struct['content'] )
	return new IXR_Error( 403, __('Comment content is required') );

#7 @dd32
6 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

#8 @dd32
6 years ago

  • Keywords dev-feedback added; 2nd-opinion removed

Just after a bit of feedback from other devs on the line of attack taken here

#9 @solarissmoke
6 years ago

Perhaps we might add a $return_wp_error argument (defaults to false) to the new functions you're proposing, offering the option to return an error instead of wp_dying? That way plugins (as well as things like the xml-rpc handler) could still use them to do the logic part, instead of having to replicate them.

(At least for the comment flood/duplicate checks, the empty comment check is simple enough to replicate)

Last edited 6 years ago by solarissmoke (previous) (diff)

#10 @SergeyBiryukov
5 years ago

  • Keywords needs-refresh added

#11 @mdgl
2 years ago

Patch uploaded to #18630 that allows the call to wp_die() in the commenting process to be overridden (third item in the ticket description).

#13 @SergeyBiryukov
2 years ago

#30259 was marked as a duplicate.

#14 @ocean90
22 months ago

#32981 was marked as a duplicate.

#15 @sgrant
22 months ago

Hiya! I come from #32981. Took a crack at bringing this patch back from the old versions, and added some unit tests. The diff is attached. Any chance somebody could take a look when they have time? Thanks!

22 months ago

Updated patch from @dd32 and added unit tests.

#17 @rachelbaker
7 months ago

  • Keywords dev-feedback removed

Duplicate comment check can now be bypassed with duplicate_comment_id filter. Introduced in [34536].
Removing wp_die() is being done in #36901.

All that is left to address here is allowing a filter to override the WP_Error that is returned in wp_handle_comment_submission() when a comment is empty.

#18 @boonebgorges
4 months ago

  • Keywords needs-patch good-first-bug added; has-patch needs-refresh removed

I've run into this yet again. Let's get the empty check filtered or broken out.

Note: See TracTickets for help on using tickets.