Fix Nav block invalid permissions warning by avoiding using trashed wp_navigation posts. #42940
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Overides the get_post to allow for only published or draft posts
getdave
changed the title
getdave
added
[Type] Bug
getdave
changed the title
|
It would be great to get some experienced REST API folks on this. Perhaps there's a better way to resolve this issue? |
getdave
commented
Aug 3, 2022
| * @subpackage REST_API | ||
| */ | ||
|
|
||
| class Gutenberg_REST_Navigation_Controller extends WP_REST_Posts_Controller { |
There was a problem hiding this comment.
Note we will need to introduce this class anyway (see #42809)
|
Size Change: +18 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
scruffian
changed the title
|
I don't like this. I think the change should handle in Javascript and not php. |
|
@spacedmonkey Would you please elaborate? Why do you think this should be handled in Javascript? More importantly, how do you envision the implementation? |
|
@adamziel I thought a little more about @spacedmonkey's prompt. I'd definitely like to know more about the objections but I can see some arguments for keeping things simple and doing it on the JS side. Firstly, it seems we were already trying to treat non published Nav Posts as "missing". However we didn't fill in the final piece of the puzzle which was to set the #42982 implements a JS-only solution which seems to have the same effect as this PR. Open to feedback on which route is preferable. |
|
The rest api is not just for gutenberg. It is used by other clients. Changing the functionality of the server to fix a client problem is backwards. Consider that other clients are using the rest api and this "fix" might break their client implementation. Consider future implementation of this functionality. |
Indeed. And I'm going to keep this in mind going forward. In my head I was thinking "Oh, I'm introducing a new REST API class" so I'm good for backwards compatibility". But of course the endpoint has always been there - it's just been using the Posts Controller. So changing it like this could cause backwards compatibility problems for folks already consuming the endpoint. Thanks for taking the time to provide more clarity on your comments. It helps improve the conversation and allows others to learn. I appreciate you keeping folks diligent about how they interact with the REST API and consider backwards compatibility. As you can see I now have a JS only solution. |
9 tasks
|
Closing in favour of #42982. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please note there is now a JS alternative to this PR in #42982.
What?
This PR fixes the Nav block to avoid it showing a permissions warning for posts which don't exist.
First reported in #42735 (review).
Here's a video of the bug:
182622513-7269fb77-7389-44f5-9aa0-3317304824b8.mp4
Why?
Currently the Navigation block has a
refwhich points at a Post ID of typewp_navigation. If this post is deleted and moved to Trash then technically it still exists but it has apost_statusoftrashed.We look up Navigation posts using
getEntityRecordwhich ends up in theget_item()functino of theWP_REST_Posts_Controllerclass.This method calls the
get_postmethod of the class and simply looks up by postId. This means we cannot filter the result to return only published posts.How?
This PR creates a custom
Gutenberg_REST_Navigation_Controllerwhich overloads theget_postmethod to throw an error if the post found by the parent (i.e.WP_REST_Posts_Controller::get_post()) method has a status which is not either:This ultimately means that trashed posts will no longer be found and returned by API calls even if they exist. This means that
getEntityRecordwill not find a post with a given ID if it has been trashed.We then modify the
editcode of the Nav block to first check if the Nav post exists before displaying permission based messages.Testing Instructions
Navigation menu has been deleted or is unavailable.Screenshots or screencast
Screen.Capture.on.2022-08-03.at.16-12-44.mp4