| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
First pass at themes endpoint. #41836
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
Co-authored-by: Ben Dwyer <ben@scruffian.com>
|
@spacedmonkey, I think it seems more appropriate to discuss and review this in Core. What do you think? |
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
t-hamano
added
[Type] Enhancement
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 tried merging your branch with trunk locally but git got stuck because of the huge amount of changes. I ended up starting from trunk and cherry-picking the commits, here's the resulting branch: https://github.com/xavier-lc/gutenberg/tree/try/theme-api
I tested the endpoints and left some comments. Do you think you'll be able to address them? If not, I could take over your PR. Also, I'd be glad to help moving this forward; I could add tests, for example.
| array( | ||
| 'methods' => WP_REST_Server::READABLE, | ||
| 'callback' => array( $this, 'get_items' ), | ||
| 'permission_callback' => array( $this, 'get_items_permissions_check' ), | ||
| 'args' => $this->get_collection_params(), | ||
| ), |
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 think this could be removed, since it's doing the same as the base /wp/v2/themes route.
| $language_packs = array_map( | ||
| static function( $item ) { | ||
| return (object) $item; | ||
| }, | ||
| $api->language_packs | ||
| ); |
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'm getting an error here because $api->language_packs is null instead of the expected array.
| array( | ||
| 'methods' => WP_REST_Server::READABLE, | ||
| 'callback' => array( $this, 'get_item' ), | ||
| 'permission_callback' => array( $this, 'get_item_permissions_check' ), | ||
| ), |
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 think this could be removed, since it's doing the same as the base /wp/v2/theme/{stylesheet} route.
| * @return true|WP_Error True if the request has access to update the item, WP_Error object otherwise. | ||
| */ | ||
| public function update_item_permissions_check( $request ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable | ||
| if ( current_user_can( 'switch_themes' ) ) { |
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.
A check for current_user_can( 'manage_network_themes' ) is probably needed as well.
| 'previous' => $prepared->get_data(), | ||
| ) | ||
| ); | ||
| } |
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.
Deleting the active theme leaves the site in a broken state, the default should be set as is done in handle_theme_status:
| $theme_controller->register_routes(); | ||
| } | ||
| add_action( 'rest_api_init', 'gutenberg_register_theme_controller' ); | ||
|
|
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.
There's a broken comment block below this code.
|
@spacedmonkey @xavier-lc I was interested in pushing this work forward, but I wasn't sure if this one or #70847 is where the canonical work happens. @xavier-lc I understand there are some rebase issues - if you wish to continue in #70847, perhaps it's reasonable to pull any reasonable work from here into your PR ( give @spacedmonkey the credits of course)? Otherwise, #70847 seems a bit stuck too. Happy to help with some reviews on either PR, btw. |
What?
First pass at a REST API for themes management.
To do
Why?
How?
Testing Instructions
Screenshots or screencast