-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Allow settings to be updated and read. #2985
base: trunk
Are you sure you want to change the base?
Conversation
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.
Please find my review notes below. Thank you.
wp_set_current_user( self::$administrator ); | ||
$user = get_user_by( 'id', self::$administrator ); | ||
$user->add_cap( 'custom_rest_cap' ); |
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.
In my testing, this test didn't work because the user's capabilities were cached. Therefore, I had to reorder it like this to make the test work:
wp_set_current_user( self::$administrator ); | |
$user = get_user_by( 'id', self::$administrator ); | |
$user->add_cap( 'custom_rest_cap' ); | |
$user = get_user_by( 'id', self::$administrator ); | |
$user->add_cap( 'custom_rest_cap' ); | |
wp_set_current_user( self::$administrator ); |
wp_set_current_user( self::$administrator ); | ||
$user = get_user_by( 'id', self::$administrator ); | ||
$user->add_cap( 'custom_rest_cap' ); |
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.
Same.
wp_set_current_user( self::$administrator ); | |
$user = get_user_by( 'id', self::$administrator ); | |
$user->add_cap( 'custom_rest_cap' ); | |
$user = get_user_by( 'id', self::$administrator ); | |
$user->add_cap( 'custom_rest_cap' ); | |
wp_set_current_user( self::$administrator ); |
'status' => rest_authorization_required_code(), | ||
) | ||
); | ||
} |
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.
The WP_Test_REST_Settings_Controller::test_update_item_with_edit_permission_callbacks()
test method implies that the setting can be updated even if the current user lacks the manage_options
capability. However, the test fails because the code still checks for that capability.
I'm not sure what the intention is here: should a user be able to update a setting even without the manage_options
capability, as long as the permission callback allows it?
} | |
} | |
return true; |
* @since 6.1.0 | ||
* | ||
* @param WP_REST_Request $request Full details about the request. | ||
* @return true|WP_Error True if the request has access to update the item, WP_Error object otherwise. |
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.
Nitpick:
* @return true|WP_Error True if the request has access to update the item, WP_Error object otherwise. | |
* @return bool|WP_Error True if the request has access to update the item, WP_Error object otherwise. |
Working on #48885. Decided to go a different direction than the ticket name. Use the existing endpoint but add some new callbacks.
To use this PR, here is the example I use to test.
This make a setting called
wibble
, that will now be public to read and edit. You never do this, but it gives you an idea of how the code works.Trac ticket: https://core.trac.wordpress.org/ticket/48885
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.