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

ramonjd · 2023-11-03T03:26:03Z

Tracking:

Tracking: Style Revision UX Improvements #55776

Core sync PR:

Global styles revisions: add route for single styles revisions wordpress-develop#5622

What?

Adding route for single global styles revisions: /wp/v2/global-styles/${ parentId }/revisions/${ revisionsId }

Why?

To make global styles revisions compatible with the proposed Core Data revisions API, that is, so that we can fetch single revisions via getRevision().

Testing Instructions

Run the tests!

npm run test:unit:php:base -- --filter Gutenberg_REST_Global_Styles_Revisions_Controller_Test

- Adding route for single global styles revisions

github-actions · 2023-11-03T03:27:55Z

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ lib/compat/wordpress-6.5/class-gutenberg-rest-global-styles-revisions-controller-6-5.php
❔ lib/compat/wordpress-6.5/rest-api.php
❔ lib/compat/wordpress-6.4/rest-api.php
❔ lib/load.php
❔ phpunit/class-gutenberg-rest-global-styles-revisions-controller-test.php

andrewserong

Works nicely for me via making calls within the site editor via wp.apiFetch( { path: '/wp/v2/global-styles/2054/revisions/4125' } ); (where 2054 is my global styles id and 4125 is a valid revision).

✅ Requesting a valid global styles revision works as expected
✅ Attempting to request an arbitrary post id correctly fails
✅ Fails when requesting a post's revision id instead of a global styles revision

Just left a couple of comments: I wasn't able to attack it manually, but it looks like we're not (yet) checking that the revision id matches the parent? Rather it's currently checking just that the parent can be accessed?

andrewserong · 2023-11-08T04:45:42Z

lib/compat/wordpress-6.5/class-gutenberg-rest-global-styles-revisions-controller-6-5.php

+ return $parent;
+ }
+
+ $revision = $this->get_revision( $request['id'] );


In the get_items version of retrieving revisions, they're guarded behind a check against the $parent of if ( wp_revisions_enabled( $paren ) ) {... should we add a similar check here, too?

I've modelled this class as closely as possible to WP_REST_Revisions_Controller.

The reason I wouldn't want to make these changes here is because they should be probably be raised in trac and discussed in the wider context of revisions since they all work this way.

Even so I think it should be okay for global styles.

This is what the docs say about the supports flag for register_post_type, specifically:

'revisions' feature dictates whether the post type will store revisions

So if a post type cannot store revisions, there'll be none to fetch.

It's possible wp_revisions_enabled is used in get_items to save an expensive new WP_Query() call.

I've modelled this class as closely as possible to WP_REST_Revisions_Controller.

@ramonjd I'm curious why you couldn't just use the core revisions controller, what feature was missing?

@adamsilverstein Thanks for the question. It is a good one.

Global styles post revisions store stringified JSON in the post content field. When preparing the response, the JSON needs to be parsed and structured according to the theme.json model, that is, something like this:

{ "settings": {}, "styles": {}, ... // whatever else comes along }

That's the main difference - the responses are different.

Also deleting revisions wasn't a requirement.

I did consider extending WP_REST_Revisions_Controller way back when I introduced the global styles revisions endpoint.

Global styles revisions were a bit new and had the following characteristics:

Didn't, and still doesn't, return excerpt, content, guid fields (has 2 custom fields: settings, styles and later ??)

Did not (yet) require delete routes

Had to do some JSON parsing when preparing the response

Back then, it was more code to override the parent class's functions, and I thought having it as a stand alone class left more elbow room for subsequent tweaks and changes. Especially since global styles revisions were new and evolving.

Also, since I tried to sync what I needed from WP_REST_Revisions_Controller, I thought that it wouldn't affect backwards compatibility if we one day decided to refactor this code and extend WP_REST_Revisions_Controller.

Today I think the functionality is more balanced. Overriding fields and response-related methods would still be needed, but are a lot of shared methods between the two classes and, arguably, a delete route would bring it in line with current revision expectations.

What do you think?

Hey @ramonjd thanks for clarifying your thinking here. I agree it makes sense to build out a separate class for your exact needs, especially while building something new where you want to change things and avoid breaking the existing controller.

That's the main difference - the responses are different.

Given that you mainly want to alter the response behavior, I wonder if you could use the existing revisions controller and leverage the rest_prepare_revision filter provided to adjust the shape of the return as you desire - this should let you control the exact response you want for each revision.

My main concern with adding another controller is the added maintenance burden. For example the issue you discovered with mismatched parent ids would need to get fixed in two places.

If you feel your controller is doing everything it needs to, I would encourage at least experimenting with using the filter approach and relying on the existing controller. This should reduce the amount of code you need.

cc: @TimothyBJacobs for a second opinion

Thanks, I appreciate the insight. 🙇🏻

If you feel your controller is doing everything it needs to, I would encourage at least experimenting with using the filter approach and relying on the existing controller.

Good call. I'd like to try this. The maintenance burden is on my mind 😅

+1 let me know if you find any shortcomings in the core endpoint or the filter/hooks it provides - if so maybe we can address those in core.

I have a Core PR in progress here:

Global styles controllers: extend with WP_REST_Posts_Controller and WP_REST_Revisions_Controller wordpress-develop#5699

I ended up replacing the base class with WP_REST_Revisions_Controller so that the WP_REST_Global_Styles_Revisions_Controller was still available (I wasn't sure about how radical I should be about maintaining backwards compatibility), but more importantly, so I could preserve the following features by overwriting class methods:

no deletion of revisions (I think this can come later - probably needs a discussion with Gutenberg Core members)

massage the schema

Thanks for the nudge! It removes a lot of code.

andrewserong · 2023-11-08T04:54:59Z

lib/compat/wordpress-6.5/class-gutenberg-rest-global-styles-revisions-controller-6-5.php

+ $revision = get_post( (int) $id );
+ if ( empty( $revision ) || empty( $revision->ID ) || 'revision' !== $revision->post_type ) {
+ return $error;
+ }


Do we need to check that the $revision->post_parent matches the provided parent id? Currently it looks like the endpoint might only be checking that the parent is valid and can be retrieved, not that the revision id matches that parent 🤔

This is also taken from WP_REST_Revisions_Controller.

The parent post type check ('wp_global_styles') should be enough for our purposes to start with.

You bring up a great point though.

Currently I can, for any post type, return the revision of different post just by knowing the post ID.

So for the following:

await wp.apiFetch( { path: '/wp/v2/posts/19/revisions/40' } )

19 is the id of a different parent. So long as 19 is valid and 40 is a revision post type, I'll get the revision object of 40, whose parent is 21.

As with the above comment, it could be a good question to raise in Core Slack or in a trac ticket.

There might be a reason why all post type revisions work this way. 🤔

I'll make a note to ask. Thank you!

Question asked here: https://wordpress.slack.com/archives/C02RQC26G/p1699504213232439

andrewserong

Thanks for the explanations @ramonjd! I agree, after seeing how you've copied the approach used in https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php exactly, it sounds like the questions I raised are best resolved separately if need be, for now it makes sense for this endpoint to match the existing revisions behaviour 👍

The checks in the current form were all adequate for the global styles use case and correctly guarded against folks retrieving things they don't have access to read.

LGTM! ✨

* Basic setup: - Adding route for single global styles revisions * Expected 1 blank line at end of file; 2 found * Add test for get_item * Add ticket number and 404 test

Now that #55827 has landed we can start supporting individual global styles URLs as well.

…tures (#56416) * Remove default value of query {} on getRevision. It messes up selector memoization since the function signatures are different. * Resolve individual revisions as per `getEntityRecords`. This ensures that any subsequent calls to `getRevision` for revisions that we've already fetched via `getRevisions()` are marked as resolved and therefore do not trigger the resolver (and therefore an API call). * Copy update. Now that #55827 has landed we can start supporting individual global styles URLs as well. * While I'm here... :)

Basic setup:

09affe5

- Adding route for single global styles revisions

ramonjd added [Type] Enhancement A suggestion for improvement. REST API Interaction Related to REST API Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Nov 3, 2023

ramonjd self-assigned this Nov 3, 2023

ramonjd requested a review from spacedmonkey as a code owner November 3, 2023 03:26

This was referenced Nov 3, 2023

Revisions: add new selectors to fetch entity revisions #54046

Merged

Tracking: Style Revision UX Improvements #55776

Closed

ramonjd added 3 commits November 3, 2023 14:28

Expected 1 blank line at end of file; 2 found

a000125

Add test for get_item

7472e68

Add ticket number and 404 test

ec9b0e4

ramonjd mentioned this pull request Nov 6, 2023

Global styles revisions: add route for single styles revisions WordPress/wordpress-develop#5622

Closed

andrewserong reviewed Nov 8, 2023

View reviewed changes

andrewserong approved these changes Nov 9, 2023

View reviewed changes

ramonjd merged commit c12f892 into trunk Nov 9, 2023
49 checks passed

ramonjd deleted the add/global-styles-revisions-single-revision branch November 9, 2023 04:38

github-actions bot added this to the Gutenberg 17.1 milestone Nov 9, 2023

This was referenced Nov 22, 2023

Tracking: Core data revisions follow ups #56413

Closed

Core data: harmonize getRevision selector and resolver function signatures #56416

Merged

ramonjd added a commit that referenced this pull request Nov 23, 2023

Copy update.

9f99392

Now that #55827 has landed we can start supporting individual global styles URLs as well.

ramonjd mentioned this pull request Nov 23, 2023

Global styles controllers: extend with WP_REST_Posts_Controller and WP_REST_Revisions_Controller WordPress/wordpress-develop#5699

Closed

getdave mentioned this pull request Jan 18, 2024

Plugin: Synchronize PHP changes for WordPress 6.5 release #57959

Closed

youknowriad added the Backported to WP Core Pull request that has been successfully merged into WP Core label Jan 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ramonjd commented Nov 3, 2023 •

edited

Loading

github-actions bot commented Nov 3, 2023 •

edited

Loading

andrewserong left a comment •

edited

Loading

andrewserong Nov 8, 2023

ramonjd Nov 8, 2023 •

edited

Loading

adamsilverstein Nov 20, 2023

ramonjd Nov 20, 2023 •

edited

Loading

adamsilverstein Nov 21, 2023

ramonjd Nov 21, 2023

adamsilverstein Nov 22, 2023

ramonjd Nov 23, 2023

andrewserong Nov 8, 2023

ramonjd Nov 8, 2023 •

edited

Loading

ramonjd Nov 9, 2023

andrewserong left a comment

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

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

Conversation

ramonjd commented Nov 3, 2023 • edited Loading

What?

Why?

Testing Instructions

github-actions bot commented Nov 3, 2023 • edited Loading

andrewserong left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonjd Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonjd Nov 20, 2023 • edited Loading

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

Choose a reason for hiding this comment

ramonjd Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewserong left a comment

Choose a reason for hiding this comment

ramonjd commented Nov 3, 2023 •

edited

Loading

github-actions bot commented Nov 3, 2023 •

edited

Loading

andrewserong left a comment •

edited

Loading

ramonjd Nov 8, 2023 •

edited

Loading

ramonjd Nov 20, 2023 •

edited

Loading

ramonjd Nov 8, 2023 •

edited

Loading