Add user pattern categories to the pattern categories API response #5621
Conversation
… the `user` source is specified
|
@TimothyBJacobs do you have any thoughts about whether this is an appropriate/backwards compat way to add additional data and fields to the |
glendaviesnz
changed the title
|
There will be a bit of work in adding additional unit tests for this as the taxonomy terms will need to be mocked, so I will wait until this approach is approved and then add additional tests to cover it. |
| $valid_query_args = array( | ||
| 'source' => true, | ||
| ); | ||
| $query_args = array_intersect_key( $request->get_params(), $valid_query_args ); |
There was a problem hiding this comment.
What's the purpose of this code?
There was a problem hiding this comment.
This approach was copied from here, and I assumed it was a way of ensuring that any non-valid query args where dropped 🤔
There was a problem hiding this comment.
Right, but in that case it's because we are forwarding those query args wholesale to another API. Here, we are just consuming a single query parameter. So we can just do $request['source'].
| if ( rest_is_field_included( 'id', $fields ) && isset( $item['id'] ) ) { | ||
| $data['id'] = $item['id']; | ||
| } |
There was a problem hiding this comment.
Generally, introducing a new field would not be seen as a BC break. Is there a specific issue we are trying to work around here?
There was a problem hiding this comment.
ok, good to know, I thought additional fields would be seen as breaking change, I can remove this if not.
| 'description' => __( 'Limit result set to specific sources, `core` and/or `user`' ), | ||
| 'type' => 'array', | ||
| 'items' => array( | ||
| 'type' => 'string', |
There was a problem hiding this comment.
We can specify the allowed sources using the enum keyword.
| * @return array Collection parameters. | ||
| */ | ||
| public function get_collection_params() { | ||
| $query_params = parent::get_collection_params(); |
There was a problem hiding this comment.
We should unset the collection parameters we don't support, search, page and per_page. The default value for the context should also be set to view.
| * | ||
| * @return array Collection parameters. | ||
| */ | ||
| public function get_collection_params() { |
There was a problem hiding this comment.
The result of this method should be called in args in register_rest_route.
|
It seems a bit less than ideal that we are now making a database call, but this endpoint isn't paginated. What would it look like to support pagination? It's also unfortunate that this API shape does not match the terms controller which it is now returning data for, the |
|
Have you considered what this would look like as a search controller type via That would solve the different shape issues and is the more canonical way of handling similar but disparate data types in a single REST API request. |
|
Thanks for the pointer to |
|
Going to close this for now - not sure if I will get it sorted before 6.5 |
Currently just a draft PR to get some feedback on adding user pattern categories into the
block-patterns/categoriesendpoint to check that we are doing this in a standard and backward compatible way.This PR adds the user pattern categories only if the
sourcequery param is specified and includesuser.Also added is a new
idfield as this is needed to map the user categories back to the taxonomy terms in certain parts of the UI. This field is only returned ifidis explicitly request in the_fieldsparam.So to see the added data the endpoint has to be called with params
_fields: 'name,label,description, id'andsource: [ 'core', 'user' ],Trac ticket: https://core.trac.wordpress.org/ticket/59903
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.