Opened 4 years ago
Last modified 4 months ago
#52925 new defect (bug)
Autosaves controller: Post checks will never catch invalid IDs
Reported by: | coreymckrill | Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 5.8 |
Component: | REST API | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
The create_item
and create_post_autosave
methods both try to check if the id parameter in a request is for a valid post, by calling the get_post function. The problem is that both methods expect that if it's not a valid post, it will return a WP_Error object, when in fact get_post only returns null on failure.
The Posts controller has a protected get_post method that will generate an appropriate WP_Error for this case, but neither the Autosaves, nor its parent Revisions controller has a similar method. Copying that method to the Revisions controller, and then using it in the create_*
methods seems like the best approach here.
Change History (9)
#1
@
4 years ago
- Keywords needs-patch needs-unit-tests added
- Milestone changed from Awaiting Review to 5.8
This ticket was mentioned in PR #1145 on WordPress/wordpress-develop by donmhico.
4 years ago
#3
This PR copies the implementation of get_post()
from WP_REST_Posts_Controller
to WP_REST_Autosaves_Controller
so create_item()
and create_post_autosave()
would return the correct WP_Error
on get_post()
failure.
I'll try to add a unit test when possible.
Trac ticket: https://core.trac.wordpress.org/ticket/52925
#4
@
3 years ago
Hi @donmhico , I have some suggestions for unit tests. I made a pull request into your branch here https://github.com/donmhico/wordpress-develop/pull/1.
#5
@
3 years ago
- Keywords needs-patch added; has-patch removed
- Milestone changed from 5.8 to Future Release
I like the changes from @hermpheus. Can we get a unified PR created?
This ticket was mentioned in Slack in #core by whyisjake. View the logs.
3 years ago
#7
@
4 months ago
I am willing to create a unified PR to help move this ticket forward.
@hermpheus and @donmhico would either of you prefer to take this on?
#8
@
4 months ago
I'm working on an unified PR for this ticket.
Update: The PR I've submitted is based on the two PRs above, but it updates the code according to the latest changes in the Autosaves controller. Additionally, it contains an updated version of the unit tests that ensures the actual changes being made are covered.
This ticket was mentioned in PR #7076 on WordPress/wordpress-develop by @antonvlasenko.
4 months ago
#9
- Keywords has-patch has-unit-tests added; needs-unit-tests needs-patch removed
Trac ticket: https://core.trac.wordpress.org/ticket/52925
Created a PR here - https://github.com/WordPress/wordpress-develop/pull/1145 - i'm not sure why the PR is not attached in this ticket. PR still needs unit test though.