wp_insert_comment should require comment_post_ID

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

[westonruter]

Simplify construction of commentdata array for insertion with defaults

[westonruter]

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

[westonruter]

Update unit tests to always create comments with a valid comment_post_ID

[westonruter]

Return WP_Error from wp_insert_comment if invalid comment_post_ID inserted

[westonruter]

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

[westonruter]

All patches pushed to branch on GitHub: ​https://github.com/x-team/WordPress/compare/87f9d11...23931-require-comment_post_ID

[westonruter]

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

[jdgrimes]

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.

[wonderboymusic]

Please combine/refresh the patches for src/tests.

[westonruter]

Refreshed patch. PR: ​https://github.com/x-team/wordpress-develop/pull/25

[SergeyBiryukov]

#30944 was marked as a duplicate.

[SergeyBiryukov]

Related: #30946

[DrewAPicture]

Latest patch needs a refresh.

[valendesigns]

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.

[rachelbaker]

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

[boonebgorges]

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.

[rachelbaker]

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

[markoheijnen]

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.