Global styles controllers: extend with WP_REST_Posts_Controller and WP_REST_Revisions_Controller #5699

ramonjd · 2023-11-23T05:56:43Z

This change:

extends WP_REST_Global_Styles_Revisions_Controller with WP_REST_Revisions_Controller
extends WP_REST_Global_Styles_Controller with WP_REST_Posts_Controller

As part of this work, the PR also:

Removes shared public methods
Updates schema to use parent and parent controller's properties. Unsets unnecessary properties.
Updates relevant tests. Deletes dupes.

Trac ticket: https://core.trac.wordpress.org/ticket/60131

Related PRs/discussions

Add a Global Styles endpoint and use it in the site editor gutenberg#35801
Revisions Controller: return single revision only when it matches the parent id #5655 (ensure this PR is rebased on top)
Global styles revisions: add route for single styles revisions #5622 (some of the changes in this PR will be redundant, e.g., get_item but we'll still need the single revision route)
Global styles revisions: add route for single styles revisions gutenberg#55827 (comment)

github-actions · 2023-12-22T03:38:40Z

Test using WordPress Playground

The 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

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

ramonjd · 2023-12-22T03:47:06Z

@adamsilverstein No huge rush, but if you get a chance to take this for a test drive, I'd be very grateful 🙇🏻

ramonjd · 2023-12-22T04:08:28Z

I double checked and the failing E2E test is also failing on trunk:

[[chromium] › gutenberg-plugin.test.js:25:6 › Gutenberg plugin › should activate: tests/e2e/specs/gutenberg-plugin.test.js#L12](https://github.com/WordPress/wordpress-develop/pull/5699/files#annotation_16570999825)
  1) [chromium] › gutenberg-plugin.test.js:25:6 › Gutenberg plugin › should activate ───────────────

    Retry #1 ───────────────────────────────────────────────────────────────────────────────────────
    SyntaxError: Unexpected token '<', "<br />
    <b>"... is not valid JSON

      10 | 	test.beforeAll( async ( { requestUtils } ) => {
      11 | 		// Install Gutenberg plugin if it's not yet installed.
    > 12 | 		const pluginsMap = await requestUtils.getPluginsMap();
         | 		                   ^
      13 | 		if ( ! pluginsMap.gutenberg ) {
      14 | 			await requestUtils.rest( {
      15 | 				method: 'POST',

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-revisions-controller.php

spacedmonkey

Let's leave the unit tests and not delete them.

One issue here is the revisions controller currently guesses that the parent controller is a instance or sub class of post controller. Means means it expects some classes and properties that might not exist in WP_REST_Global_Styles_Controller.

tests/phpunit/tests/rest-api/rest-global-styles-revisions-controller.php

tests/qunit/fixtures/wp-api-generated.js

spacedmonkey

My biggest worry about this WP_REST_Global_Styles_Controller does not extend the WP_REST_Posts_Controller. Future refactors would might meaning breaks here, as class methods / properties do not match. I feel like if we are going to make this change, we go the whole way and convert WP_REST_Global_Styles_Controller to extend WP_REST_Posts_Controller. Then we can use the work done in 1f51e1f, to regsiter custom post / revision controllers when registering the global post type. Then this manaul regsiteration can be removed here.

wordpress-develop/src/wp-includes/rest-api.php

Lines 326 to 332 in 43d2455

    
           // Global Styles revisions. 
        
           $controller = new WP_REST_Global_Styles_Revisions_Controller(); 
        
           $controller->register_routes(); 
        
           // Global Styles. 
        
           $controller = new WP_REST_Global_Styles_Controller(); 
        
           $controller->register_routes();

ramonjd · 2024-01-04T00:02:39Z

I feel like if we are going to make this change, we go the whole way and convert WP_REST_Global_Styles_Controller to extend WP_REST_Posts_Controller.

Thanks for the feedback.

Looks like this point was raised by @TimothyBJacobs back when the endpoint was introduced: WordPress/gutenberg#35801 (review)

I think in this case, considering how custom the global styles API is, I think it is fine to extend the base controller instead of using WP_REST_Posts_Controller.

@TimothyBJacobs do you think this is still the case? The "custom" nature of global styles, I think, still stands.

I'm happy to take a stab at it if we think inheriting WP_REST_Posts_Controller will bring more benefits as described above.

jonnynews · 2024-01-12T18:00:51Z

I'm happy to take a stab at it if we think inheriting WP_REST_Posts_Controller will bring more benefits as described above.

The big difference is now 1f51e1f is committed, we can make custom revisions endpoints. I think now we can have custom revisions, it register this as a post endpoint and revisions endpoint.

Let's try this change in another PR, see if it makes sense.

ramonjd · 2024-02-02T01:14:10Z

Let's try this change in another PR, see if it makes sense.

Started here. More stuff and tests to come 👍🏻

[WIP] Global styles controller: extend with WP_REST_Posts_Controller #6008

github-actions · 2024-02-02T01:14:41Z

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 props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @jonnynews.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

Core Committers: Use this line as a base for the props when committing in SVN:

Props ramonopoly, spacedmonkey, mukesh27.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

ramonjd · 2024-02-02T05:30:08Z

The big difference is now 1f51e1f is committed, we can make custom revisions endpoints. I think now we can have custom revisions, it register this as a post endpoint and revisions endpoint.

Works pretty well over at #6008, but I'm wondering if registering endpoints should also be optional - e.g., Global styles should be able to opt out of autosaves routes

Perhaps explicitly setting '${ autosave | revision }_rest_controller_class' to false when registering the post?

Ora a supports_autosaves, similar to supports_revisions? 🤷🏻

I'm happy to get a PR up to test this out.

spacedmonkey · 2024-02-05T21:19:56Z

Closing in favour of #6022

ramonjd · 2024-02-05T21:32:27Z

Closing in favour of #6022

@spacedmonkey Just checking, this PR was working as a stand alone patch.

Did you close with the intention to port these changes over to #6022? Otherwise I think we should reopen.

spacedmonkey · 2024-02-05T22:12:20Z

@spacedmonkey Just checking, this PR was working as a stand alone patch.

@ramonjd I thought we were aligned on #6022? I feel like #6022 is good to commit.

ramonjd · 2024-02-05T22:21:42Z

@ramonjd I thought we were aligned on #6022? I feel like #6022 is good to commit.

Definitely, but just not sure why this PR needed to be closed?

I think maybe our wires just got crossed 😅

This PR affects WP_REST_Global_Styles_Revisions_Controller (Revisions) not WP_REST_Global_Styles_Controller. I just assumed you were going to bundle them into #6022.

Happy to reapprove #6022 - it's good to go, but we still need this PR I think.

ramonjd · 2024-02-06T19:42:15Z

To be clear, #6022 needs to be part of this PR. Let's do this change in one commit.

Ah, got it. Thanks, for clearing that up.

#6022 seemed to work well on its own, but I can pull over the changes into this PR if that's your advice.

Thanks again.

WP_REST_Global_Styles_Revisions_Controller extends WP_REST_Controller

…ntroller. we

…ted from WP_REST_Revisions_Controller tests

…-revisions-controller.php Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

Excluding order and order by from the collection query params Comment

… individual items.

Updating generated API file

jonnynews

I believe the contractor, get_item and update_item methods no longer need to be overridden in the global style controller.

I also had a note about marking this endpoint as not support batching.

I love this change, as it allows the gutenberg plugin to override these classes easier.

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php

removing __construct, get_item and update_item in favour of the parent class methods Add $allow_batch property, setting it to false

ramonjd · 2024-02-08T03:14:08Z

I believe the contractor, get_item and update_item methods no longer need to be overridden in the global style controller.

👍🏻 Done

What do you think about get_post() as well?

The functions are the identical, the only difference being the error message, which seems fairly samey.

In global styles controller:

		$error = new WP_Error(
			'rest_global_styles_not_found',
			__( 'No global styles config exist with that id.' ),
			array( 'status' => 404 )
		);

In post controller:

		$error = new WP_Error(
			'rest_post_invalid_id',
			__( 'Invalid post ID.' ),
			array( 'status' => 404 )
		);

spacedmonkey

Looks good to me

mukeshpanchal27 · 2024-02-13T10:11:04Z

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php

@@ -457,10 +382,10 @@ protected function prepare_links( $id ) {
 	 *
 	 * @return array List of link relations.
 	 */
-	protected function get_available_actions() {
+	protected function get_available_actions( $post, $request ) {


Missing @param documentation for new variables.

mukeshpanchal27 · 2024-02-13T10:15:09Z

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-revisions-controller.php

-		$this->rest_base        = 'revisions';
-		$this->parent_base      = 'global-styles';
-		$this->namespace        = 'wp/v2';
+	public function __construct( $parent_post_type ) {


Missing @param documentation for new variables.

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-revisions-controller.php

Co-authored-by: Jonny Harris <148061917+jonnynews@users.noreply.github.com>

mukeshpanchal27 · 2024-02-13T13:48:01Z

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php

@@ -454,13 +379,16 @@ protected function prepare_links( $id ) {
 	 *
 	 * @since 5.9.0
 	 * @since 6.2.0 Added 'edit-css' action.
+	 * @since 6.5.0 Add $post and $request parameters. 


Suggested change

* @since 6.5.0 Add $post and $request parameters.

* @since 6.5.0 Added $post and $request parameters.

spacedmonkey · 2024-02-13T14:36:02Z

Committed.

This was referenced Nov 23, 2023

Global styles revisions: add route for single styles revisions #5622

Closed

Global styles revisions: add route for single styles revisions WordPress/gutenberg#55827

Merged

Tracking: Style Revision UX Improvements WordPress/gutenberg#55776

Closed

ramonjd force-pushed the update/global-styles-revision-extends-revisions-controller branch from 1e43217 to 1428167 Compare November 23, 2023 08:12

ramonjd force-pushed the update/global-styles-revision-extends-revisions-controller branch from 222463e to 7e40ac4 Compare December 22, 2023 03:20

ramonjd force-pushed the update/global-styles-revision-extends-revisions-controller branch from 7e40ac4 to 4ffbee2 Compare December 22, 2023 03:44

mukeshpanchal27 reviewed Dec 22, 2023

View reviewed changes

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-revisions-controller.php Outdated Show resolved Hide resolved

spacedmonkey self-requested a review December 27, 2023 21:41

spacedmonkey requested changes Dec 27, 2023

View reviewed changes

tests/phpunit/tests/rest-api/rest-global-styles-revisions-controller.php Show resolved Hide resolved

tests/qunit/fixtures/wp-api-generated.js Outdated Show resolved Hide resolved

ramonjd requested a review from spacedmonkey January 3, 2024 01:30

ramonjd force-pushed the update/global-styles-revision-extends-revisions-controller branch from edc8162 to 1120502 Compare January 3, 2024 01:51

spacedmonkey reviewed Jan 3, 2024

View reviewed changes

ramonjd force-pushed the update/global-styles-revision-extends-revisions-controller branch from 1120502 to 2275567 Compare January 31, 2024 21:34

ramonjd mentioned this pull request Feb 2, 2024

[WIP] Global styles controller: extend with WP_REST_Posts_Controller #6008

Closed

ramonjd mentioned this pull request Feb 4, 2024

Try/rest global styles controller inherits post controller #6022

Open

spacedmonkey closed this Feb 5, 2024

tellthemachines reopened this Feb 5, 2024

ramonjd requested a review from spacedmonkey February 6, 2024 02:57

ramonjd and others added 11 commits February 7, 2024 12:49

Initial commit.

7d192d8

WP_REST_Global_Styles_Revisions_Controller extends WP_REST_Controller

Adding title to schema.

d7b4c1c

Remove permissions checks and favour parent's

41c98a5

Remove duplicated tests

59d6953

Reverting post type registration changes as it assigns an autosave co…

dbcc3b5

…ntroller. we

Regenerated fixture

25cbe83

Revert introducing title in schema

e9d1f10

Reinstating global styles revision controller tests that were duplica…

ec4229f

…ted from WP_REST_Revisions_Controller tests

Update src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles…

ca524b2

…-revisions-controller.php Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

Excluding search, include and exclude from the collection query params

93190ae

Excluding order and order by from the collection query params Comment

Now that WordPress#5655 has landed, use inherited methods for getting…

b43344d

… individual items.

ramonjd force-pushed the update/global-styles-revision-extends-revisions-controller branch from d4d7e82 to bf20794 Compare February 7, 2024 01:51

Pulling across changes from WordPress#6022

dac8e0b

Updating generated API file

ramonjd force-pushed the update/global-styles-revision-extends-revisions-controller branch from bf20794 to dac8e0b Compare February 7, 2024 02:05

ramonjd changed the title ~~Global styles revisions controller: extend with WP_REST_Revisions_Controller~~ Global styles controllers: extend with WP_REST_Posts_Controller and WP_REST_Revisions_Controller Feb 7, 2024

jonnynews suggested changes Feb 7, 2024

View reviewed changes

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php Outdated Show resolved Hide resolved

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php Show resolved Hide resolved

Implementing latest round of feedback:

acd2590

removing __construct, get_item and update_item in favour of the parent class methods Add $allow_batch property, setting it to false

spacedmonkey requested review from jonnynews and mukeshpanchal27 February 13, 2024 10:02

spacedmonkey approved these changes Feb 13, 2024

View reviewed changes

mukeshpanchal27 reviewed Feb 13, 2024

View reviewed changes

jonnynews reviewed Feb 13, 2024

View reviewed changes

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php Show resolved Hide resolved

jonnynews reviewed Feb 13, 2024

View reviewed changes

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-revisions-controller.php Show resolved Hide resolved

Apply suggestions from code review

dfa02f8

Co-authored-by: Jonny Harris <148061917+jonnynews@users.noreply.github.com>

mukeshpanchal27 reviewed Feb 13, 2024

View reviewed changes

spacedmonkey closed this Feb 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global styles controllers: extend with WP_REST_Posts_Controller and WP_REST_Revisions_Controller #5699

Global styles controllers: extend with WP_REST_Posts_Controller and WP_REST_Revisions_Controller #5699

ramonjd commented Nov 23, 2023 •

edited

Loading

github-actions bot commented Dec 22, 2023

ramonjd commented Dec 22, 2023

ramonjd commented Dec 22, 2023

spacedmonkey left a comment

spacedmonkey left a comment •

edited

Loading

ramonjd commented Jan 4, 2024

jonnynews commented Jan 12, 2024

ramonjd commented Feb 2, 2024

github-actions bot commented Feb 2, 2024 •

edited

Loading

ramonjd commented Feb 2, 2024 •

edited

Loading

spacedmonkey commented Feb 5, 2024

ramonjd commented Feb 5, 2024

spacedmonkey commented Feb 5, 2024

ramonjd commented Feb 5, 2024 •

edited

Loading

ramonjd commented Feb 6, 2024

jonnynews left a comment

ramonjd commented Feb 8, 2024

spacedmonkey left a comment

mukeshpanchal27 Feb 13, 2024

mukeshpanchal27 Feb 13, 2024

mukeshpanchal27 Feb 13, 2024

spacedmonkey commented Feb 13, 2024

	// Global Styles revisions.
	$controller = new WP_REST_Global_Styles_Revisions_Controller();
	$controller->register_routes();

	// Global Styles.
	$controller = new WP_REST_Global_Styles_Controller();
	$controller->register_routes();

	* @since 6.5.0 Add $post and $request parameters.
	* @since 6.5.0 Added $post and $request parameters.

Global styles controllers: extend with WP_REST_Posts_Controller and WP_REST_Revisions_Controller #5699

Global styles controllers: extend with WP_REST_Posts_Controller and WP_REST_Revisions_Controller #5699

Conversation

ramonjd commented Nov 23, 2023 • edited Loading

Related PRs/discussions

github-actions bot commented Dec 22, 2023

Test using WordPress Playground

Some things to be aware of

ramonjd commented Dec 22, 2023

ramonjd commented Dec 22, 2023

spacedmonkey left a comment

Choose a reason for hiding this comment

spacedmonkey left a comment • edited Loading

Choose a reason for hiding this comment

ramonjd commented Jan 4, 2024

jonnynews commented Jan 12, 2024

ramonjd commented Feb 2, 2024

github-actions bot commented Feb 2, 2024 • edited Loading

Unlinked Accounts

ramonjd commented Feb 2, 2024 • edited Loading

spacedmonkey commented Feb 5, 2024

ramonjd commented Feb 5, 2024

spacedmonkey commented Feb 5, 2024

ramonjd commented Feb 5, 2024 • edited Loading

ramonjd commented Feb 6, 2024

jonnynews left a comment

Choose a reason for hiding this comment

ramonjd commented Feb 8, 2024

spacedmonkey left a comment

Choose a reason for hiding this comment

mukeshpanchal27 Feb 13, 2024

Choose a reason for hiding this comment

mukeshpanchal27 Feb 13, 2024

Choose a reason for hiding this comment

mukeshpanchal27 Feb 13, 2024

Choose a reason for hiding this comment

spacedmonkey commented Feb 13, 2024

ramonjd commented Nov 23, 2023 •

edited

Loading

spacedmonkey left a comment •

edited

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading

ramonjd commented Feb 2, 2024 •

edited

Loading

ramonjd commented Feb 5, 2024 •

edited

Loading