Skip to content
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

Rest API: add /revisions endpoint for global styles #49974

Merged
merged 6 commits into from
Apr 26, 2023

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Apr 21, 2023

Parent issue:

What?

Adds an endpoint that returns revisions for the global styles custom post.

/wp/v2/global-styles/revisions

Plucked from the experimental branch:

Why?

In order to display revisions for global styles in the UI. See explorations in #49601 and #46667 for context.

How?

Registering a new /revisions endpoint in the Gutenberg_REST_Global_Styles_Revisions_Controller class.
Much inspiration is taken from WP_REST_Revisions_Controller.

Response example:

[
    {
        "author": 1,
        "date": "2023-04-23T23:29:24",
        "date_gmt": "2023-04-23T23:29:24",
        "id": 6,
        "modified": "2023-04-24T05:15:03",
        "modified_gmt": "2023-04-24T05:15:03",
        "parent": 4,
        "slug": "wp-global-styles-twentytwentythree",
        "settings": {},
        "styles": {
            "color": {
                "background": "var:preset|color|vivid-purple"
            }
        },
        "date_display": "5 hours ago (24 Apr @ 00:58)",
        "author_display_name": "admin",
        "author_avatar_url": "http://2.gravatar.com/avatar/ea8b076b398ee48b71cfaecf898c582b?s=24&d=mm&r=g"
    }
]

Explanations

Why not extend WP_REST_Revisions_Controller?

I did try this, but WP_REST_Revisions_Controller has a lot of custom methods and the response is geared towards revisions from posts/pages/templates that have rendered HTML content.

Also I wanted to reduce the schema and ensure maximum flexibility for global styles, which are its own class of citizen.

Testing Instructions

Run the tests!!!

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

To test the endpoint in the site editor, fire up this branch and head over to the site editor.

In your browser's console, switch to the network tab and run the following :

await wp.apiFetch( { url: `/index.php?rest_route=%2Fwp%2Fv2%2Fglobal-styles%2F${ wp.data.select('core').__experimentalGetCurrentGlobalStylesId() }%2Frevisions&_locale=user` } );

Screenshot 2023-04-21 at 1 47 40 pm

Check the network request and response.

@ramonjd ramonjd self-assigned this Apr 21, 2023
@ramonjd ramonjd added REST API Interaction Related to REST API Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Apr 21, 2023
@ramonjd ramonjd marked this pull request as ready for review April 21, 2023 03:54
@ramonjd ramonjd requested a review from spacedmonkey as a code owner April 21, 2023 03:54
ramonjd added 4 commits April 24, 2023 16:21
- adding /revisions endpoint to global styles API
- tests
…erg_REST_Global_Styles_Controller that we can overwrite in the subclass. It makes for less code and a neater abstraction.
…classes into Gutenberg_REST_Global_Styles_Revisions_Controller

Returning revisions URL in prepare_links
@ramonjd ramonjd force-pushed the add/global-styles-revisions-rest-api branch from 3fc0a73 to 01ab6f2 Compare April 24, 2023 06:31
*
* @see WP_REST_Controller
*/
class Gutenberg_REST_Global_Styles_Revisions_Controller extends WP_REST_Controller {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did start with extending WP_REST_Revisions_Controller, but global style revisions need special treatment, e.g.,

  • JSON instead of rendered HTML
  • We only need one route right now (to fetch some revisions for the UI)
  • I wanted to add some custom properties/schema

The methods I had to overwrite contained just as much code as doing it from scratch.

Happy to go back to extending WP_REST_Revisions_Controller if folks think it's better longer term.

@ramonjd
Copy link
Member Author

ramonjd commented Apr 24, 2023

Thanks for all the nudges @spacedmonkey 🙇

@ramonjd ramonjd force-pushed the add/global-styles-revisions-rest-api branch from cb23b7c to bee3b61 Compare April 24, 2023 06:59
'format' => 'date-time',
'context' => array( 'view', 'edit', 'embed' ),
),
'date_gmt' => array(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need half of these properties, at least for the purposes of planned UI changes: #49601

I just added them because they were in the revisions controller.

I've a mind to remove them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which ones were you thinking of removing?

In principle, I quite like the idea of trying to keep a close parallel between the two revisions endpoints so that there's predictability between the endpoints if someone were to write a plugin or other tool that wants to call the endpoints — but I have only just started looking at this PR 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I was wondering whether to include only a minimal set of properties that will serve to create the revisions UI (see #49601), and leave it open for extension later, but you're right about predictability. There's no harm in leaving them in. 👍

Copy link
Member Author

@ramonjd ramonjd Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which ones were you thinking of removing?

I've already left out slug, but was thinking of removing dates (except for the display one).

Do you reckon I should reinstate slug? I guess it could be at least used as a key for iterable components

Here's a raw preview of all standard, post revision props (ignore the rendered content)

Screenshot 2023-04-24 at 12 08 45 pm

Copy link
Contributor

@andrewserong andrewserong Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already left out slug, but was thinking of removing dates (except for the display one).

Personally, I'd keep all the dates ones so that they're filterable / sortable by the consumer.

I've already left out slug, but was thinking of removing dates (except for the display one).

We've already got id, so slug might not be all that useful, especially since title isn't present in the response, and users aren't able to update the title of the global styles entry, anyway?

I quite like the balance that you've already got in this PR, it's looking quite good to me 🙂

@github-actions
Copy link

github-actions bot commented Apr 24, 2023

Flaky tests detected in 041c382.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4804073257
📝 Reported issues:

@ramonjd ramonjd changed the title Rest API: add /revisions endpoint to global styles controller Rest API: add /revisions endpoint for global styles Apr 24, 2023
Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work honing in on an MVP for this endpoint @ramonjd! It's all testing nicely for me:

✅ Responses return the expected data
✅ Tests pass and appear to have an appropriate level of coverage
✅ Accessing the endpoint directly correctly fails as a non-logged in user (401)
✅ Accessing the endpoint while logged in as a non-admin user (e.g. Author) correctly 403s
✅ Link to the new endpoint is correctly added to the existing global styles endpoint
✅ Schema returned looks correct, based on an OPTIONS request:

await wp.apiFetch( { method: 'OPTIONS', url: `/index.php?rest_route=%2Fwp%2Fv2%2Fglobal-styles%2F${ wp.data.select('core').__experimentalGetCurrentGlobalStylesId() }%2Frevisions&_locale=user` } );

I've left a few comments, mostly just ideas or thoughts for things to add in potential follow-up PRs, and likely to be added before a core merge for 6.3 to make sure that the endpoint is well-rounded enough to feel consistent with the revisions endpoint. For now, I think this a great place to begin, with a simple collection endpoint that only returns the latest 100 revisions 👍

Also, I quite like the decision to copy / paste what was needed from the revisions endpoint rather than extending the existing class, so that it's possible to continue to experiment and add / change things in this endpoint safely within the plugin and in a single place. I imagine there'll be subsequent iterations prior to backporting to core anyway.

LGTM, nice work digging into all the details! ✨

* @return void
*/
public function register_routes() {
register_rest_route(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will there also be a route for accessing an individual revision / deleting a revision?

I imagine that's not required for the initial version of the endpoint (exposing a list of revisions in the site editor UI), but could be a good follow-up PR to ensure consistency?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I expect one day they'll both be needed. I haven't heard anything about it for 6.3.

*
* @return array Collection parameters.
*/
public function get_collection_params() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I imagine this isn't required for the initial goal for this endpoint, but would we eventually include support for common collection args (like per page, offset, order, etc so that paginated responses would be possible?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, and, now that you've raised it, I can probably remove it from this iteration. I think it was a leftover from experimentation. Thank you!

$user_theme_revisions = wp_get_post_revisions(
$parent->ID,
array(
'posts_per_page' => 100,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just following on — so to begin with, this endpoint only supports listing out the 100 most recent global styles revisions, in descending order. That sounds good to me!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the MVP 😄 It's possible we'll need finer query params in the future, but there are no requirements and nothing has been planned so it could be a long way off.

public function test_register_routes() {
$routes = rest_get_server()->get_routes();
$this->assertArrayHasKey(
// '/wp/v2/global-styles/(?P<parent>[\d]+)/revisions',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a stray comment? It looks identical to the following line 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦

@ramonjd
Copy link
Member Author

ramonjd commented Apr 26, 2023

Thanks @andrewserong 🙇 I'll update with the small changes to correct the minor oversights.

As more requirements come in I hope it'll serve as a good base to extend with functionality similar to WP_REST_Revisions_Controller

@ramonjd ramonjd merged commit 832da50 into trunk Apr 26, 2023
@ramonjd ramonjd deleted the add/global-styles-revisions-rest-api branch April 26, 2023 04:23
@github-actions github-actions bot added this to the Gutenberg 15.7 milestone Apr 26, 2023
@bph bph added the [Type] Experimental Experimental feature or API. label Apr 26, 2023
ramonjd added a commit that referenced this pull request Apr 27, 2023
…ther details from the revision item not the parent

Adding tests
ramonjd added a commit that referenced this pull request Apr 27, 2023
* Correcting a mistake I made in #49974: we need the author, date and other details from the revision item not the parent
Adding tests

* ICH LIEBE DICH, LINTER
@ramonjd ramonjd added the Needs PHP backport Needs PHP backport to Core label Jun 5, 2023
@ramonjd ramonjd removed the Needs PHP backport Needs PHP backport to Core label Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json REST API Interaction Related to REST API [Type] Experimental Experimental feature or API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants