-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Comment REST API route: add support for filtering by multiple statuses. #9870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Changes from all commits
82c76fa
1fc983f
4ee6fe8
bdd5a6d
b878d8b
f435d93
7b22c39
6225ae6
f121b3e
5fba5f6
1ee7a86
3bf3121
f210ce5
faca069
0426084
c4159ea
a2972fd
4b93f1a
d064fb8
5217fd2
cd4496f
c1c7823
f44de34
b9b22de
9a34a07
f29b134
6dd63d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -157,7 +157,7 @@ public function get_items_permissions_check( $request ) { | |
|
|
||
| foreach ( $protected_params as $param ) { | ||
| if ( 'status' === $param ) { | ||
| if ( 'approve' !== $request[ $param ] ) { | ||
| if ( array( 'approve' ) !== $request[ $param ] ) { | ||
| $forbidden_params[] = $param; | ||
| } | ||
| } elseif ( 'type' === $param ) { | ||
|
|
@@ -200,7 +200,7 @@ public function get_items_permissions_check( $request ) { | |
| if ( ! current_user_can( 'edit_posts' ) ) { | ||
| foreach ( $protected_params as $param ) { | ||
| if ( 'status' === $param ) { | ||
| if ( 'approve' !== $request[ $param ] ) { | ||
| if ( array( 'approve' ) !== $request[ $param ] ) { | ||
| $forbidden_params[] = $param; | ||
| } | ||
| } elseif ( 'type' === $param ) { | ||
|
|
@@ -1777,9 +1777,12 @@ public function get_collection_params() { | |
|
|
||
| $query_params['status'] = array( | ||
| 'default' => 'approve', | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adamsilverstein Why this one is still string instead of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the endpoint will accept a string or array? Still for consistency you are right this should probably be an array. |
||
| 'description' => __( 'Limit result set to comments assigned a specific status. Requires authorization.' ), | ||
| 'sanitize_callback' => 'sanitize_key', | ||
| 'type' => 'string', | ||
| 'description' => __( 'Limit result set to comments assigned one or more statuses. Requires authorization.' ), | ||
| 'type' => 'array', | ||
| 'items' => array( | ||
| 'type' => 'string', | ||
| ), | ||
| 'sanitize_callback' => array( $this, 'sanitize_comment_statuses' ), | ||
| 'validate_callback' => 'rest_validate_request_arg', | ||
|
adamsilverstein marked this conversation as resolved.
|
||
| ); | ||
|
|
||
|
|
@@ -2041,6 +2044,29 @@ protected function check_is_comment_content_allowed( $prepared_comment ) { | |
| return '' !== $check['comment_content']; | ||
| } | ||
|
|
||
| /** | ||
| * Sanitizes a single comment status or a list of comment statuses. | ||
| * | ||
| * @since 7.1.0 | ||
| * | ||
| * @param string[]|string $statuses Comment status or array of comment statuses. | ||
| * @return string[] Sanitized array of comment statuses. | ||
| * @phpstan-return list<non-empty-lowercase-string> | ||
| */ | ||
| public function sanitize_comment_statuses( $statuses ): array { | ||
| $statuses = array_unique( array_map( 'sanitize_key', wp_parse_list( $statuses ) ) ); | ||
|
|
||
| // Only drop empty values. A literal '0' is a queryable status, as comments are held with `comment_approved` of '0'. | ||
| $statuses = array_filter( | ||
| $statuses, | ||
| static function ( $status ) { | ||
| return '' !== $status; | ||
| } | ||
| ); | ||
|
|
||
| return array_values( $statuses ); | ||
| } | ||
|
|
||
| /** | ||
| * Check if post type supports notes. | ||
| * | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, is there a back-compat concern with the
statusarg now becoming an array of strings? Could there be any plugins that are looking at this parsed request param which would fail now?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. The raw query string is unchanged, but once sanitization runs (which happens before the permission callback),
$request['status']is always an array - so a plugin doing a string comparison against the parsed param, eg.'approve' === $request['status']inside arest_comment_queryfilter callback, would now seearray( 'approve' )and fail.$prepared_args['status']handed toWP_Comment_Queryalso becomes an array, though that part is transparent since the query class accepts both forms.So yes, there is a narrow back compat surface for code inspecting the parsed request param directly. It matches the shape the posts controller
statusparam has had since 4.7 ([39104], https://core.trac.wordpress.org/ticket/38420), so the pattern is well established. I'd rather call this out in the dev note than try to preserve the string shape for single values, which would make the parsed type unpredictable. What do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try searching https://veloria.dev/ but may be hard to be sure without a ton of digging. I can also merge this in early 7.2 so there is plenty of time to get feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's ok.