Expose theme_supports data through a minimial theme controller #10518

desrosj · 2018-10-11T14:56:09Z

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 of current to match the wording in wp-admin and the codebase. I also went with the edit_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:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

… list view context on formats and post-thumbnails fields. Uncomment permissions logic. Remove old code placing this data in the index.

youknowriad · 2018-10-11T14:59:31Z

packages/core-data/src/resolvers.js

@@ -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' } );


Maybe we can preload this API endpoint here https://github.com/WordPress/gutenberg/blob/master/lib/client-assets.php#L1303 ?

Added this endpoint to that list. It correctly prevents the Featured Image editor from flashing onto the screen like it was.

Cool, do we need to pass the edit context here? (and the preloaded path)?

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.

Context was removed entirely per the discussion in #core-restapi.

danielbachhuber

Great start! Few more minor issues and then it should be good to go.

danielbachhuber · 2018-10-11T14:59:07Z

lib/class-wp-rest-themes-controller.php

+	 */
+	protected function prepare_links( $theme ) {
+		$links = array(
+			'active'       => array(


Should this be self ?

danielbachhuber · 2018-10-11T14:59:38Z

lib/class-wp-rest-themes-controller.php

+							'readonly'    => true,
+						),
+						'post-thumbnails' => array(
+							'description' => __( '' ),


Does this need a description?

lib/class-wp-rest-themes-controller.php

danielbachhuber · 2018-10-11T15:03:41Z

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`

desrosj · 2018-10-11T15:46:06Z

Made adjustments suggested. Also corrected the indentation and missing text domain errors picked up by PHPCS.

danielbachhuber · 2018-10-11T16:47:40Z

lib/load.php

@@ -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';


We need to wrap this with a class_exists() but it can happen in a follow-up PR for all of the controllers: #10526

danielbachhuber

👍 Looks to me

…/C02RQC26G/p1539297122000100).

danielbachhuber · 2018-10-11T23:10:11Z

@desrosj While you're at it, can you merge master for #10531 and apply the class_exists() check?

TimothyBJacobs · 2018-10-11T23:18:12Z

I don't think the magic __get accessor should be used directly instead either just the property syntax, $theme->name or $theme->get( 'Name' )

Additionally, WP_Theme has a display method for returning the "display" form of a lot of that data. If we include things like the theme name and such, we should probably include that and follow the same format we do elsewhere.

$data['name'] = array(
	'raw'      => $theme->get( 'Name' ),
	'rendered' => $theme->display( 'Name' )
);

If we do do that, we should then add the context property back and only expose raw on edit context. Or just omit the raw version entirely.

lib/class-wp-rest-themes-controller.php

…-rest-api-themes-controller # Conflicts: # lib/load.php

…ses.

kadamwhite · 2018-10-12T20:13:52Z

lib/class-wp-rest-themes-controller.php

+	public function register_routes() {
+		register_rest_route(
+			$this->namespace,
+			'/' . $this->rest_base . '/active',


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 an enum 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 like status 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.

kadamwhite · 2018-10-12T20:15:18Z

lib/class-wp-rest-themes-controller.php

+	 * @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 ) {


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.

kadamwhite · 2018-10-12T20:16:40Z

lib/class-wp-rest-themes-controller.php

+		}
+
+		if ( in_array( 'version', $fields, true ) ) {
+			$data['version'] = $theme->get( 'Version' );


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.

danielbachhuber

Looking good! Just a couple small nits on my end.

danielbachhuber · 2018-10-12T21:26:44Z

lib/class-wp-rest-themes-controller.php

+	protected function prepare_links( $theme ) {
+		$links = array(
+			'self' => array(
+				'href' => rest_url( sprintf( '%s/%s?status=active', $this->namespace, $this->rest_base ) ),


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.

danielbachhuber · 2018-10-12T21:27:10Z

lib/class-wp-rest-themes-controller.php

+			'type'       => 'object',
+			'properties' => array(
+				'theme_supports' => array(
+					'description' => __( 'A list of features this theme supports.', 'gutenberg' ),


Language tweak: Features supported by this theme.

…rty description.

…-rest-api-themes-controller

kadamwhite

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.

desrosj · 2018-10-15T17:31:50Z

@kadamwhite I believe test_theme_supports_formats() will verify that scenario. The code here will always return at least array( 'standard' ). Do you think standard should be excluded if there is no post format support?

kadamwhite · 2018-10-15T18:49:20Z

@kadamwhite I believe test_theme_supports_formats() will verify that scenario. The code here will always return at least array( 'standard' ). Do you think standard should be excluded if there is no post format support?

No opinion, @desrosj ; 👍 on the current test sufficiently covering that use-case.

desrosj added 4 commits October 10, 2018 17:24

First pass at adding a Themes endpoint for the current active theme.

049f30f

Don't return false when the theme does not support thumbnails. Only…

9187617

… list view context on formats and post-thumbnails fields. Uncomment permissions logic. Remove old code placing this data in the index.

Use the new active theme endpoint for determining theme support.

e013653

Add unit tests for the new active theme controller.

08eb03d

desrosj requested a review from danielbachhuber October 11, 2018 14:56

desrosj self-assigned this Oct 11, 2018

desrosj added the REST API Interaction Related to REST API label Oct 11, 2018

youknowriad reviewed Oct 11, 2018

View reviewed changes

danielbachhuber requested changes Oct 11, 2018

View reviewed changes

danielbachhuber requested a review from kadamwhite October 11, 2018 15:02

desrosj added 2 commits October 11, 2018 11:38

Pre-load the active theme endpoint.

5b5675d

PR Review Feedback adjustments:

3af33f2

- 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`

desrosj added 2 commits October 11, 2018 11:52

Fix failing tests. Change default context to edit.

f18a663

Add missing textdomain.

7a1c75e

danielbachhuber reviewed Oct 11, 2018

View reviewed changes

danielbachhuber approved these changes Oct 11, 2018

View reviewed changes

Remove contexts from themes (see https://wordpress.slack.com/archives…

06fb8cf

…/C02RQC26G/p1539297122000100).

danielbachhuber added this to the 4.1 milestone Oct 11, 2018

TimothyBJacobs reviewed Oct 11, 2018

View reviewed changes

lib/class-wp-rest-themes-controller.php Show resolved Hide resolved

desrosj added 3 commits October 11, 2018 19:21

Merge branch 'master' of github.com:WordPress/gutenberg into add/core…

2ff2fb4

…-rest-api-themes-controller # Conflicts: # lib/load.php

Always include post-thumbnails property to ensure consistent respon…

2bbd6ba

…ses.

Avoid using the __get() magic method.

f593169

kadamwhite requested changes Oct 12, 2018

View reviewed changes

desrosj requested review from danielbachhuber and kadamwhite October 12, 2018 20:51

Change variable name from currentTheme to activeThemes, as the en…

c972f4a

…dpoint now returns a collection.

danielbachhuber requested changes Oct 12, 2018

View reviewed changes

desrosj added 2 commits October 15, 2018 10:55

Remove _links from the response and adjust the theme supports prope…

d3dbb6f

…rty description.

Merge branch 'master' of github.com:WordPress/gutenberg into add/core…

9693640

…-rest-api-themes-controller

kadamwhite approved these changes Oct 15, 2018

View reviewed changes

Updating JavaScript documentation.

b1fe1b7

Rename TotalThemes to TotalPages

f7cd0a5

danielbachhuber approved these changes Oct 15, 2018

View reviewed changes

kadamwhite merged commit de2fab7 into WordPress:master Oct 15, 2018

desrosj deleted the add/core-rest-api-themes-controller branch October 15, 2018 19:24

desrosj mentioned this pull request Oct 18, 2018

Make responsive embeds a theme option #10477

Merged

4 tasks

vindl mentioned this pull request Oct 24, 2018

Gutenberg: update npm packages to latest versions Automattic/wp-calypso#28000

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose theme_supports data through a minimial theme controller #10518

Expose theme_supports data through a minimial theme controller #10518

desrosj commented Oct 11, 2018

youknowriad Oct 11, 2018

desrosj Oct 11, 2018

youknowriad Oct 11, 2018

desrosj Oct 11, 2018

desrosj Oct 11, 2018

danielbachhuber left a comment

danielbachhuber Oct 11, 2018

danielbachhuber Oct 11, 2018

danielbachhuber commented Oct 11, 2018

desrosj commented Oct 11, 2018

danielbachhuber Oct 11, 2018

danielbachhuber left a comment

danielbachhuber commented Oct 11, 2018

TimothyBJacobs commented Oct 11, 2018

kadamwhite Oct 12, 2018

kadamwhite Oct 12, 2018

kadamwhite Oct 12, 2018

danielbachhuber left a comment

danielbachhuber Oct 12, 2018

danielbachhuber Oct 12, 2018

kadamwhite left a comment

desrosj commented Oct 15, 2018

kadamwhite commented Oct 15, 2018

Expose theme_supports data through a minimial theme controller #10518

Expose theme_supports data through a minimial theme controller #10518

Conversation

desrosj commented Oct 11, 2018

Description

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber commented Oct 11, 2018

desrosj commented Oct 11, 2018

Choose a reason for hiding this comment

danielbachhuber left a comment

Choose a reason for hiding this comment

danielbachhuber commented Oct 11, 2018

TimothyBJacobs commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadamwhite left a comment

Choose a reason for hiding this comment

desrosj commented Oct 15, 2018

kadamwhite commented Oct 15, 2018