Skip to content
This repository has been archived by the owner. It is now read-only.

Experiment with registered settings #13

Merged
merged 16 commits into from Sep 22, 2016
Merged

Experiment with registered settings #13

merged 16 commits into from Sep 22, 2016

Conversation

@joehoyle
Copy link
Collaborator

joehoyle commented Aug 30, 2016

In https://core.trac.wordpress.org/ticket/37885 I have added a patch to
make the registered settings be imbued with more data for inclusion in
the REST API. This is a work in progress to see how the controller might
look for that.

joehoyle added 4 commits Aug 30, 2016
In https://core.trac.wordpress.org/ticket/37885 I have added a patch to
make the registered settings be imbued with more data for inclusion in
the REST API. This is a work in progress to see how the controller might
look for that.
this is until core implements is, hopefully!
@joehoyle

This comment has been minimized.

Copy link
Collaborator Author

joehoyle commented Sep 14, 2016

Another round of changes:

  1. Renamed to WP_REST_Settings_Controller along with the route moving to /settings per #14.
  2. We now make use of register_setting( ..., ..., array( 'show_in_rest' => array( ...rest options ) ) ) to pass options for the setting in the rest api.
  3. We merge in values from the register_setting like description and type into the show_in_rest args.
  4. Registering a setting in the API now looks like this:
register_setting( 'general', 'blogname', array(
    'show_in_rest'   => array(
        'name'            => 'title',
    ),
    'type'           => 'string',
    'description'    => __( 'Site title.' ),
) );

name is the property name of the setting in the rest api.

The response so far looks like this:

{
    "date_format": "F j, Y",
    "default_category": 1,
    "default_post_format": "0",
    "description": "Just another Fake Site site",
    "email": "joe@hmn.md",
    "language": "en_US",
    "posts_per_page": 10,
    "start_of_week": 1,
    "time_format": "g:i a",
    "timezone": "",
    "title": "",
    "url": "http://wordpress.dev",
    "use_smilies": true
}

Updating a setting looks like this:

http POST http://wordpress.dev/wp-json/wp/v2/settings title="This is the new title"

Right now I'm registering core settings in this plugin, but ideally this would be moved to core. This all depends on buy-in from https://core.trac.wordpress.org/ticket/37885 of course.

@joehoyle

This comment has been minimized.

Copy link
Collaborator Author

joehoyle commented Sep 14, 2016

Deleting Options

There's no way to "DELETE" an option from a REST API perspective, as an setting is part of the item schema and fixed, you can however "null" out a setting value which will essentially reset it back to the default. To do that, the client just has to set a setting to null.

@joehoyle

This comment has been minimized.

Copy link
Collaborator Author

joehoyle commented Sep 14, 2016

Aside: the WordPress.com mobile app makes use of Site Title, Description and Site URL.

Copy link
Member

rmccue left a comment

Seems good to me, apart from some stylistic points here.

class WP_REST_Settings_Controller extends WP_REST_Controller {
public function __construct() {
$this->namespace = 'wp/v2';
$this->rest_base = 'settings';

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 14, 2016

Member

These should be moved to protected $namespace = '...' instead.

'methods' => WP_REST_Server::READABLE,
'callback' => array( $this, 'get_item' ),
'args' => array(),
'permission_callback' => array( $this, 'get_item_permissions_check' ),

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 14, 2016

Member

Alignment is off

'methods' => WP_REST_Server::EDITABLE,
'callback' => array( $this, 'update_item' ),
'args' => $this->get_endpoint_args_for_item_schema( WP_REST_Server::EDITABLE ),
'permission_callback' => array( $this, 'get_item_permissions_check' ),

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 14, 2016

Member

Alignment

) );
}
public function get_item_permissions_check( $request ) {

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 14, 2016

Member

Needs phpDoc

$response = array();
foreach ( $options as $name => $args ) {
// default to a null value as "null" in the response means "not set"

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 14, 2016

Member

Comments need to start with a capital and end with a period.

plugin.php Outdated
$site_route->register_routes();
}
function rest_register_settings() {

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 14, 2016

Member

Needs phpDoc

plugin.php Outdated
}
function rest_register_settings() {
require_once ABSPATH . 'wp-admin/includes/plugin.php';

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 14, 2016

Member

💀 💯

plugin.php Outdated
),
'type' => 'string',
'description' => __( 'Site title.' ),
) );

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 14, 2016

Member

The space here isn't necessary.

plugin.php Outdated
register_setting( 'general', 'blogname', array(
'show_in_rest' => array(
'name' => 'title',

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 14, 2016

Member

Alignment is off here (and below)

plugin.php Outdated
'name' => 'language',
),
'type' => 'string',
'description' => __( 'IETF "like" WordPress locale code.' ),

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 14, 2016

Member

IETF-like should be hyphenated. I'd probably change this to WordPress locale code., no need to mention the spec, since it has to be from the defined list of WP-valid locales anyway.

@kadamwhite kadamwhite mentioned this pull request Sep 15, 2016
}
$rest_args = array_merge( $rest_args, array(
'option_name' => $name,

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 19, 2016

Member

This shouldn't be overridable, since the data is contained in the registration for the option itself.

@joehoyle

This comment has been minimized.

Copy link
Collaborator Author

joehoyle commented Sep 19, 2016

Thanks for the review @rmccue, I'll wait on the outcome of register_setting on trac today before fixing this up as that might come with some extra work.

@joehoyle

This comment has been minimized.

Copy link
Collaborator Author

joehoyle commented Sep 22, 2016

@rmccue ok patched up those and also added unit tests here.

$params = $request->get_params();
foreach ( $options as $name => $args ) {
if ( array_key_exists( $name, $params ) ) {

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 22, 2016

Member

This should be flipped.

This comment has been minimized.

Copy link
@joehoyle

joehoyle Sep 22, 2016

Author Collaborator

Fixed in dcacb38

* @return WP_Error|array
*/
public function update_item( $request ) {
$options = $this->get_registered_options();

This comment has been minimized.

Copy link
@rmccue

rmccue Sep 22, 2016

Member

Spacing is off.

This comment has been minimized.

Copy link
@joehoyle

joehoyle Sep 22, 2016

Author Collaborator

Fixed in 5008108

joehoyle added 2 commits Sep 22, 2016
@joehoyle

This comment has been minimized.

Copy link
Collaborator Author

joehoyle commented Sep 22, 2016

Think we are looking good here @rmccue

* @param string $type Type that the data should be converted to.
* @return mixed
*/
protected function cast_value_to_type( $value, $type ) {

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Sep 22, 2016

Member

Would this be a useful utility function somewhere else?

register_setting( 'general', 'WPLANG', array(
'show_in_rest' => array(
'name' => 'language',

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Sep 22, 2016

Member

Is a locale always a language?

@danielbachhuber

This comment has been minimized.

Copy link
Member

danielbachhuber commented Sep 22, 2016

This reads pretty well 👍 Left a couple small comments.

@rmccue
rmccue approved these changes Sep 22, 2016
@rmccue rmccue merged commit 18c14e4 into master Sep 22, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rmccue rmccue deleted the register-setting-experiment branch Sep 22, 2016
@rmccue

This comment has been minimized.

Copy link
Member

rmccue commented Sep 22, 2016

Would this be a useful utility function somewhere else?

Let's split this into a new ticket.

joehoyle added a commit to WP-API/WP-API that referenced this pull request Sep 28, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.