#60131 closed enhancement (fixed)
Global styles controllers: extend classes with WP_REST_Posts_Controller and WP_REST_Revisions_Controller
Reported by: |
|
Owned by: |
|
---|---|---|---|
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 )
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
withWP_REST_Posts_Controller
. - extend
WP_REST_Global_Styles_Revisions_Controller
withWP_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
@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
@
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.
This ticket was mentioned in PR #6022 on WordPress/wordpress-develop by @spacedmonkey.
12 months ago
#10
Trac ticket: https://core.trac.wordpress.org/ticket/60131
#11
@
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
@
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.
#20
@
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
withWP_REST_Posts_Controller
@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.
$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 )
);
@spacedmonkey commented on PR #5699:
12 months ago
#23
Committed.
#26
@
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
@
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!
This ticket was mentioned in PR #6105 on WordPress/wordpress-develop by @spacedmonkey.
12 months ago
#29
Trac ticket: https://core.trac.wordpress.org/ticket/60131
#30
@
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
@
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?
#32
@
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
@
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
@
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.
#36
@
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
@
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
withWP_REST_Revisions_Controller
- extends
WP_REST_Global_Styles_Controller
withWP_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
- Ensure that the revisions appear in the global styles revisions panel.
- 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
@
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.
#41
@
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.
@ramonopoly commented on PR #6573:
9 months ago
#43
Closing in favour of https://github.com/WordPress/wordpress-develop/pull/6105
#44
@
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
@
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
@
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
@
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
@
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.
@swissspidy commented on PR #6573:
9 months ago
#51
Committed in https://core.trac.wordpress.org/changeset/58225
This change removes a lot of the duplication between
WP_REST_Global_Styles_Revisions_Controller
andWP_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:
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
WP_REST_Global_Styles_Revisions_Controller
andWP_REST_Revisions_Controller
register_routes
andget_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
get_item
but we'll still need the single revision route)