Revisions Controller: return single revision only when it matches the parent id #5655

ramonjd · 2023-11-10T05:13:58Z

This patch adds a condition to WP_REST_Revisions_Controller->get_item that checks the revision parent id against the supplied parent id.

If it doesn't match, the method returns a 404.

This is to ensure that get_item does not return a revision whose parent does not match the parent route fragment.

Test coverage added.

Run tests: npm run test:php -- --filter WP_Test_REST_Revisions_Controller

Commit 1a7ff6c demonstrates that currently, get_item() can return the revision of different parent post of that same type.

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

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.

tests/phpunit/tests/rest-api/rest-revisions-controller.php

andrewserong

Thanks for following up with this fix! Looks like a good approach to me, just left a tiny comment about the value used in the error message.

src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php

tests/phpunit/tests/rest-api/rest-revisions-controller.php

src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php

…se parent does not match the `parent` route fragment.

…t the supplied parent id. If it doesn't match, return a 404 Updated test coverage Update error message in class-wp-rest-revisions-controller.php

danielbachhuber

Looks good! Thanks for your work on this, @ramonjd

I'll let @adamsilverstein give it a final review.

src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php

string replacement type signifier %d Co-authored-by: Daniel Bachhuber <daniel@bachhuber.co>

ramonjd · 2023-11-19T20:18:03Z

Thanks, folks. I appreciate the testing and reviews!

adamsilverstein

Nice work!

jonnynews · 2023-11-27T17:06:38Z

@ramonjd Has this change been tested with https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-template-revisions-controller.php controller that extends this controller?

ramonjd · 2023-11-27T20:38:37Z

Has this change been tested with https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-template-revisions-controller.php controller that extends this controller?

Good point. I can add a relevant unit test to the WP_REST_Template_Revisions_Controller test suite.

ramonjd · 2023-11-28T02:59:58Z

~~I can confirm that we'll need to overwrite get_item() in WP_REST_Template_Revisions_Controller to account for the wp_id > parent relationship between template record and their revisions.~~

~~I'll do this and add a test to this PR since it's related, and also check any other post types that extend this controller.~~

No. I was mixing up the results of get_post and WP_REST_Template_Revisions_Controller.

See below.

ramonjd · 2023-11-28T03:25:39Z

I can confirm that we'll need to overwrite get_item() in WP_REST_Template_Revisions_Controller to account for the wp_id > parent relationship between template record and their revisions.

Actually, withdraw that, I should have checked WP_REST_Revisions_Controller::get_item before posting.

The check in get_item that this PR introduces does so by comparing the results of get_post for both the parent and the revision.

The results of get_post is independent of the WP_REST_Template_Revisions_Controller which performs all the wp_id faffery: get_post will return integer IDs and parent IDs for template posts and revisions alike.

Regardless, WP_REST_Template_Revisions_Controller has unit tests for get_item, which would have failed otherwise.

I've also double checked other registered post types, none of which, wp_template and wp_template_part notwithstanding, define a custom revisions_rest_controller_class.

I'd submit, therefore, that this PR is fine as it is.

Thanks for the reviews!

tests/phpunit/tests/rest-api/rest-revisions-controller.php

src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php

ramonjd · 2023-12-19T20:38:56Z

Thanks a lot for the feedback @spacedmonkey! 🙇🏻 I'll get it done today or tomorrow.

Adding parent id revision parent_id mismatch invalid test to the templates revisions controller tests Remove specific mention of "post" to discern between other post types, e.g., "template"

github-actions · 2023-12-20T20:15:41Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

…nt ID is returned in the error message.

…, e.g., "template"

adamsilverstein

Excellent, thanks for all the tests!

tellthemachines · 2023-12-22T02:11:07Z

Committed in r57222.

… individual items.

…ss#5655

This is required to sync with WordPress/wordpress-develop#5655

…59049) This is required to sync with WordPress/wordpress-develop#5655 Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org>

… individual items.

ramonjd commented Nov 13, 2023

View reviewed changes

tests/phpunit/tests/rest-api/rest-revisions-controller.php Outdated Show resolved Hide resolved

tests/phpunit/tests/rest-api/rest-revisions-controller.php Show resolved Hide resolved

andrewserong reviewed Nov 13, 2023

View reviewed changes

src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php Outdated Show resolved Hide resolved

danielbachhuber requested changes Nov 13, 2023

View reviewed changes

ramonjd added 2 commits November 14, 2023 10:11

Adding some tests to check whether get_item can return a revision who…

f6f8e3f

…se parent does not match the `parent` route fragment.

Add a condition to get_item that checks the revision parent id agains…

6fdc982

…t the supplied parent id. If it doesn't match, return a 404 Updated test coverage Update error message in class-wp-rest-revisions-controller.php

ramonjd force-pushed the try/revisions-controller-get-item-valid-parent branch from 9f528bf to 6fdc982 Compare November 13, 2023 23:11

ramonjd requested a review from danielbachhuber November 13, 2023 23:11

ramonjd added 2 commits November 14, 2023 10:12

Coerce types

83ddc64

Remove unused var in test

f8bf0da

danielbachhuber approved these changes Nov 19, 2023

View reviewed changes

src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php Outdated Show resolved Hide resolved

src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php Outdated Show resolved Hide resolved

Apply suggestions from code review

6d489eb

string replacement type signifier %d Co-authored-by: Daniel Bachhuber <daniel@bachhuber.co>

adamsilverstein approved these changes Nov 20, 2023

View reviewed changes

ramonjd mentioned this pull request Nov 23, 2023

Global styles controllers: extend with WP_REST_Posts_Controller and WP_REST_Revisions_Controller #5699

Closed

This comment was marked as outdated.

Sign in to view

spacedmonkey requested changes Dec 19, 2023

View reviewed changes

tests/phpunit/tests/rest-api/rest-revisions-controller.php Show resolved Hide resolved

src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php Outdated Show resolved Hide resolved

spacedmonkey approved these changes Dec 20, 2023

View reviewed changes

spacedmonkey requested review from jonnynews and spacedmonkey December 20, 2023 11:10

Changed duped error code

ff84ef6

Adding parent id revision parent_id mismatch invalid test to the templates revisions controller tests Remove specific mention of "post" to discern between other post types, e.g., "template"

ramonjd force-pushed the try/revisions-controller-get-item-valid-parent branch from ae49d86 to ff84ef6 Compare December 20, 2023 19:59

ramonjd added 2 commits December 21, 2023 14:18

Genericize error message and create a test to ensure the correct pare…

c5f48bd

…nt ID is returned in the error message.

Remove specific mention of "post" to discern between other post types…

139f6ca

…, e.g., "template"

adamsilverstein approved these changes Dec 21, 2023

View reviewed changes

tellthemachines closed this Dec 22, 2023

ramonjd added a commit to ramonjd/wordpress-develop that referenced this pull request Jan 31, 2024

Now that WordPress#5655 has landed, use inherited methods for getting…

f7cf55a

… individual items.

ramonjd added a commit to ramonjd/wordpress-develop that referenced this pull request Feb 7, 2024

Now that WordPress#5655 has landed, use inherited methods for getting…

b43344d

… individual items.

ramonjd added a commit to ramonjd/wordpress-develop that referenced this pull request Feb 14, 2024

Brings global styles controller in line with the changes from WordPre…

f87622d

…ss#5655

ramonjd mentioned this pull request Feb 14, 2024

Global styles revisions Controller: return single revision only when it matches the parent id #6108

Closed

ramonjd added a commit to WordPress/gutenberg that referenced this pull request Feb 15, 2024

Backports from Core, the changes in WordPress/wordpress-develop#6108

5bc68dc

This is required to sync with WordPress/wordpress-develop#5655

ramonjd mentioned this pull request Feb 15, 2024

Global styles revisions Controller: return single revision only when it matches the parent id WordPress/gutenberg#59049

Merged

ramonjd added a commit to ramonjd/wordpress-develop that referenced this pull request May 18, 2024

Now that WordPress#5655 has landed, use inherited methods for getting…

fd33d14

… individual items.

ramonjd added a commit to ramonjd/wordpress-develop that referenced this pull request May 28, 2024

Now that WordPress#5655 has landed, use inherited methods for getting…

e7d8ddd

… individual items.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revisions Controller: return single revision only when it matches the parent id #5655

Revisions Controller: return single revision only when it matches the parent id #5655

ramonjd commented Nov 10, 2023 •

edited

Loading

andrewserong left a comment

danielbachhuber left a comment

ramonjd commented Nov 19, 2023

adamsilverstein left a comment

jonnynews commented Nov 27, 2023

ramonjd commented Nov 27, 2023

ramonjd commented Nov 28, 2023 •

edited

Loading

ramonjd commented Nov 28, 2023 •

edited

Loading

This comment was marked as outdated.

ramonjd commented Dec 19, 2023

github-actions bot commented Dec 20, 2023

adamsilverstein left a comment

tellthemachines commented Dec 22, 2023

Revisions Controller: return single revision only when it matches the parent id #5655

Revisions Controller: return single revision only when it matches the parent id #5655

Conversation

ramonjd commented Nov 10, 2023 • edited Loading

andrewserong left a comment

Choose a reason for hiding this comment

danielbachhuber left a comment

Choose a reason for hiding this comment

ramonjd commented Nov 19, 2023

adamsilverstein left a comment

Choose a reason for hiding this comment

jonnynews commented Nov 27, 2023

ramonjd commented Nov 27, 2023

ramonjd commented Nov 28, 2023 • edited Loading

ramonjd commented Nov 28, 2023 • edited Loading

This comment was marked as outdated.

ramonjd commented Dec 19, 2023

github-actions bot commented Dec 20, 2023

Test using WordPress Playground

Some things to be aware of

adamsilverstein left a comment

Choose a reason for hiding this comment

tellthemachines commented Dec 22, 2023

ramonjd commented Nov 10, 2023 •

edited

Loading

ramonjd commented Nov 28, 2023 •

edited

Loading

ramonjd commented Nov 28, 2023 •

edited

Loading