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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Nov 9, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
09affe5
Basic setup:
ramonjd Nov 3, 2023
a000125
Expected 1 blank line at end of file; 2 found
ramonjd Nov 3, 2023
7472e68
Add test for get_item
ramonjd Nov 6, 2023
ec9b0e4
Add ticket number and 404 test
ramonjd Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions lib/compat/wordpress-6.4/rest-api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,3 @@ function gutenberg_register_rest_block_patterns_routes() {
$block_patterns->register_routes();
}
add_action( 'rest_api_init', 'gutenberg_register_rest_block_patterns_routes' );

/**
* Registers the Global Styles Revisions REST API routes.
*/
function gutenberg_register_global_styles_revisions_endpoints() {
$global_styles_revisions_controller = new Gutenberg_REST_Global_Styles_Revisions_Controller_6_4();
$global_styles_revisions_controller->register_routes();
}
add_action( 'rest_api_init', 'gutenberg_register_global_styles_revisions_endpoints' );
102 changes: 102 additions & 0 deletions lib/compat/wordpress-6.5/class-gutenberg-rest-global-styles-revisions-controller-6-5.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
<?php
/**
* REST API: Gutenberg_REST_Global_Styles_Revisions_Controller class, inspired by WP_REST_Revisions_Controller.
*
* @package WordPress
* @subpackage REST_API
* @since 6.3.0
*/

/**
* Core class used to access global styles revisions via the REST API.
*
* @since 6.3.0
*
* @see WP_REST_Controller
*/
class Gutenberg_REST_Global_Styles_Revisions_Controller_6_5 extends Gutenberg_REST_Global_Styles_Revisions_Controller_6_4 {
/**
* Registers the controller's routes.
*
* @since 6.3.0
* @since 6.5.0 Adds route to fetch individual global styles revisions.
*/
public function register_routes() {
register_rest_route(
$this->namespace,
'/' . $this->parent_base . '/(?P<parent>[\d]+)/' . $this->rest_base . '/(?P<id>[\d]+)',
array(
'args' => array(
'parent' => array(
'description' => __( 'The ID for the parent of the global styles revision.' ),
'type' => 'integer',
),
'id' => array(
'description' => __( 'Unique identifier for the global styles revision.' ),
'type' => 'integer',
),
),
array(
'methods' => WP_REST_Server::READABLE,
'callback' => array( $this, 'get_item' ),
'permission_callback' => array( $this, 'get_item_permissions_check' ),
'args' => array(
'context' => $this->get_context_param( array( 'default' => 'view' ) ),
),
),
'schema' => array( $this, 'get_public_item_schema' ),
)
);
parent::register_routes();
}

/**
* Retrieves one global styles revision from the collection.
*
* @since 6.5.0
*
* @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 ) {
$parent = $this->get_parent( $request['parent'] );
if ( is_wp_error( $parent ) ) {
return $parent;
}

$revision = $this->get_revision( $request['id'] );
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@ramonjd ramonjd Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@ramonjd ramonjd Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 😅

Copy link
Member

Choose a reason for hiding this comment

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

+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.

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 have a Core PR in progress here:

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.

if ( is_wp_error( $revision ) ) {
return $revision;
}

$response = $this->prepare_item_for_response( $revision, $request );
return rest_ensure_response( $response );
}

/**
* Get the global styles revision, if the ID is valid.
*
* @since 6.5.0
*
* @param int $id Supplied ID.
* @return WP_Post|WP_Error Revision post object if ID is valid, WP_Error otherwise.
*/
protected function get_revision( $id ) {
$error = new WP_Error(
'rest_post_invalid_id',
__( 'Invalid revision ID.' ),
array( 'status' => 404 )
);

if ( (int) $id <= 0 ) {
return $error;
}

$revision = get_post( (int) $id );
if ( empty( $revision ) || empty( $revision->ID ) || 'revision' !== $revision->post_type ) {
return $error;
}
Comment on lines +95 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤔

Copy link
Member Author

@ramonjd ramonjd Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Screenshot 2023-11-08 at 5 32 20 pm

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

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


return $revision;
}
}
21 changes: 21 additions & 0 deletions lib/compat/wordpress-6.5/rest-api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?php
/**
* PHP and WordPress configuration compatibility functions for the Gutenberg
* editor plugin changes related to REST API.
*
* @package gutenberg
*/

if ( ! defined( 'ABSPATH' ) ) {
die( 'Silence is golden.' );
}

/**
* Registers the Global Styles Revisions REST API routes.
*/
function gutenberg_register_global_styles_revisions_endpoints() {
$global_styles_revisions_controller = new Gutenberg_REST_Global_Styles_Revisions_Controller_6_5();
$global_styles_revisions_controller->register_routes();
}

add_action( 'rest_api_init', 'gutenberg_register_global_styles_revisions_endpoints' );
4 changes: 4 additions & 0 deletions lib/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ function gutenberg_is_experiment_enabled( $name ) {
require_once __DIR__ . '/compat/wordpress-6.4/rest-api.php';
require_once __DIR__ . '/compat/wordpress-6.4/theme-previews.php';

// WordPress 6.5 compat.
require_once __DIR__ . '/compat/wordpress-6.5/class-gutenberg-rest-global-styles-revisions-controller-6-5.php';
require_once __DIR__ . '/compat/wordpress-6.5/rest-api.php';

// Plugin specific code.
require_once __DIR__ . '/class-wp-rest-global-styles-controller-gutenberg.php';
require_once __DIR__ . '/rest-api.php';
Expand Down
45 changes: 38 additions & 7 deletions phpunit/class-gutenberg-rest-global-styles-revisions-controller-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,11 @@ public function test_register_routes() {
$routes,
'Global style revisions based on the given parentID route does not exist.'
);
$this->assertArrayHasKey(
'/wp/v2/global-styles/(?P<parent>[\d]+)/revisions/(?P<id>[\d]+)',
$routes,
'Single global style revisions based on the given parentID and revision ID route does not exist.'
);
}

/**
Expand Down Expand Up @@ -298,6 +303,38 @@ public function test_get_items() {
$this->check_get_revision_response( $data[2], $this->revision_1 );
}

/**
* @ticket 59810
*
* @covers Gutenberg_REST_Global_Styles_Revisions_Controller_6_4::get_item
*/
public function test_get_item() {
wp_set_current_user( self::$admin_id );

$request = new WP_REST_Request( 'GET', '/wp/v2/global-styles/' . self::$global_styles_id . '/revisions/' . $this->revision_1_id );
$response = rest_get_server()->dispatch( $request );
$data = $response->get_data();

$this->assertSame( 200, $response->get_status(), 'Response status is 200.' );
$this->check_get_revision_response( $data, $this->revision_1 );
}

/**
* @ticket 59810
*
* @covers Gutenberg_REST_Global_Styles_Revisions_Controller_6_4::get_revision
*/
public function test_get_item_invalid_revision_id_should_error() {
wp_set_current_user( self::$admin_id );

$expected_error = 'rest_post_invalid_id';
$expected_status = 404;
$request = new WP_REST_Request( 'GET', '/wp/v2/global-styles/' . self::$global_styles_id . '/revisions/20000001' );
$response = rest_get_server()->dispatch( $request );

$this->assertErrorResponse( $expected_error, $response, $expected_status );
}

/**
* @ticket 58524
*
Expand All @@ -320,20 +357,14 @@ public function test_get_item_schema() {
$this->assertArrayHasKey( 'modified', $properties, 'Schema properties array has "modified" key.' );
$this->assertArrayHasKey( 'modified_gmt', $properties, 'Schema properties array has "modified_gmt" key.' );
}

/**
* @doesNotPerformAssertions
*/
public function test_context_param() {
// Controller does not implement test_context_param().
}

/**
* @doesNotPerformAssertions
*/
public function test_get_item() {
// Controller does not implement get_item().
}

/**
* @doesNotPerformAssertions
*/
Expand Down
Loading