Make WordPress Core

Opened 14 months ago

Closed 9 months ago

Last modified 9 months ago

#60131 closed enhancement (fixed)

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 needs-refresh
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 (53)

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


14 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:


14 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:


14 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
13 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:


13 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:


13 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:


12 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:


12 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
12 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:


12 months ago
#12

Closing in favour of #6022

@ramonopoly commented on PR #5699:


12 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
12 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:


12 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:


12 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:


12 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:


12 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
12 months ago

  • Description modified (diff)

#20 @ramonopoly
12 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
Version 0, edited 12 months ago by ramonopoly (next)

@ramonopoly commented on PR #5699:


12 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
12 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:


12 months ago
#23

Committed.

#24 @spacedmonkey
12 months ago

  • Keywords needs-dev-note added

#25 @davidbaumwald
12 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
12 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.


12 months ago

#28 @ramonopoly
12 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 12 months ago by ramonopoly (previous) (diff)

#30 @spacedmonkey
12 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
12 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
12 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
12 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
12 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
12 months ago

  • Milestone changed from 6.5 to 6.6

#36 @oglekler
9 months ago

@swissspidy and @ramonopoly we have 2.5 until next Beta 1 and there is quite a change ) Can you check that patch is still valid and write testing instruction? Thank you 🙏

#37 @ramonopoly
9 months ago

Thanks @oglekler!

As mentioned over in https://core.trac.wordpress.org/ticket/59810#comment:20 I think @spacedmonkey's PR for this ticket would also close #59810.

I can give the PR a re-test as soon as I can. There have been no changes to the revisions endpoint so I think it's still good.

I don't have access to edit the PR description but can add test instructions in a comment if @spacedmonkey is not around.

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


9 months ago
#38

[!IMPORTANT]
This is a rebased version of https://github.com/WordPress/wordpress-develop/pull/6105. I don't have rights to update or push to that branch.

This change:

  • extends WP_REST_Global_Styles_Revisions_Controller with WP_REST_Revisions_Controller
  • extends WP_REST_Global_Styles_Controller with WP_REST_Posts_Controller

As part of this work, the PR also:

  • Removes shared public methods
  • Updates schema to use parent and parent controller's properties. Unsets unnecessary properties.
  • Updates relevant tests. Deletes dupes.

## Testing

Head over to the site editor, make a few changes to global styles.

https://github.com/WordPress/wordpress-develop/assets/6458278/39d779fa-7b12-43aa-87be-52726aaec050

  1. Ensure that the revisions appear in the global styles revisions panel.
  2. Check that you can apply them to the editor canvas
// Get all revisions.
await wp.data.resolveSelect( 'core' ).getRevisions( 'root', 'globalStyles', await wp.data.resolveSelect( 'core' ).__experimentalGetCurrentGlobalStylesId() ) 

// Get a single revision, where <REVISION_ID> is the id of the single revision.
await wp.data.resolveSelect( 'core' ).getRevision( 'root', 'globalStyles', await wp.data.resolveSelect( 'core' ).__experimentalGetCurrentGlobalStylesId(), <REVISION_ID> ) 

#39 @ramonopoly
9 months ago

I've retested the current patch and it works to my satisfaction in the editor.

Because I can't update the current PR, I've pushed a version to https://github.com/WordPress/wordpress-develop/pull/6573, with updates based on @swissspidy's feedback.

I've also added some testing instructions.

#40 @spacedmonkey
9 months ago

@ramonopoly i am still good to commit this release?

#41 @ramonopoly
9 months ago

i am still good to commit this release?

Yeah for sure! Thank you!

I've retested and it works as expected (still).

If you want to carry on with https://github.com/WordPress/wordpress-develop/pull/6105 I'll just close my most recent PR. I only put it up to add @swissspidy's feedback and rebase, and to add test instructions.

@spacedmonkey commented on PR #6105:


9 months ago
#42

@swissspidy Do you mind doing a review of this PR. I like think this PR is ready to commit.

#44 @oglekler
9 months ago

@spacedmonkey,
Just a reminder that we have 1 week before Beta 1, and it isn't a good idea to do things at the last moment, so, please proceed. If you need testing, please provide testing instructions and mark this ticket accordingly. Thank you! :)

#45 @spacedmonkey
9 months ago

  • Keywords needs-refresh added

This patch needs a refresh after #41172 has been committed. Do you mind taking a look either @swissspidy or @ramonopoly as I am away at the moment.

@oglekler 100% agree. This was committed after minute on the last release and then reverted. Not doing that again.

#46 @ramonopoly
9 months ago

Do you mind taking a look either @swissspidy or @ramonopoly as I am away at the moment.

I can reopen https://github.com/WordPress/wordpress-develop/pull/6573 and bring the suggestions across.

I don't have rights to push to your PR unfortunately.

#47 @ramonopoly
9 months ago

I've reopened https://github.com/WordPress/wordpress-develop/pull/6573 with the latest updates and suggestions by @swissspidy

I retested and I think it's right to go.

Thanks!

#48 @spacedmonkey
9 months ago

I am back at my desktop on Thursday. If @swissspidy is happy, you can take over and commit otherwise I will try and retest and commit by the end of the week.

@swissspidy commented on PR #6105:


9 months ago
#49

Closing in favor of #6573 so we can finalize the patch there.

#50 @swissspidy
9 months ago

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

In 58225:

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.

This reintroduces [57624] (reverted in [57628]) with improved backward compatibility and further enhancements.

Props ramonopoly, spacedmonkey, mukesh27, swissspidy.
Fixes #60131.

#52 @swissspidy
9 months ago

#59810 was marked as a duplicate.

#53 @ramonopoly
9 months ago

Thanks for all the work on this @spacedmonkey and @swissspidy!

Note: See TracTickets for help on using tickets.