Skip to content
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

Use WP_REST_Posts_Controller as a base for global styles API #36076

Closed
wants to merge 4 commits into from

Conversation

spacedmonkey
Copy link
Member

Description

Follow up from #35801.

Global styles are a custom post type. So we use simple extend the WP_REST_Posts_Controller for the rest controller. This PR adds lots of functionality but removes lots of code.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@spacedmonkey spacedmonkey marked this pull request as ready for review October 30, 2021 09:31
@TimothyBJacobs
Copy link
Member

Thanks for taking a look at this @spacedmonkey, but as I mentioned in the original issue, I'm not in favor of this at the moment. Just because something is stored as a CPT does not mean it should be exposed as a base class of WP_REST_Posts_Controller. There is a lot of functionality there that we are not interested in.

Additionally, for instance, if we are to integrate the customizer into the REST API, I don't expect customizer changesets to be implemented via the posts controller either.

Fundamentally, the goal of the REST API is to provide the most appropriate interface for each resource type, not to shoehorn all types into an existing type.

If we want to introduce more advanced functionality in the future, we can, but there isn't a need for it now. Additionally, our building blocks don't necessitate inheritance. Things like meta are designed to be able to be pulled in via the WP_REST_Meta_Fields helpers.

@spacedmonkey
Copy link
Member Author

I don't understand this. I believe this PR deminstrates that perfectly, why this should use the post controller. The interface is exactly the same, global styles are requests via their ID.

The only difference here, is a little field mapping. Which this class does. There are already two other examples of where we do something similar. The WP_REST_Blocks_Controller, overrides some of the fields and the WP_REST_Menu_Items_Controller that does field mapping as well. I don't see the difference here?

This PR, adds functionality, like

  • Taxonomy querying
  • Meta handling
  • Delete endpoint
  • Create endpoint

All with the less code and extending an existing endpoint. It means also, that we have less code to maintain and any other improvements will filter down to this endpoint.

I have yet to have anyone explain why this endpoint HAS to be different. Can you provide some more context to this @youknowriad

@spacedmonkey
Copy link
Member Author

Similarly, by is the templates post type controller registion. See #36095

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants