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

Rest API: add /revisions endpoint for global styles #4606

Closed
wants to merge 21 commits into from
Closed

Rest API: add /revisions endpoint for global styles #4606

wants to merge 21 commits into from

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Jun 13, 2023

The following Global Revisions UI was added to Gutenberg in:

To support this, an endpoint (/wp/v2/global-styles/revisions) that returns revisions for the global styles custom post was added and updated in:

This PR adds the endpoint to Core and adds tests.

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

Testing

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 :

await wp.apiFetch( { url: `/index.php?rest_route=%2Fwp%2Fv2%2Fglobal-styles%2F${ wp.data.select('core').__experimentalGetCurrentGlobalStylesId() }%2Frevisions&_locale=user` } );

npm run test:php -- --filter WP_REST_Global_Styles_Revisions_Controller_Test

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@ramonjd ramonjd force-pushed the add/rest-api-global-revisions-controller branch from 268d816 to 82b6ddd Compare June 13, 2023 21:35
@ramonjd ramonjd force-pushed the add/rest-api-global-revisions-controller branch from ef8ab8a to 40f7e65 Compare June 14, 2023 02:05
@ramonjd ramonjd mentioned this pull request Jun 14, 2023
Merged
mukeshpanchal27
mukeshpanchal27 reviewed Jun 16, 2023
View reviewed changes
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks @ramonjd, Can you add ticket number in unit tests?

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-revisions-controller.php Outdated Show resolved Hide resolved
TimothyBJacobs
TimothyBJacobs reviewed Jun 22, 2023
View reviewed changes
tests/phpunit/tests/rest-api/rest-global-styles-revisions-controller.php Outdated
* @doesNotPerformAssertions
*/
public function test_context_param() {
// Controller does not use get_context_param().
Copy link
Member

Choose a reason for hiding this comment

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

The controller does have context set, so this should get tests.

Copy link
Member Author

@ramonjd ramonjd Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for that.

I was looking at how other tests implement test_context_param() (example), and they're testing the data in $data['endpoints'] after an OPTIONS call.

This controller doesn't have any other endpoint, e.g., WP_REST_Server::CREATABLE and so I'm not sure how to test it. $data['endpoints'] doesn't have any testable data. Any tips?

I'll add another get_items test to check that the correct fields are returned however.

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've added a test test_get_item_embed_context to check the difference when sending context as an arg in the GET request

peterwilsoncc
peterwilsoncc reviewed Jun 23, 2023
View reviewed changes
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

A couple of nit picks, really.

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-revisions-controller.php Outdated Show resolved Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-revisions-controller.php Outdated Show resolved Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-revisions-controller.php Outdated Show resolved Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-revisions-controller.php Outdated Show resolved Hide resolved
@ramonjd ramonjd force-pushed the add/rest-api-global-revisions-controller branch 2 times, most recently from 92207a8 to de82ac9 Compare June 23, 2023 05:52
@ramonjd
Copy link
Member Author

ramonjd commented Jun 23, 2023

Thank @TimothyBJacobs @spacedmonkey and @peterwilsoncc for helping me make this patch better.

I think I've addressed all of the feedback, (except for the context test) at least I think so. Please let me know where it can be improved.

@ramonjd ramonjd force-pushed the add/rest-api-global-revisions-controller branch from d97362a to 41a765e Compare June 23, 2023 07:05
This was referenced Jun 25, 2023
Merged
Closed
@ramonjd ramonjd force-pushed the add/rest-api-global-revisions-controller branch from 1ec3540 to f9bb652 Compare June 26, 2023 23:37
peterwilsoncc
peterwilsoncc reviewed Jun 27, 2023
View reviewed changes
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

A few nits but otherwise looks good.

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-revisions-controller.php Outdated Show resolved Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-revisions-controller.php Outdated Show resolved Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-revisions-controller.php Outdated Show resolved Hide resolved
tests/phpunit/tests/rest-api/rest-global-styles-revisions-controller.php Outdated Show resolved Hide resolved
spacedmonkey
spacedmonkey approved these changes Jun 27, 2023
View reviewed changes
Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

This change looks good. The unit tests could do with @covers.

@tellthemachines
Copy link
Contributor

committed in r56082 / c345180.

ramonjd added a commit to ramonjd/wordpress-develop that referenced this pull request Jun 29, 2023
@ramonjd
Implementing feedback from WordPress#4606
d4ea5a8 
Formatting and test annotations to include @Covers
@ramonjd ramonjd mentioned this pull request Jun 29, 2023
Closed
@ramonjd
Copy link
Member Author

ramonjd commented Jun 29, 2023

The unit tests could do with @Covers.

Thanks! PR for that here: #4750

@ramonjd ramonjd mentioned this pull request Jun 29, 2023
Merged
ramonjd added a commit to WordPress/gutenberg that referenced this pull request Jun 29, 2023
@ramonjd
backporting from WordPress/wordpress-develop#4606
7c56354
ramonjd added a commit to WordPress/gutenberg that referenced this pull request Jun 29, 2023
@ramonjd
backporting from WordPress/wordpress-develop#4606 (#52095)
b2b968d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterwilsoncc peterwilsoncc peterwilsoncc left review comments

@TimothyBJacobs TimothyBJacobs TimothyBJacobs left review comments

@mukeshpanchal27 mukeshpanchal27 mukeshpanchal27 left review comments

@spacedmonkey spacedmonkey spacedmonkey approved these changes

@kadamwhite kadamwhite Awaiting requested review from kadamwhite

Assignees
No one assigned
Labels
None yet
Projects
No open projects
WordPress 6.3.x Editor Tasks
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ramonjd @tellthemachines @spacedmonkey @peterwilsoncc @TimothyBJacobs @mukeshpanchal27