WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 4 months ago

#23931 new defect (bug)

wp_insert_comment should require comment_post_ID

Reported by: markoheijnen Owned by:
Milestone: Future Release 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 3 years ago.
Simplify construction of commentdata array for insertion with defaults
tests-comment-query.1.php (1.2 KB) - added by westonruter 3 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 3 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 3 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 2 years ago.
Refreshed patch. PR: https://github.com/x-team/wordpress-develop/pull/25
23931.1.diff (7.5 KB) - added by valendesigns 14 months ago.

Download all attachments as: .zip

Change History (23)

#1 @markoheijnen
3 years ago

  • Description modified (diff)

@westonruter
3 years ago

Simplify construction of commentdata array for insertion with defaults

#2 @westonruter
3 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 3 years ago by westonruter (previous) (diff)

@westonruter
3 years ago

Update unit tests to always create comments with a valid comment_post_ID

@westonruter
3 years ago

Return WP_Error from wp_insert_comment if invalid comment_post_ID inserted

@westonruter
3 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
3 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
3 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
3 years ago

  • Cc jdg@… added

#7 @wonderboymusic
2 years ago

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

Please combine/refresh the patches for src/tests.

#8 @westonruter
2 years ago

  • Keywords needs-refresh removed

#9 @SergeyBiryukov
17 months ago

#30944 was marked as a duplicate.

#10 @SergeyBiryukov
17 months ago

  • Milestone changed from Future Release to 4.2

#12 @DrewAPicture
15 months ago

  • Keywords needs-refresh added

Latest patch needs a refresh.

#13 @valendesigns
14 months 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
14 months 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
8 months 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
4 months 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
4 months 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.

Note: See TracTickets for help on using tickets.