Make WordPress Core

Opened 4 months ago

Last modified 7 weeks ago

#60131 reopened enhancement

Global styles controllers: extend classes with WP_REST_Posts_Controller and WP_REST_Revisions_Controller

Reported by: ramonopoly's profile ramonopoly Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.6 Priority: normal
Severity: normal Version: 6.3
Component: REST API Keywords: has-patch has-unit-tests dev-feedback
Focuses: Cc:

Description (last modified by ramonopoly)

As part of the efforts to improve the maintainability of Global Styles REST controllers, and to remove a lot of duplicate code we can:

  • extend WP_REST_Global_Styles_Controller with WP_REST_Posts_Controller.
  • extend WP_REST_Global_Styles_Revisions_Controller with WP_REST_Revisions_Controller.

The Global styles REST response has the following characteristics:

  • Doesn't return excerpt, content, guid fields
  • Does some JSON parsing when preparing the response

In other words, the functionality was "special" enough not to warrant extending the base controller instead of using WP_REST_Posts_Controller. Also, global styles were new and evolving.

For revisions especially, more code was required to override the parent class's functions, and having a stand alone class left more elbow room for subsequent tweaks and changes.

Now that global styles have "settled" however, and shares more methods with WP_REST_Posts_Controller and WP_REST_Revisions_Controller, it makes sense to change the base classes for more consistent interface, less code and less maintenance burden.

See discussion and related PRs:

Change History (35)

This ticket was mentioned in PR #5699 on WordPress/wordpress-develop by @ramonopoly.


4 months ago
#1

  • Keywords has-patch has-unit-tests added

This change removes a lot of the duplication between WP_REST_Global_Styles_Revisions_Controller and WP_REST_Revisions_Controller by extending the former with the latter.

## Context

The global styles revisions controller was introduced in https://github.com/WordPress/gutenberg/pull/49974.

Originally, global styles revisions were a bit new and had the following characteristics:

  • Didn't, and still doesn't, return excerpt, content, guid fields
  • 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.

Now that global styles revisions have "settled", and shares more methods with WP_REST_Revisions_Controller, it makes sense to change the base class for more consistent interface and less maintenance burden.

See discussion: https://github.com/WordPress/gutenberg/pull/55827#discussion_r1399856581

## What this change does

  1. Extends WP_REST_Global_Styles_Revisions_Controller and WP_REST_Revisions_Controller
  2. Removes shared public methods
  3. Updates schema to use parent and parent controller's properties. Unsets unnecessary properties.
  4. Updates relevant tests. Deletes dupes.
  5. Overrides key methods such as register_routes and get_items. The former because we want to rule out deletion endpoints for now, the latter because we need to parse theme JSON before returning a response.

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

## Related PRs

@ramonopoly commented on PR #5699:


4 months ago
#2

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

@ramonopoly commented on PR #5699:


4 months ago
#3

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

[https://github.com/WordPress/wordpress-develop/pull/5699/files#annotation_16570999825 [chromium] › gutenberg-plugin.test.js:25:6 › Gutenberg plugin › should activate: tests/e2e/specs/gutenberg-plugin.test.js#L12]
  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',

#5 @spacedmonkey
3 months ago

Side note, I believe that global styles endpoints, should just extend using post controller and revisions controllers. Under the hood they are just post types. They should not be treated differently.

Specially now we can have custsom revisions endpoints for different post types. See #56922.

@ramonopoly commented on PR #5699:


3 months ago
#6

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: https://github.com/WordPress/gutenberg/pull/35801#pullrequestreview-788267865

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 commented on PR #5699:


3 months ago
#7

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 https://github.com/WordPress/wordpress-develop/commit/1f51e1f4f6c81238fffed706ff165795ea34d522 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.

@ramonopoly commented on PR #5699:


2 months ago
#8

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

Started here. More stuff and tests to come 👍🏻

@ramonopoly commented on PR #5699:


2 months ago
#9

The big difference is now https://github.com/WordPress/wordpress-develop/commit/1f51e1f4f6c81238fffed706ff165795ea34d522 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 https://github.com/WordPress/wordpress-develop/pull/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?

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

#11 @spacedmonkey
2 months ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 6.5
  • Owner set to spacedmonkey
  • Status changed from new to assigned

Marking #6022 as ready to commit.

@spacedmonkey commented on PR #5699:


2 months ago
#12

Closing in favour of #6022

@ramonopoly commented on PR #5699:


2 months ago
#13

Closing in favour of https://github.com/WordPress/wordpress-develop/pull/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.

#14 @ramonopoly
2 months ago

  • Description modified (diff)
  • Summary changed from Global styles revisions controller: extend with WP_REST_Revisions_Controller to Global styles controllers: extend classes with WP_REST_Posts_Controller and WP_REST_Revisions_Controller

@spacedmonkey commented on PR #5699:


2 months ago
#15

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

@ramonjd I thought we were aligned on https://github.com/WordPress/wordpress-develop/pull/6022? I feel like #6022 is good to commit.

@ramonopoly commented on PR #5699:


2 months ago
#16

@ramonjd I thought we were aligned on https://github.com/WordPress/wordpress-develop/pull/6022? I feel like https://github.com/WordPress/wordpress-develop/pull/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.

jonnynews commented on PR #5699:


2 months ago
#17

@ramonjd To be clear, https://github.com/WordPress/wordpress-develop/pull/6022 needs to be part of this PR. Let's do this change in one commit.

@ramonopoly commented on PR #5699:


2 months ago
#18

To be clear, https://github.com/WordPress/wordpress-develop/pull/6022 needs to be part of this PR. Let's do this change in one commit.

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

https://github.com/WordPress/wordpress-develop/pull/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.

#19 @ramonopoly
2 months ago

  • Description modified (diff)

#20 @ramonopoly
2 months ago

https://github.com/WordPress/wordpress-develop/pull/5699 now:

  • extends WP_REST_Global_Styles_Revisions_Controller with WP_REST_Revisions_Controller
  • extends WP_REST_Global_Styles_Controller with WP_REST_Posts_Controller
Last edited 2 months ago by ramonopoly (previous) (diff)

@ramonopoly commented on PR #5699:


2 months ago
#21

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 )
                );

#22 @spacedmonkey
2 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 57624:

REST API: Refactor global styles endpoints in REST API to register with post type.

Updated the global styles endpoints in the REST API to extend from existing posts and revisions controllers. This reduces duplicated code and inconsistencies. The revisions controller is now a subclass of the WP_REST_Revisions_Controller. Related redundant methods were removed and schema generation and collection parameters were adjusted to suit the global styles context. Updated permission checks, constructor, and collection parameters accordingly. This change allows for easy override of these classes using the register_post_type_args filter.

Props ramonopoly, spacedmonkey, mukesh27.
Fixes #60131.

@spacedmonkey commented on PR #5699:


2 months ago
#23

Committed.

#24 @spacedmonkey
2 months ago

  • Keywords needs-dev-note added

#25 @davidbaumwald
2 months ago

In 57628:

REST API: Revert the refactor of global styles endpoints in REST API in [57624].

[57624] introduced some E2E test failures which are the result of an incompatibility with the Gutenberg plugin.

Props jorbin, spacedmonkey, swissspidy, hellofromTonya, youknowriad, costdev.
See #60131.

#26 @swissspidy
2 months ago

  • Keywords dev-feedback added; commit needs-dev-note removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening because this was reverted again before beta 1 today due to an incompatibility with the same controller in Gutenberg.

It was discussed that we could consider landing a proper fix again before the next beta, else we would punt this to 6.6

https://github.com/WordPress/wordpress-develop/pull/6105 has since been updated with a change that should address the test failures. Now the question is whether there's consensus on this being the right approach.

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


2 months ago

#28 @ramonopoly
2 months ago

Reopening because this was reverted again before beta 1 today due to an incompatibility with the same controller in Gutenberg.

I can look into updating the controller in Gutenberg.

It was discussed that we could consider landing a proper fix again before the next beta

Was a specific "proper fix" discussed at all?

If so, could you elaborate on this ticket for the record?

I assume it's related to the parent post type not being passed, as per the E2E fix here:

https://github.com/WordPress/wordpress-develop/commit/95db1e9e668861c48ed974dd6562c8d1c63adf7d

Thank you!

Last edited 2 months ago by ramonopoly (previous) (diff)

#30 @spacedmonkey
2 months ago

@ramonopoly To update.

This ticket was committed in [57624] but was revert as part of beta 1 release, becuase unit tests are failing [57628].

The reason it was reverted, as the signuare of the WP_REST_Global_Styles_Revisions_Controller class found in core did not match the one found in gutenberg. As the constuctor and properties were different, this resulted in a PHP error, resulting in the e2e fail.

I have reopened the PR and added a fix. This fix is really simple and makes the class backwards compatible. Need to reviews and testing if we want to get this committed in beta 2.

#31 @ramonopoly
2 months ago

I have reopened the PR and added a fix. This fix is really simple and makes the class backwards compatible. Need to reviews and testing if we want to get this committed in beta 2.

@spacedmonkey Great stuff, thanks for the update.

Looks good at first glance. I'll give it a run today.

The reason it was reverted, as the signuare of the WP_REST_Global_Styles_Revisions_Controller class found in core did not match the one found in gutenberg. As the constuctor and properties were different, this resulted in a PHP error, resulting in the e2e fail.

Out of interest, which constructor's signature was the problem?

This one?

https://github.com/WordPress/gutenberg/blob/wp/6.3/lib/compat/wordpress-6.3/class-gutenberg-rest-global-styles-revisions-controller-6-3.php#L39

#32 @ramonopoly
2 months ago

Out of interest, which constructor's signature was the problem?

Okay, sorry. Now I see.

Fatal error: Uncaught ArgumentCountError: Too few arguments to function WP_REST_Global_Styles_Revisions_Controller::__construct(), 0

Was a specific "proper fix" discussed at all?

I see the commmit in the message above.

Thanks everyone for helping with this!

#33 @swissspidy
2 months ago

  • Version changed from trunk to 6.3

The updated PR does appear to address the backward compatibility issues that were reported yesterday, so thanks for working on that!

It's a bit unfortunate that this had to be reverted last minute, but deadlines are not arbitrary and we wanted to get the beta out.

Beta is also the cutoff for any enhancements, so technically this means we'll have to punt this to 6.6, and as a release squad member I'd vote for punting.

I’ve just looked through the ticket/PR again and it doesn’t seem to be a release blocker, since these classes were already introduced in 6.3 anyway. (Aside: #41172 is related).

@ramonopoly Since you opened this ticket, feel free to provide more context in case you think this absolutely must not be punted to 6.6, despite being an enhancement. This would then be a good topic for dev chat.

#34 @ramonopoly
2 months ago

Since you opened this ticket, feel free to provide more context in case you think this absolutely must not be punted to 6.6, despite being an enhancement. This would then be a good topic for dev chat.

Thanks for the explanation @swissspidy

In my mind these changes represent a nice-to-have refactor, which I don't believe affects any 6.5 functionality.

Punting to 6.6 is a fine option.

Thanks again.

#35 @swissspidy
2 months ago

  • Milestone changed from 6.5 to 6.6
Note: See TracTickets for help on using tickets.