Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#29612 closed enhancement (fixed)

Query for multiple comment statuses

Reported by: ebinnion's profile ebinnion Owned by: boonebgorges's profile boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.0
Component: Comments Keywords: needs-patch
Focuses: Cc:

Description

I am currently working on a project where I need to query both trashed and approved comments. Currently, I am doing this by calling get_comments twice and then merging the results.

I think it would be more ideal if a developer could query for multiple comment statuses by passing in an array of statuses.

For example:

$comments = get_comments(
	array(
		'status' => array( 'trash', 'approve' )
	)
);

My use case is that I want to show trashed comments (with a "This comment was trashed notice' ) as well as approved comments in an effort to better preserve conversation threads. Here is an example:

https://i.cloudup.com/LZowG5KNS9-1200x1200.png

Attachments (5)

29612_gowp.diff (1.2 KB) - added by karpstrucking 10 years ago.
add support for array of comment statuses in get_comments()
trac-29612.diff (1.7 KB) - added by ebinnion 10 years ago.
Alternative patch for allowing an array of comment statuses.
29612.diff (3.1 KB) - added by ebinnion 10 years ago.
Updates patch to include unit tests.
29612.2.diff (3.1 KB) - added by ebinnion 10 years ago.
Adding patch with improved whitespace.
29612.patch (12.6 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (20)

@karpstrucking
10 years ago

add support for array of comment statuses in get_comments()

#2 @karpstrucking
10 years ago

  • Keywords has-patch needs-testing added

#3 follow-up: @nacin
10 years ago

  • Keywords needs-unit-tests added

There is a lot of history in those few lines of code. See #21101. I don't think this is as simple as wrapping a foreach around it, as 'all' probably needs to be handled a bit differently, etc.

Also, would need unit tests.

#4 in reply to: ↑ 3 @karpstrucking
10 years ago

Replying to nacin:

There is a lot of history in those few lines of code. See #21101. I don't think this is as simple as wrapping a foreach around it, as 'all' probably needs to be handled a bit differently, etc.

Also, would need unit tests.

That was an interesting read, thanks for that. I believe my patch retains the existing handling for a status of 'all': it's still "( comment_approved = '0' OR comment_approved = '1' )"

Although, it occurs to me that if 'all' is included as one of the statuses in an array, it will be ignored. I'm not sure what the behaviour should be in that situation.

@ebinnion
10 years ago

Alternative patch for allowing an array of comment statuses.

#5 @ebinnion
10 years ago

I have added an alternative patch that will allow an array of comment statuses.

karpstrucking brought up a good point about how to handle all in the array of statuses. I took the approach to add the approve and hold statuses if not already present.

@ebinnion
10 years ago

Updates patch to include unit tests.

#6 @ebinnion
10 years ago

  • Keywords needs-testing needs-unit-tests removed

I have updated the patch to include a couple of unit tests. I have verified that the unit tests are passing with my suggested patch.

vagrant@vvv:/srv/www/wordpress-develop$ phpunit --group comment
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests... To execute these, use --group ajax.
PHPUnit 4.0.20 by Sebastian Bergmann.

Configuration read from /srv/www/wordpress-develop/phpunit.xml.dist

........................

Time: 3.89 seconds, Memory: 60.50Mb

OK (24 tests, 178 assertions)
vagrant@vvv:/srv/www/wordpress-dev

@ebinnion
10 years ago

Adding patch with improved whitespace.

#7 @psycleuk
10 years ago

I'm also working on a project where multiple statuses need to be returned at the same time. The patch by ebinnion would be ideal. The work around at the moment is to combine the statuses together into a single 'status' then filter on 'comments_clauses' to explode it back out.

I'd also like to see it combined with ryans patch ( https://core.trac.wordpress.org/ticket/21101#comment:27 ) which included a 'full' option which then allowed no comment_approved column to be part of the SQL. The reason for that is if you are requesting a custom comment type then you may well want all regardless of status.

#8 @boonebgorges
10 years ago

In 29980:

Improve unit tests for 'status' param in WP_Comment_Query.

See #29612.

@boonebgorges
10 years ago

#9 @boonebgorges
10 years ago

  • Milestone changed from Awaiting Review to 4.1
  • Owner set to boonebgorges
  • Status changed from new to accepted

In [29980], I added some more systematic unit tests for the existing functionality.

29612.patch adds support for multiple comment statuses:

  • Accepts an array or a comma-separated string
  • A value of 'any' will cause the 'comment_approved' clause to be skipped, returning comments regardless of 'comment_approved' value. 'any' will override any status it's included with (eg array( 'any', 'approve' )
  • The 'any' functionality means that sometimes the 'approved' WHERE clause will be empty - and, in certain cases, the WP_Comment_Query SQL query will not contain a WHERE clause at all. This means that the current way of concatenating the WHERE clause won't work. I've changed it so that it builds an array of subclauses, which get assembled just before the final query string is built. Note that in the case of date_query, meta_query, and get_search_sql(), this meant stripping the leading ' AND ' from the SQL chunks. A bit ugly, but necessary for backward compatibility.

This ticket was mentioned in IRC in #wordpress-dev by boonebgorges. View the logs.


10 years ago

This ticket was mentioned in IRC in #wordpress-dev by doc-bot. View the logs.


10 years ago

#12 @boonebgorges
10 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 30084:

Support multiple 'status' values in WP_Comment_Query.

This change required turning the SQL concatenation into the generation of an
array, for greater flexibility.

Props karpstrucking, ebinnion.
Fixes #29612.

#13 @ocean90
10 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

[30084] brokes /wp-admin/edit-comments.php. The All view includes spam comments now too.

Last edited 10 years ago by ocean90 (previous) (diff)

#14 @boonebgorges
10 years ago

  • Status changed from reopened to accepted

ocean90 - Crud, thanks for the heads up.

#15 @boonebgorges
10 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 30093:

Support an empty string passed as a status in WP_Comment_Query.

The changes in [30084] broke backward compatibility with interfaces that
manually passed an empty string for the value of 'status', such as on
wp-admin/edit-comments.php.

Fixes #29612.

Note: See TracTickets for help on using tickets.