Extra hooks needed in comment process

[dd32]

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

[aaroncampbell]

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?

[dd32]

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

[dd32]

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.

[dd32]

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

[dd32]

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') );

[dd32]

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

[solarissmoke]

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)

[mdgl]

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

[SergeyBiryukov]

Related: #4332

[SergeyBiryukov]

#30259 was marked as a duplicate.

[ocean90]

#32981 was marked as a duplicate.

[sgrant]

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!

[sgrant]

Updated patch from @dd32 and added unit tests.

[rachelbaker]

Related #36901

[rachelbaker]

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.

[boonebgorges]

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

[jpurdy647]

Patch to allow filter to bypass wP Error caused by an empty comment

[jpurdy647]

Uploaded a patch to allow empty comments via a filter. Filter name currently is require_valid_comment.

Could be changed to require_valid_comment_content or require_comment_content as well. I just used the verbiage already present.

[DrewAPicture]

Assigning ownership to mark the good-first-bug as "claimed".

[boonebgorges]

16979.2.diff​ is a modification of @jpurdy647's previous patch. It renames the filter 'allow_empty_comment', which I think is more descriptive of what's really happening here. (I know the term "valid" is used in the WP_Error string, but it feels confusing to me in this context, since "valid" could mean any number of things.) I've also changed the position of the filter a bit so that it can receive the $commentdata for more fine-grained filtering.

Do others have thoughts about how specific this filter ought to be? ("empty" vs "valid")

[jpurdy647]

I like that, I did not consider the comment data and the new name is more indicative of the purpose of the filter.

[boonebgorges]

In 42661:

Comments: Introduce 'allow_empty_comment' filter.

This filter allows plugin authors to allow empty comments on a selective
basis during comment submission.

Props jpurdy647.
Fixes #16979.

[boonebgorges]

In 42662:

Improve parameter description for 'allow_empty_comments'.

Introduced in [42661].

Props swissspidy.
See #16979.

[johnbillion]

@since needs updating

[audrasjb]

Hi,

5.0.3 is going to be released in a couple of weeks. We are currently sorting the remaining tickets in the milestone. The ticket is sorted in milestone 5.0.3, but requires an @since refresh and a commit/backport to the associated branch. That would be nice to have a refreshed patch asap.
Thanks

[boonebgorges]

Given that we're moving back to more normal x.y.z releases, 5.1.0 seems like the more proper milestone for this ticket.

[boonebgorges]

In 44364:

Update since annotation on allow_empty_comment hook.

Originally introduced in [42661].

Fixes #16979.