Query for comments where post isn't set

[danielbachhuber]

WP_Comment_Query should support querying for comments where post=0

This isn't currently supported because of this conditional:

$post_id = absint( $this->query_vars['post_id'] );
if ( ! empty( $post_id ) ) {
	$this->sql_clauses['where']['post_id'] = $wpdb->prepare( 'comment_post_ID = %d', $post_id );
}

Related

We'll need to accommodate for #34956, as comments with post='' should be considered equivalent to post=0

[danielbachhuber]

I don't fully understand the unit test added by @boonebgorges in r30402:

public function test_query_post_id_0() {
	$c1 = self::factory()->comment->create( array( 'comment_post_ID' => $this->post_id, 'comment_approved' => '1' ) );

	$q = new WP_Comment_Query();
	$found = $q->query( array(
		'post_id' => 0,
		'fields' => 'ids',
	) );

	$this->assertEqualSets( array( $c1 ), $found );
}

Why would we return all comments if post_id=0?

[danielbachhuber]

35090.1.2.diff has better tests

[boonebgorges]

The test in [30402] was meant to describe the legacy behavior of get_comments(). Since it was introduced in [8543], it's had an empty( $post_id ) check, so that post_id = 0 has always resulted in the post_id param being ignored. In other words, I wasn't endorsing it as "correct" behavior so much as I was describing it.

I agree that there ought to be a way to query for comments that have 0 as a post_id. This change will necessarily involve breaking backward compatibility in at least some cases. 35090.1.2.diff​ breaks too much: it makes it impossible to query for all comments regardless of comment_post_ID. Here's what I think would be an acceptable break, and what should be described in unit tests:

• Default value of post_id (ie not provided in the params array) should skip the comment_post_ID clause (return all comments)
• Passing an empty string, bool false, or null to post_id should skip the comment_post_ID clause
• int 0 or string '0' (is_numeric( $post_id )) should return comments with comment_post_ID IN ( '0', '' )

This will probably require changing the default value of post_id in get_comments() and/or WP_Comment_Query::query().

The only behavior that will change will thus be cases where 'post_id' => '0' or 'post_id' => 0; currently, they return comments of all posts, but after the change they'll return only comments with a null post. This is a break that makes sense to me.

[boonebgorges]

35090.diff​ shows what I have in mind:

• Passing null, false, or '' to post_id results in the clause being ignored (current behavior).
• Omitting the post_id param will return results regardless of comment_post_ID (current behavior).
• Passing 0 or '0' to post_id limits results to comments with comment_post_ID=0 (new behavior)

[boonebgorges]

In 36381:

Allow comment query results to be limited to comments with comment_post_ID = 0.

Previously, this was not possible due to an overly broad empty() check.

Passing null, false, or '' to 'post_id', or omitting 'post_id'
altogether, will continue to return comments regardless of comment_post_ID,
as before. Passing 0 or '0' will limit results to comments with no
associated post.

Props danielbachhuber.
Fixes #35090.

[ocean90]

[36381] broke the comments list table because we're passing 0 to post_id, see trunk/src/wp-admin/includes/class-wp-comments-list-table.php?rev=36381&marks=43,130#L43​.

[boonebgorges]

Replying to ocean90:

[36381] broke the comments list table because we're passing 0 to post_id, see trunk/src/wp-admin/includes/class-wp-comments-list-table.php?rev=36381&marks=43,130#L43​.

Oy.

It's trivial to fix the comment list table. But if we're doing something like this in core, it's extremely likely that the same technique is used extensively throughough the WP universe. This almost certainly means that we will not be able to make the change in [36381], and if we want a way to query for comments with no post, we'll have to come up with some other way of doing it.

I'll try to take some time to search through the plugin repo to see how extensive this kind of thing is. (Namely: either passing post_id=0 to a comment query, or copypasta from the core list table.) In the meantime, @ocean90, if you think this merits a revert so that trunk is usable, let me know (or be my guest).

[ocean90]

Replying to boonebgorges:

In the meantime, @ocean90, if you think this merits a revert so that trunk is usable, let me know (or be my guest).

Yeah, we should do a revert or fix the list table to make w.org usable again. :) I'm fine with a fix to the list table until we know how plugins are handling it.

[boonebgorges]

In 36387:

In comments list table, $post_id should default to false rather than 0.

After [36381], the default value of 0 was causing the list table at
edit-comments.php to be empty. false prevents this.

This fix is likely temporary, while more research is done into the backward
compatibility concerns tied to [36381].

See #35090.

[danielbachhuber]

Should this become a hairy bug, another option would be to introduce post__in (and post__not_in) for WP_Comment_Query which work as expected, and point people to those.

[danielbachhuber]

As it turns out, post__in already exists for WP_Comment_Query, and works well for our needs.

[boonebgorges]

Given that post__in is a good workaround, coupled with the likelihood that plugins are using 0 in the same way that core list tables do, I think it's wisest to revert these changes and mark this ticket as wontfix.

[boonebgorges]

In 36480:

Comments: Restore the ability to bypass post_id filter using 0 or '0'.

The changes introduced in [36381], while logical and clearly awesome, introduce
the potential for much breakage. Those who want to query for comments with a
null comment_post_ID should use 'post_in' => array( 0 ) instead.

Reverts [36381], [36387].
See #35090.

[boonebgorges]

With a heavy heart.