-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Expose theme_supports data through a minimial theme controller #10518
Expose theme_supports data through a minimial theme controller #10518
Conversation
… list view context on formats and post-thumbnails fields. Uncomment permissions logic. Remove old code placing this data in the index.
packages/core-data/src/resolvers.js
Outdated
@@ -70,8 +70,8 @@ export function* getEntityRecords( kind, name, query = {} ) { | |||
* Requests theme supports data from the index. | |||
*/ | |||
export function* getThemeSupports() { | |||
const index = yield apiFetch( { path: '/' } ); | |||
yield receiveThemeSupportsFromIndex( index ); | |||
const currentTheme = yield apiFetch( { path: '/wp/v2/themes/active' } ); |
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.
Maybe we can preload this API endpoint here https://github.com/WordPress/gutenberg/blob/master/lib/client-assets.php#L1303 ?
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.
Added this endpoint to that list. It correctly prevents the Featured Image editor from flashing onto the screen like it was.
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.
Cool, do we need to pass the edit
context here? (and the preloaded path)?
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.
That I am not sure of. The default context is now edit
because that is the only context this endpoint currently supports. But might be good to include it for when/if that default changes.
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.
Context was removed entirely per the discussion in #core-restapi.
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.
Great start! Few more minor issues and then it should be good to go.
*/ | ||
protected function prepare_links( $theme ) { | ||
$links = array( | ||
'active' => array( |
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.
Should this be self
?
'readonly' => true, | ||
), | ||
'post-thumbnails' => array( | ||
'description' => __( '' ), |
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.
Does this need a description?
Flagged @kadamwhite to review too, only if he has any opinions on this. |
- Alignment fixes - Add missing textdomain arguments - Update the property contexts to edit (should be the context the data is accessed) - Add missing description for `post-thumbnails` - Link should be `self`, not `active`
Made adjustments suggested. Also corrected the indentation and missing text domain errors picked up by PHPCS. |
lib/load.php
Outdated
@@ -15,6 +15,7 @@ | |||
require dirname( __FILE__ ) . '/class-wp-rest-blocks-controller.php'; | |||
require dirname( __FILE__ ) . '/class-wp-rest-autosaves-controller.php'; | |||
require dirname( __FILE__ ) . '/class-wp-rest-block-renderer-controller.php'; | |||
require dirname( __FILE__ ) . '/class-wp-rest-themes-controller.php'; |
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.
We need to wrap this with a class_exists()
but it can happen in a follow-up PR for all of the controllers: #10526
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.
👍 Looks to me
I don't think the magic Additionally,
If we do do that, we should then add the context property back and only expose |
…-rest-api-themes-controller # Conflicts: # lib/load.php
public function register_routes() { | ||
register_rest_route( | ||
$this->namespace, | ||
'/' . $this->rest_base . '/active', |
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.
Per discussion in Slack, we should use the following structure for querying this information:
/wp/v2/themes?status=active
will return an array containing the object representing the capabilities of the active theme.- For this initial release,
status
will be anenum
parameter with one option,active
, and that parameter will be required.- This leaves us room to expand the functionality of the endpoint to permit listing all themes down the road, etc, but restricts the complexity to the bare minimum currently needed.
- Using a filter further avoids any lock-in around how to point to the active theme from other resources (the debate between a link from the index,
/active
,/~active
, etc, which has unfolded mostly in slack). Any of these approaches might be found to be limiting down the road as we add support for theme management, multi-site, etcetera, while a query parameter likestatus
is unlikely to conflict with any future plans.
Long-term I imagine we'd update the active theme by PUT'ing a theme slug to the settings endpoint, or a hypothetical new site endpoint. Figuring all that out is out of scope and not necessary for Gutenberg in terms of 5.0.
* @param WP_REST_Request $request Full details about the request. | ||
* @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure. | ||
*/ | ||
public function get_item( $request ) { |
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.
Per the comment about ?status=active
, it would probably be best to introduce a get_items
method since we will technically be querying the collection endpoint to return the active theme.
} | ||
|
||
if ( in_array( 'version', $fields, true ) ) { | ||
$data['version'] = $theme->get( 'Version' ); |
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.
Are stylesheet, version, etc strictly required for Gutenberg? No argument that they seem like they would be useful long-term but I feel it best to limit what we expose on this endpoint to the bare minimum of theme-supports values we know we need now, and add more later.
…r would cause future issues when `themes/themename` returns data for a specific theme. For now, limit the `status` param to one value, `active`, by specifying the param as an `enum`. Also, remove field not 100% required for Gutenberg (name, version, template, etc.).
…dpoint now returns a collection.
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.
Looking good! Just a couple small nits on my end.
protected function prepare_links( $theme ) { | ||
$links = array( | ||
'self' => array( | ||
'href' => rest_url( sprintf( '%s/%s?status=active', $this->namespace, $this->rest_base ) ), |
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 href
is supposed to be /wp/v2/themes/<slug>
, which we don't have right now. We could probably remove prepare_links()
entirely.
'type' => 'object', | ||
'properties' => array( | ||
'theme_supports' => array( | ||
'description' => __( 'A list of features this theme supports.', 'gutenberg' ), |
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.
Language tweak: Features supported by this theme.
…rty description.
…-rest-api-themes-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.
Changes look good, thank you @desrosj ! Only outstanding question would be whether we should add a unit test assertion for themes which do not support post formats, to validate that we get that empty array.
@kadamwhite I believe |
No opinion, @desrosj ; 👍 on the current test sufficiently covering that use-case. |
Description
This PR moves the theme support data previously in the REST API index (landed in #6318) into a read-only theme controller for the active theme.
I went with
wp/v2/themes/active
instead ofcurrent
to match the wording in wp-admin and the codebase. I also went with theedit_posts
capability, as anyone using the edit screen will need Gutenberg to know what the active theme supports.I included the theme name, stylesheet (directory name), version, template, and theme supports data (what Gutenberg needs). I don't feel strongly about including any of those fields, but those are a few straightforward properties of a
WP_Theme
object that I felt were ok to include.This is the first step for landing Trac 45016 in Core.
Checklist: