WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#35090 closed defect (bug) (wontfix)

Query for comments where post isn't set

Reported by: danielbachhuber Owned by: boonebgorges
Milestone: Priority: high
Severity: normal Version:
Component: Comments Keywords: needs-patch
Focuses: Cc:
PR Number:

Description

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

Attachments (3)

35090.1.diff (1004 bytes) - added by danielbachhuber 4 years ago.
35090.1.2.diff (1.4 KB) - added by danielbachhuber 4 years ago.
35090.diff (4.6 KB) - added by boonebgorges 4 years ago.

Download all attachments as: .zip

Change History (19)

#1 @danielbachhuber
4 years ago

  • Keywords has-patch has-unit-tests dev-feedback added; needs-patch removed

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?

#2 @danielbachhuber
4 years ago

35090.1.2.diff has better tests

#3 @boonebgorges
4 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch has-unit-tests dev-feedback removed

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.

This ticket was mentioned in Slack in #core by danielbachhuber. View the logs.


4 years ago

@boonebgorges
4 years ago

#5 @boonebgorges
4 years ago

  • Keywords needs-patch needs-unit-tests removed

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)

#6 @boonebgorges
4 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

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.

#7 follow-up: @ocean90
4 years ago

  • Keywords needs-patch added
  • Priority changed from normal to high
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

This ticket was mentioned in Slack in #meta-i18n by ocean90. View the logs.


4 years ago

#9 in reply to: ↑ 7 ; follow-up: @boonebgorges
4 years ago

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

#10 in reply to: ↑ 9 @ocean90
4 years ago

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.

#11 @boonebgorges
4 years ago

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.

#12 @danielbachhuber
4 years ago

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.

#13 @danielbachhuber
4 years ago

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

#14 @boonebgorges
4 years ago

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.

#15 @boonebgorges
4 years ago

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.

#16 @boonebgorges
4 years ago

  • Milestone 4.5 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

With a heavy heart.

Note: See TracTickets for help on using tickets.