JSON API: Use register_rest_route() consistently#6006
Conversation
|
I have not tested it but I fully approve of this PR. Endpoint registrations should be very clear and easy to follow with as little indirection as possible. |
|
@tyxla Thanks for adding the proposed entry to the PR description, that's really helpful! Don't mind me changing that label back to "Needs Changelog". :) We actually only add the "Has Changelog" once the changelog entry has been added to the readme. I usually handle this with the release manager, so you don't have to worry about it. |
|
@jeherve sorry for any confusion. And thank you for clarifying those 👍 |
|
Excellent work. This solves part 1 of #5973. I'm going to rebase now, and try to solve part 2. |
f043493 to
d2cfffb
Compare
|
I've pushed some changes, and this is ready for review again. |
eliorivero
left a comment
There was a problem hiding this comment.
Blocking this since the change about get_json_params is a microperformance one and should be addressed.
The others about phpDocs are desirable but not blockers.
| /** | ||
| * Allows the `settings` and `module/<slug>` EDITABLE endpoints to accept both JSON and multi-part POST bodies. | ||
| * | ||
| * @param $request A WP REST API Request Object |
There was a problem hiding this comment.
The type for this is WP_REST_Request so this should be
@param WP_REST_Request $request The request sent to the WP REST API.
| */ | ||
| public function parse_settings_request_body( $request ) { | ||
| if ( is_array( $request->get_json_params() ) ) { | ||
| return $request->get_json_params(); |
There was a problem hiding this comment.
We can avoid calling get_json_params() twice by doing:
$params = $request->get_json_params();
if ( is_array( $params ) ) {
return $params;
}
| public static function dismiss_notice( $data ) { | ||
| $notice = $data['notice']; | ||
| $param = $data->get_json_params(); | ||
| public static function dismiss_notice( $request ) { |
There was a problem hiding this comment.
Since we're renaming $data to $request can we also fix the phpDoc and add the following everywhere where it applies?
@param WP_REST_Request $request The request sent to the WP REST API.
13b49e5 to
089ed72
Compare
|
Thanks @eliorivero - I addressed your feedback. @samhotchkiss - any reason for eliminating the milestone on this? |
zinigor
left a comment
There was a problem hiding this comment.
Looks good to me, can be merged after two changes in comments. Thank you!
| * | ||
| * @since 4.3.0 | ||
| * | ||
| * @param WP_REST_Request $request The request sent to the WP REST API. |
There was a problem hiding this comment.
Same here, this doc entry is not needed.
There was a problem hiding this comment.
This function does have a $request parameter.
|
@roccotripaldi -- we got rid of release based milestones, due to our new release structure. Looks good to merge once you address Igor's feedback on the comments, and it'll get into the next release! |
| * | ||
| * @return array|bool | ||
| */ | ||
| public function parse_settings_request_body( $request ) { |
There was a problem hiding this comment.
What problem does this function solve? In general, API endpoints should pull the parameters they need directly from $request['parameter_name'], which has the benefits of simplicity and of treating parameters equally from all sources. From #5973:
almost all endpoints should access parameters directly like
$request['parameter_name']so that they can come from any source and still be treated correctly. I can't think of a reason why an endpoint would still need to get all its parameters at once, but in that case it should use$request->get_params()to get all the parameters from every source, rather than only accepting JSON.
Given that, and given that this function is only called once that I see, I would recommend removing it.
There was a problem hiding this comment.
James, with this function I was trying to avoid calling get_json_params for any settings-related request, which is how we are currently doing it. See: https://github.com/Automattic/jetpack/pull/6006/files#diff-2ccec9069213650359171727de9a2471L426
Calling get_json_params only allows for JSON encoded POST bodies. This function allows for both.
There was a problem hiding this comment.
Just call $request->get_params() instead, this will handle all parameter sources.
Then, since this function is only called once, you can inline the rest of the logic into update_data. Something like this:
$params = $request->get_params();
$params = array_filter( $params, 'is_string' );
unset( $params['context'] );
unset( $params['slug'] );| $response = $this->create_and_get_request( 'settings', array( 'carousel_background_color' => 'black' ), 'POST' ); | ||
| $this->assertResponseStatus( 200, $response ); | ||
|
|
||
| // It should also save correctly with a multi-part POST body. |
There was a problem hiding this comment.
Here and elsewhere, the term "multi-part" is incorrect and should be removed. If it were a real request, this would just be a regular POST request with Content-Type application/x-www-form-urlencoded. A multipart request has Content-Type multipart/form-data and looks very different, usually containing uploaded files. Examples of each type of request:
…Endpoints::route()
- We want to offer support for both JSON and non-JSON request bodies.
were having trouble with `array_filter`, made changes to accommodate.
For clarity, we will pass `$request` to all registered endpoints, rather than passing `$data` - It does not make sense to refer to a WP_REST_API_Request object as `$data`
When parsing the request body, only `get_json_params()` once at most. It's more performant.
beaa110 to
d5393bb
Compare
I was using the term 'multi-part POST body' incorrectly. When I said 'multi-part POST body', what I was actually referring to was a POST body that was NOT JSON encoded.
- removing `parse_settings_request_body` method, and instead parsing body inline within `update_data` method - adding a explanation on why we cannot use `$request->get_params()`
|
@nylen - I removed the method Added an explanation of why we cannot use When we register the settings route, we declare When we are processing the request, we don't need that gigantic array, but only the actual params in the body, which we can access with either I'm not sure why we decided to register the route like this, but it would not be trivial to fix it. We can create an issue for it and handle it in an other PR perhaps. |
nylen
left a comment
There was a problem hiding this comment.
Alright, thanks for the explanation @roccotripaldi.
Having left my feedback that the parameter parsing pattern in update_data looks a bit strange, I'll leave it up to y'all for the best way to handle it.
If you haven't already, you'll want to get someone who's familiar with these features to make sure everything still works and/or is covered by tests. The code looks good though. 👍🏻
- add clearer explanations - use `unset` to remove unwanted params
* Changelog: update stable tag and move changelog to changelog.txt Also remove old releases from readme.txt to keep the changelog tab short. * Changelog: add #5883 Also update the filter's docblock to match new version. * Changelog: add #5938 * Changelog: add #6298 * Changelog: add #3405 * Changelog: add #5941 * Changelog: add #6239 * Changelog: add #6281 * Changelog: add #6303 * Changelog: add #6018 * Changelog: add #6300 * Changelog: add #6296 * Changelog: add #6130 * Changelog: add #6292 * Readme: remove extra "on". * Changelog: add #6307 * Changelog: add #3297 * Changelog: add #6275 * Changelog: add #6321 * Changelog: add #6297 * Readme: update the support forum link anchor. Anchor changed when WordPress.org forums were updated to bbPress 2 * Readme: update list of a12s, it wasn't up to date anymore! * Changelog: add #6338 * Changelog: add #6337 * Changelog: add #6335 * Changelog: add #6333 * Testing List: first version of the 4.7 testing list. * Changelog: add #6332 * Changelog: add #6325 * Changelog: add #6326 * Changelog: add #6339 * Changelog: add #6342 * Changelog: add #6343 * Changelog: add #6346 * Changelog: add #6347 * Changelog: add #6279 * Changelog: add #6306 * Changelog: add #6312 * Changelog: add #6316 * Changelog: add #6171 * Changelog: add #6317 * Changelog: add #6246 * Changelog: add #6263 * Changelog: add #4220 * Changelog: add #5888 * Changelog: add #3406 * Changelog: add #3637 * Changelog: add #6320 * Changelog: add #5992 * Changelog: add #6322 * Changelog: add #6324 * Changelog: add #6352 * Changelog: add #6355 * Changelog: add #6360 * Changelog: add #6362 * Changelog: add #6369, #6382 * Changelog: add #6370 * Changelog: add #6375 * Changelog: add #6383 * Changelog: add #6384 * Changelog: add #6386 * Changelog: add #6395 * Changelog: add #6403 * Changelog: add #6406 * Changelog: add #6418 * Changelog: add #6419 * Changelog: add #6434 * Changelog: add #6446 * Changelog: add #6006 * Changelog: add #6096 * Changelog: add #6399 * Changelog: fix typo. @see #6331 (comment) * Changelog: add #6440 * Changelog: add #6443 * Changelog: add #6445 * Changelog: add #6463 * Changelog: add #6468 * Changelog: add #6471 * Changelog: add #6474 * Changelog: add #6480 * Changelog: add #6497 * Changelog: add #6499 * Changelog: add #6514 * Changelog: add #6267 * Changelog: add #5940 * Changelog: add #6492 * Changelog: add #5281 * Changelog: add #6327 * Changelog: add #6451 * Changelog: add #6525 * Changelog: add #6530
Changes proposed in this Pull Request:
This PR suggests that we use
register_rest_route()consistently for registering JSON API endpoint routes. It also removes the now obsoleteJetpack_Core_Json_Api_Endpoints::route()method.Testing instructions:
Proposed changelog entry for your changes:
register_rest_route()consistently for registering JSON API endpoint routes.gulp js:hintbefore to commit your changes. It will allow you to detect errors in Javascript files.