Make WordPress Core

Opened 11 years ago

Last modified 14 months ago

#23931 new defect (bug)

wp_insert_comment should require comment_post_ID

Reported by: markoheijnen's profile markoheijnen Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.0
Component: Comments Keywords: needs-patch close
Focuses: Cc:

Description (last modified by markoheijnen)

At this moment there is no check for example comment_post_ID. Not sure if there are more checks needed. Reason I asked are a few notices on the unit tests caused by WP_UnitTest_Factory_For_Comment. Those comments don't add a post ID what should change.

Currently, if a null comment_post_ID is passed, the comment isn't connected to a post. This can create confusion. Also Unit tests should run with WP_Debug on.

Attachments (6)

comment.php.1.diff (1.9 KB) - added by westonruter 11 years ago.
Simplify construction of commentdata array for insertion with defaults
tests-comment-query.1.php (1.2 KB) - added by westonruter 11 years ago.
Update unit tests to always create comments with a valid comment_post_ID
comment.php.2.diff (2.5 KB) - added by westonruter 11 years ago.
Return WP_Error from wp_insert_comment if invalid comment_post_ID inserted
tests-comment-query.2.php (2.5 KB) - added by westonruter 11 years ago.
Also add assertions for unit tests to check for returned WP_Error instance when invalid comment_post_ID is supplied, and test the wp_insert_comment_data filter
23931.diff (5.2 KB) - added by westonruter 10 years ago.
Refreshed patch. PR: https://github.com/x-team/wordpress-develop/pull/25
23931.1.diff (7.5 KB) - added by valendesigns 9 years ago.

Download all attachments as: .zip

Change History (24)

#1 @markoheijnen
11 years ago

  • Description modified (diff)

@westonruter
11 years ago

Simplify construction of commentdata array for insertion with defaults

#2 @westonruter
11 years ago

  • Cc weston@… added
  • Keywords has-patch added; needs-patch removed

By refactoring the way that the commentdata is prepared for insertion into the DB, it now merges the passed-in data on top of a defaults array that includes a comment_post_ID, so there will be no undefined index.

Patch also includes a wp_insert_comment_data filter before insert to accompany the wp_insert_comment action which happens after insert.

See first attached patch, or see from branch on GitHub: https://github.com/x-team/WordPress/compare/87f9d11...7588a21

Last edited 11 years ago by westonruter (previous) (diff)

@westonruter
11 years ago

Update unit tests to always create comments with a valid comment_post_ID

@westonruter
11 years ago

Return WP_Error from wp_insert_comment if invalid comment_post_ID inserted

@westonruter
11 years ago

Also add assertions for unit tests to check for returned WP_Error instance when invalid comment_post_ID is supplied, and test the wp_insert_comment_data filter

#4 @westonruter
11 years ago

BTW, the need for a wp_insert_comment_data filter came up while doing work on the Liveblog plugin: https://github.com/x-team/wp-liveblog/commit/94958de51f786e4eed6389cc0c3124fcdb8c5c29#L4R458

#5 @jdgrimes
11 years ago

Something does need to happen here. At the very least, the tests should set comment_post_ID to 0 by default. That will keep it from erroring. If we're going to force comment_post_ID to be valid though, we should make WP_UnitTest_Factory_For_Comment automatically create a post if comment_post_ID isn't set. I think that makes more sense then having to create one each time like in your patch.

#6 @jdgrimes
11 years ago

  • Cc jdg@… added

#7 @wonderboymusic
10 years ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to Future Release

Please combine/refresh the patches for src/tests.

#8 @westonruter
10 years ago

  • Keywords needs-refresh removed

#9 @SergeyBiryukov
9 years ago

#30944 was marked as a duplicate.

#10 @SergeyBiryukov
9 years ago

  • Milestone changed from Future Release to 4.2

#12 @DrewAPicture
9 years ago

  • Keywords needs-refresh added

Latest patch needs a refresh.

@valendesigns
9 years ago

#13 @valendesigns
9 years ago

  • Keywords needs-testing dev-feedback added; needs-refresh removed

I've refreshed the patch, but there were some test failures that needed to be addressed after the update. 3 tests failed due to a WP_Error being returned and 1 failed asserting that an array is not empty, because you can't create a comment without a comment_post_ID anymore. I added the ID and all tests pass again.

#14 @rachelbaker
9 years ago

  • Milestone changed from 4.2 to Future Release

Punting to future release, this needs more discussion before being ready for a commit.

#15 @boonebgorges
9 years ago

  • Keywords needs-patch added; has-patch needs-testing dev-feedback removed

We can't start returning WP_Error objects from wp_insert_comment(). It'll break any plugin doing something like

$comment_id = wp_insert_comment( $args );
if ( ! $comment_id ) {
    $weep = true;
}

If we want to have some more elegant and informative error handling, we'll need to do it in a new function, and turn wp_insert_comment() into a wrapper for it.

Two other possible courses of action: (1) Simply return false when no comment_post_ID is present. (2) Do nothing, and leave it as-is, since it doesn't look like there's a functional bug here.

#16 @rachelbaker
8 years ago

  • Keywords close added

There is no reason why a Comment must have a relationship with a Post. I see this as a wontfix.

#17 @markoheijnen
8 years ago

Currently comments have a relationship with a post. All functions are there to retrieve comments based on the post ID. So closing this ticket without doing nothing feels wrong to me.

#18 @costdev
14 months ago

I'm inclined to agree with @rachelbaker here. While most cases involve a comment with a relationship to a post, it's entirely possible that the desired behaviour is that a comment could be inserted without having a relationship with a post.

Some thoughts:

  • "Comments" is its own menu item. It's not nested under "Posts", "Pages", or both.
  • Most comment functions that deal with a post ID are within comment-template.php, which is specifically for use within the loop.
  • For comments that are post-independent, a relationship to a post is not desired. If a relationship to a post was required by WP, deleting that post would also delete its comments.
  • As an example, a contact form can be created by adding a form to a page that submits comments. This doesn't need a plugin that uses custom post types and such, as the Comments API provides all that's needed (email, custom comment types, etc). For this type of available usage, there's no need for a relationship to a post.
Note: See TracTickets for help on using tickets.