WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 4 weeks 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: has-patch needs-testing dev-feedback
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 21 months ago.
Simplify construction of commentdata array for insertion with defaults
tests-comment-query.1.php (1.2 KB) - added by westonruter 21 months ago.
Update unit tests to always create comments with a valid comment_post_ID
comment.php.2.diff (2.5 KB) - added by westonruter 21 months 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 21 months 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 months ago.
Refreshed patch. PR: https://github.com/x-team/wordpress-develop/pull/25
23931.1.diff (7.5 KB) - added by valendesigns 5 weeks ago.

Download all attachments as: .zip

Change History (20)

comment:1 @markoheijnen2 years ago

  • Description modified (diff)

@westonruter21 months ago

Simplify construction of commentdata array for insertion with defaults

comment:2 @westonruter21 months 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 21 months ago by westonruter (previous) (diff)

@westonruter21 months ago

Update unit tests to always create comments with a valid comment_post_ID

@westonruter21 months ago

Return WP_Error from wp_insert_comment if invalid comment_post_ID inserted

@westonruter21 months 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

comment:4 @westonruter21 months 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

comment:5 @jdgrimes20 months 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.

comment:6 @jdgrimes20 months ago

  • Cc jdg@… added

comment:7 @wonderboymusic10 months ago

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

Please combine/refresh the patches for src/tests.

comment:8 @westonruter10 months ago

  • Keywords needs-refresh removed

comment:9 @SergeyBiryukov4 months ago

#30944 was marked as a duplicate.

comment:10 @SergeyBiryukov4 months ago

  • Milestone changed from Future Release to 4.2

comment:12 @DrewAPicture2 months ago

  • Keywords needs-refresh added

Latest patch needs a refresh.

@valendesigns5 weeks ago

comment:13 @valendesigns5 weeks 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.

comment:14 @rachelbaker4 weeks ago

  • Milestone changed from 4.2 to Future Release

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

Note: See TracTickets for help on using tickets.