WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 years 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: has-patch dev-feedback needs-refresh
Focuses: Cc:

Description

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 (1)

16979.diff (6.0 KB) - added by dd32 3 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 aaroncampbell3 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?

comment:2 dd323 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 3 years ago by dd32 (previous) (diff)

comment:3 solarissmoke3 years ago

  • Cc solaris.smoke@… added

comment:4 dd323 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.

comment:5 dd323 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..

dd323 years ago

comment:6 dd323 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') );

comment:7 dd323 years ago

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

comment:8 dd323 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

comment:9 solarissmoke3 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 3 years ago by solarissmoke (previous) (diff)

comment:10 SergeyBiryukov2 years ago

  • Keywords needs-refresh added
Note: See TracTickets for help on using tickets.