Repository navigation
Comment REST API route: add support for filtering by multiple statuses. - #9870
adamsilverstein wants to merge 38 commits into
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
We might not need to extend the REST API itself. See WordPress/gutenberg#71271 (comment) |
Co-authored-by: Pascal Birchler <pascal.birchler@gmail.com>
Good point; that said its probably worth adding as its a simple change and reflects what the underlying core comments class already supports (so it feels like a bug or oversight that it isn't supported already). While we don't need it yet, I could easily see us or a plugin wanting this. |
…roller.php Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
With the status collection param now an array, the sanitized value never string-matches 'approve', so unauthenticated requests using the default status were rejected. Compare against the parsed list instead, define the schema as an array of strings per review feedback, and regenerate the QUnit fixture to match.
The unauthenticated 401 boundary was only covered for the array form of the status parameter. Since the permission check now relies on the sanitize callback having parsed the raw value, the comma-separated form needs its own case so it cannot regress independently. Convert the test to a data provider covering both forms.
array_filter() without a callback drops all falsy strings, including '0'. WP_Comment_Query treats a literal '0' status as a direct comment_approved match (how held comments are stored), so ?status=0 is a working query today and must survive sanitization. Filter only empty strings instead.
Resolve the conflict in the comments controller, where trunk added check_target_post_permission() at the same spot the branch added sanitize_comment_statuses(). Both methods are kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GqepyjmFSryBt88j6f6ubY
Trunk is now 7.2-alpha, so the new method ships in 7.2. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GqepyjmFSryBt88j6f6ubY
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DxbbMKz3sa741SjHuE1ZLS
|
@adamsilverstein, this PR looks good to merge, but do you have any concerns? |
The `status` collection param is now typed as an array, but its default was still the string 'approve'. The permission checks compare against `array( 'approve' )`, so they only matched because sanitization coerced the default; declaring it as an array keeps the schema consistent with the param type and removes that implicit dependency. The generated QUnit schema fixture is updated to match. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B6M4x5WcvJVMgMqxH6qBKv
I think its ready, sweeping for unaddressed feedback. |
Looks good to me! |
t-hamano
left a comment
There was a problem hiding this comment.
Looks good from my end. It might be worth noting the definition change under miscellaneous dev notes.
|
Drafted with Claude for the WordPress 7.2 miscellaneous developer changes post: Comments REST API: filter by multiple statusesThe comments collection endpoint (
To support this, the definition of the
Code that reads the status as a string may need an update. After sanitization, See https://core.trac.wordpress.org/ticket/63982 for more details. |
| ); | ||
|
|
||
| return array_values( $statuses ); | ||
| } |
There was a problem hiding this comment.
some late follow-up here is that we’re performing a lot of needless allocation here, and while the performance impact of this function is likely not relevant in the overall picture, it’s still part of painting that.
effectively this could be replaced by one big iteration, rather than the current five passes with intermediate arrays.
here is an alternative idea skipping that allocation.
if ( is_string( $statuses ) ) {
$statuses = wp_parse_list( $statuses );
}
$sanitized = array();
$seen = array();
foreach ( $statuses as $status ) {
if ( ! is_scalar( $status ) ) {
continue;
}
$key = sanitize_key( $status );
if ( '' === $key || isset( $seen[ $key ] ) ) {
continue;
}
$seen[ $key ] = true;
$sanitized[] = $key;
}
return $sanitized;There was a problem hiding this comment.
Thanks for the feedback @dmsnell, I'll open a follow up PR to optimize this.
There was a problem hiding this comment.
Good point. And since the number of statuses is going to be very very small, then a hash lookup for $seen would be less performant than just a regular list. So this would seem to be even better:
if ( is_string( $statuses ) ) {
$statuses = wp_parse_list( $statuses );
}
$sanitized = array();
foreach ( $statuses as $status ) {
if ( ! is_scalar( $status ) ) {
continue;
}
$key = sanitize_key( $status );
if ( '' !== $key && ! in_array( $key, $sanitized, true ) ) {
$sanitized[] = $key;
}
}
return $sanitized;There was a problem hiding this comment.
Note to self: widen the $key param in sanitize_key() from string to scalar since it is explicitly supported.
There was a problem hiding this comment.
since the number of statuses is going to be very very small
it depends if we can guarantee that @westonruter. I have been using this pattern too, but if the list of statuses is user-provided, then there are catastrophic cases that lead to DoS
so if we assume the list is small, we should make sure before we introduce quadtratic lookup.
Add support for filtering the comments collection endpoint by multiple statuses, eg.
/wp/v2/comments?status=approve,holdorstatus[]=approve&status[]=hold.WP_Comment_Queryalready accepts an array of statuses, so this brings the REST endpoint in line with what the underlying query supports. Single string values continue to work as before.Originally extracted from the Notes work, see WordPress/gutenberg#71271 (WordPress/gutenberg#71271 (comment))
Changes:
statuscollection param as an array of strings, each sanitized withsanitize_key, following the pattern of the posts controller.get_items_permissions_checkto handle array values. Behavior is unchanged: unauthenticated requests can only use the defaultapprovestatus, anything else requiresedit_posts.allstatus queries, sanitization of the param, and the authorization boundary.Note: I did not add an
enumof allowed values here -WP_Comment_Queryexplicitly supports custom comment statuses and the endpoint currently accepts any string (eg.spamandtrashwork today for authorized users), so an enum limited to the registered statuses would be a back compat break.Testing instructions
curl --user admin:APP_PASSWORD "https://example.test/wp-json/wp/v2/comments?status=approve,hold"?status=hold, and verify only pending comments are returned.?status=approvestill returns approved comments and?status=approve,holdreturns arest_forbidden_paramerror.npm run test:php -- --filter WP_Test_REST_Comments_ControllerTrac ticket: https://core.trac.wordpress.org/ticket/63982
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.
Commit message