-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
Add show show_in_rest param to register_sidebar #1578
Conversation
src/wp-includes/rest-api/endpoints/class-wp-rest-sidebars-controller.php
Outdated
Show resolved
Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-sidebars-controller.php
Show resolved
Hide resolved
Won't we need a change in the widgets controller to check if a widget is part of a public sidebar? |
@TimothyBJacobs Updated the PR with your feedback. One question. Should we / can we change raw instance values to only display in edit context. wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php Lines 783 to 787 in eb6b517
I am wonder about sensitive data leaking? |
Thanks. Looks like the test failure may be legit. We would need to change the whole I think sensitive data leakage is definitely something we should be concerned about. While a theme author may opt-in to having their sidebar available, they might not know that a widget's setting contains an API key for instance. However, I think there is definitely value in having the instance values available, not just the rendered widget, to allow for rendering the widget yourself. I think for now we could omit the instance entirely, but it may make sense in the future to allow for |
Seems like the issue here was running |
Pinging @adamziel as he's been looking into |
So should only wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php Lines 767 to 789 in 52fe9ff
|
@spacedmonkey I think the mental model of Since your issue seems to come from running Other than that, I don't think that we have a good way around multiple function calls other than what you proposed. Well, maybe aside of a hook that would run when a related REST route is matched – that way we could have only a single call that would cover both the permissions check and the actual handler. |
@spacedmonkey I wouldn't say it blocks, no. Maybe it would allow you to get rid of this check: if ( $this->widgets_retrieved ) {
return;
} But I'm not 100% certain about that since I don't have a good grasp of what the problem was.
I don't this PR would affect #1525 much – a rebase, if necessary, should be pretty easy. Feel free to move forward with this one once you have approvals.
I read the code and it looks good, but I did not test much. I'm happy to help with testing once you confirm the code is where you want it to be, Also, one more question – should this PR be moved to the Gutenberg repo so that 5.7+plugin may benefit from these changes? That's what we did with WordPress/gutenberg#34230, it may or may not make sense here as well |
Yeah, it can be backported. |
So we should wait until your work is merged before this goes in? |
If it takes a few days then it would be convenient – potentially one check less in your PR. If it takes more, it's probably not worth it – we could re-evaluate that check at some later time. Maybe just rebasing this PR on top of #1525 would do the trick? I think the consensus is there and we were just adjusting code standards. Also, if #1525 does not solve the "if retrieve_widgets already called" situation, feel free to just proceed without looking back. |
Why not Gutenberg-first? These "double" changes have been challenging lately, see the discussion in WordPress/gutenberg#33810 and WordPress/gutenberg#34536. The sidebars endpoint is getting removed from the next Gutenberg version, so no problem there. It's all about having a consistent widgets endpoint now. |
This change doesn't make sense to backport. It has little use in the gutenberg plugin and it an opt in feature for devs. It is most useful for headless applications of WordPress. |
I see, thank you for explaining! I am only concerned about merging changes later on. If we keep updating the same endpoints both in core and in the Gutenberg repo, we may end up with conflicting changes. On the other hand, I think I discussed that with @gziolo in WordPress/gutenberg#33810 and the conclusion was that in practice this rarely becomes a problem so maybe we're good here? |
If there was any value in adding this to the gutenberg plugin, I would have done it both places. I will do that future PRs. |
Unit tests are passing! |
src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php
Outdated
Show resolved
Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-sidebars-controller.php
Outdated
Show resolved
Hide resolved
@@ -240,6 +240,46 @@ public function test_get_items_no_permission() { | |||
$this->assertErrorResponse( 'rest_cannot_manage_widgets', $response, 401 ); | |||
} | |||
|
|||
public function test_get_items_no_permission_public() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused because the test name says "no permission public", but then it sets up a sidebar with two widgets and the endpoint returns both. I'd add a few test cases where the API returns a limited list of widgets or a 403 error.
/** | ||
* @ticket 41683 | ||
*/ | ||
public function test_get_items_no_permission_public() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few additional test cases would go a long way, see my other comment
* @param array $sidebar Sidebar. | ||
* @return bool Whether the side can be read. | ||
*/ | ||
public function check_read_permission( $sidebar ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this below?
public function check_read_permission( $sidebar ) { | |
public function can_show_sidebar_in_rest( $sidebar ) { |
check_read_permission
is called quite similarly to get_item_permissions_check
and do_permissions_check
, I got a bit confused after returning to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This naming follows existing core methods. See
wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-blocks-controller.php
Lines 30 to 37 in 2382765
public function check_read_permission( $post ) { | |
// By default the read_post capability is mapped to edit_posts. | |
if ( ! current_user_can( 'read_post', $post->ID ) ) { | |
return false; | |
} | |
return parent::check_read_permission( $post ); | |
} |
foreach ( wp_get_sidebars_widgets() as $id => $widgets ) { | ||
$sidebar = $this->get_sidebar( $id ); | ||
|
||
if ( ! $sidebar ) { | ||
continue; | ||
} | ||
|
||
if ( is_wp_error( $permissions_check ) && ! $this->check_read_permission( $sidebar ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
show_in_rest
is only checked for users without edit_theme_options
capability. Is this intended? Intuitively, I would expect that setting an option named show_in_rest
to false
will remove my sidebar from any and all REST API responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is namely wrong. show_in_rest
only means show publically in rest. It will always be in the rest api.
If we remove this, then it the widget editor will not work.
|
||
foreach ( wp_get_sidebars_widgets() as $sidebar_id => $widget_ids ) { | ||
if ( isset( $request['sidebar'] ) && $sidebar_id !== $request['sidebar'] ) { | ||
continue; | ||
} | ||
|
||
if ( is_wp_error( $permissions_check ) && ! $this->check_read_sidebar_permission( $sidebar_id ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same question as above – is it intended that show_in_rest is not effective for users with the right permissions?
|
||
$prepared = array(); | ||
$prepared = array(); | ||
$permissions_check = $this->permissions_check( $request ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$permissions_check = $this->permissions_check( $request ); | |
$user_has_permissions = $this->permissions_check( $request ); |
PHPUnit failures seem to be just flaky CI. I think this PR is pretty close 👍 I left some last notes and proposed a few tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Trac ticket: https://core.trac.wordpress.org/ticket/53915
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.