-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Use abilities categories in rest-api
#10402
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?
Conversation
|
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. |
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. |
|
Is this stricly necessary? I followed the feedback from @TimothyBJacobs (#9410 (comment)) when renaming the route and controller. |
|
I think so since the name of the resource is ability categories, not categories. |
|
IMO it being within the |
| ), | ||
| 'abilities' => array( | ||
| 'href' => rest_url( sprintf( '%s/abilities?category=%s', $this->namespace, $category->get_slug() ) ), | ||
| 'href' => rest_url( sprintf( '%s/abilities?ability_category=%s', $this->namespace, $category->get_slug() ) ), |
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.
Did this parameter already get changed in another PR? I'm not seeing this change in this PR.
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.
It has not been changed in trunk since being committed. https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-abilities-v1-categories-controller.php#L215
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.
This should stay as category param here, unless you also want to change the filtering in the list controller.
|
My thinking with being specific is that it prevents any confusion between categories and abilities categories. This was what I view as the consensus reached in #9410. While this endpoint is inside the I think this is important to get right since this isn't something that will be easy to change after 6.9 is released. |
|
In order for thsi to make RC1, I'm planning to commit this on Monday, please get any reviews folks have in done before them. |
JasonTheAdams
left a comment
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 agree that it's important for us to be consistent with this. 👍
Agreed.
Adopting a specific namespace for the API allows us to organize like endpoints and reduce duplicative naming. Ultimately, Code is Poetry, and I think APIs like: Are more elegant than:
I don't think this is likely to happen. I think the segmentation provided by the namespace and the API structure is pretty different. |
The problem is that it's not accurate. These are not categories, they are ability categories. Using a shortened form that is the name of something else doesn't reduce duplicative naming, it makes it inaccurate. |
|
The argument I understand you to be making, is that the API namespace doesn't matter for naming, and doesn't provide any scoping. I just can't get behind that.
I'm struggling to understand what you mean by this. Yes, they are used to categorize abilities. And that should be evident by their URL including "abilities". Most resources in this API are going to also be about abilities, and I don't want us to need to prefix every one of them with the word The mapping of PHP functions doesn't need to be an exact one-to-one with how they are represented in the REST API. We see this in other Core APIs like But even if we do look at the PHP API, we can see that the |
The consensus decided in #9410 was to name them Abilities Categories with the specific idea being "We always refer to them as Ability Categories and not merely "category"". What I'm hoping to do here is to live up to that consensus.
Thanks, I'll open a new PR to fix that. |
Follow up to https://core.trac.wordpress.org/changeset/61045 and https://core.trac.wordpress.org/changeset/61032
Trac ticket: https://core.trac.wordpress.org/ticket/64098
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.