Comment REST API route: add support for filtering by multiple statuses. - #9870
adamsilverstein wants to merge 33 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.
westonruter
left a comment
There was a problem hiding this comment.
Good to add this test: #9870 (comment)
Otherwise, pre-approving.
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.
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.