Make WordPress Core

Opened 13 years ago

Closed 5 years ago

#16979 closed enhancement (fixed)

Extra hooks needed in comment process

Reported by: dd32's profile dd32 Owned by: jpurdy647's profile jpurdy647
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch needs-refresh
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 (4)

16979.diff (6.0 KB) - added by dd32 13 years ago.
comment.diff (9.9 KB) - added by sgrant 9 years ago.
Updated patch from @dd32 and added unit tests.
0001-Add-filter-require_valid_content-to-bypass-WP-Error-.patch (1.0 KB) - added by jpurdy647 7 years ago.
Patch to allow filter to bypass wP Error caused by an empty comment
16979.2.diff (1.2 KB) - added by boonebgorges 7 years ago.

Download all attachments as: .zip

Change History (35)

#1 @aaroncampbell
13 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
13 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 13 years ago by dd32 (previous) (diff)

#3 @solarissmoke
13 years ago

  • Cc solaris.smoke@… added

#4 @dd32
13 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
13 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..

13 years ago

#6 @dd32
13 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
13 years ago

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

#8 @dd32
13 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
13 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.

Version 0, edited 13 years ago by solarissmoke (next)

#10 @SergeyBiryukov
12 years ago

  • Keywords needs-refresh added

#11 @mdgl
10 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
9 years ago

#30259 was marked as a duplicate.

#14 @ocean90
9 years ago

#32981 was marked as a duplicate.

#15 @sgrant
9 years 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!

9 years ago

Updated patch from @dd32 and added unit tests.

#17 @rachelbaker
8 years 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
7 years 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.

7 years ago

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

#19 @jpurdy647
7 years ago

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.

#20 @jpurdy647
7 years ago

  • Keywords has-patch added; needs-patch removed

#21 @DrewAPicture
7 years ago

  • Owner set to jpurdy647
  • Status changed from new to assigned

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

7 years ago

#22 @boonebgorges
7 years ago

  • Keywords dev-feedback added
  • Milestone changed from Future Release to 5.0

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")

#23 @jpurdy647
7 years ago

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

#24 @boonebgorges
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#25 @boonebgorges
6 years ago

In 42662:

Improve parameter description for 'allow_empty_comments'.

Introduced in [42661].

Props swissspidy.
See #16979.

#26 @johnbillion
6 years ago

  • Keywords good-first-bug dev-feedback removed
  • Milestone changed from 5.0 to 5.0.1
  • Resolution fixed deleted
  • Status changed from closed to reopened

@since needs updating

#27 @pento
6 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#28 @pento
6 years ago

  • Milestone changed from 5.0.2 to 5.0.3

#29 @audrasjb
5 years ago

  • Keywords needs-refresh added


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.

#30 @boonebgorges
5 years ago

  • Milestone changed from 5.0.3 to 5.1

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.

#31 @boonebgorges
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 44364:

Update since annotation on allow_empty_comment hook.

Originally introduced in [42661].

Fixes #16979.

Note: See TracTickets for help on using tickets.