-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
lib/compat/wordpress-6.3/class-gutenberg-rest-global-styles-controller-6-3.php
Outdated
Show resolved
Hide resolved
lib/compat/wordpress-6.3/class-gutenberg-rest-global-styles-controller-6-3.php
Outdated
Show resolved
Hide resolved
lib/compat/wordpress-6.3/class-gutenberg-rest-global-styles-controller-6-3.php
Outdated
Show resolved
Hide resolved
lib/compat/wordpress-6.3/class-gutenberg-rest-global-styles-controller-6-3.php
Outdated
Show resolved
Hide resolved
lib/compat/wordpress-6.3/class-gutenberg-rest-global-styles-controller-6-3.php
Outdated
Show resolved
Hide resolved
lib/compat/wordpress-6.3/class-gutenberg-rest-global-styles-controller-6-3.php
Show resolved
Hide resolved
- 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
3fc0a73
to
01ab6f2
Compare
* | ||
* @see WP_REST_Controller | ||
*/ | ||
class Gutenberg_REST_Global_Styles_Revisions_Controller extends WP_REST_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.
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.
Thanks for all the nudges @spacedmonkey 🙇 |
cb23b7c
to
bee3b61
Compare
'format' => 'date-time', | ||
'context' => array( 'view', 'edit', 'embed' ), | ||
), | ||
'date_gmt' => 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.
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.
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.
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 😄
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.
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. 👍
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.
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)
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'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 🙂
Flaky tests detected in 041c382. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4804073257
|
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.
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( |
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.
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?
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 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() { |
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.
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?)
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.
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, |
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.
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!
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.
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', |
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.
Is this a stray comment? It looks identical to the following line 😄
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.
🤦
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 |
…ther details from the revision item not the parent Adding tests
* 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
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 theGutenberg_REST_Global_Styles_Revisions_Controller
class.Much inspiration is taken from
WP_REST_Revisions_Controller
.Response example:
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 :
Check the network request and response.