WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#38490 closed task (blessed) (fixed)

Add settings to the `/wp/v2/settings` endpoint that are in the WordPress.com api

Reported by: joehoyle Owned by: joehoyle
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch reporter-feedback close
Focuses: Cc:

Description

See https://github.com/WP-API/WP-API/issues/2860#issuecomment-255910043 for background, we want to get feature parity with the WordPress.com API, for non-.com / jetpack things, this means adding a few of the other core options to our settings endpoint, via register_setting.

Attachments (4)

38490.diff (2.1 KB) - added by joehoyle 3 years ago.
38490.2.diff (1.9 KB) - added by rachelbaker 3 years ago.
Added full stops
38490.3.diff (4.4 KB) - added by jnylen0 3 years ago.
38490.4.diff (3.6 KB) - added by jnylen0 3 years ago.

Download all attachments as: .zip

Change History (28)

@joehoyle
3 years ago

This ticket was mentioned in Slack in #core by stevenkword. View the logs.


3 years ago

#2 @stevenkword
3 years ago

  • Owner set to rachelbaker
  • Status changed from new to assigned

#3 @rachelbaker
3 years ago

@joehoyle anything else to add here?

#4 @joehoyle
3 years ago

@rachelbaker no that's all of them actually, #reviewmerge, as it were :)

This ticket was mentioned in Slack in #core by helen. View the logs.


3 years ago

#6 @ocean90
3 years ago

38490.diff The descriptions should probably end with a full stop like the others.

#7 @rachelbaker
3 years ago

  • Keywords reporter-feedback added

@joehoyle It looks like there are still some settings missing when I compare our settings with https://github.com/Automattic/jetpack/blob/1801d53/json-endpoints.php#L2443-L2495

Isn't the idea to support the same core options as the Jetpack/.com API? After 38490.diff we would still be missing support for the following options:

                'default_pingback_flag'                
		'blog_public'                          
		'require_name_email'                   
		'comment_registration'                 
		'close_comments_for_old_posts'         
		'close_comments_days_old'              
		'thread_comments'                      
		'thread_comments_depth'                
		'page_comments'                        
		'comments_per_page'                    
		'default_comments_page'                
		'comment_order'                        
		'comments_notify'                      
		'moderation_notify'                    
		'comment_moderation'                   
		'comment_whitelist'                    
		'comment_max_links'                    
		'moderation_keys'                      
		'blacklist_keys'                       
		'gmt_offset'                           

This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.


3 years ago

@rachelbaker
3 years ago

Added full stops

#9 @rachelbaker
3 years ago

In 38971:

REST API: Add the default_comment_status and default_ping_status options to the setting endpoint.

Props joehoyle.
See #38490.

#10 @jnylen0
3 years ago

As discussed in Slack, the remaining settings are read-only via the WP.com settings API. We should look carefully before adding more - for example, gmt_offset is automatically calculated from timezone_offset (via `pre_option_gmt_offset` and `wp_timezone_override_offset`).

Should we add an easier way to include settings as read-only? If so, how can clients know whether or not they can write to a given setting?

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by helen. View the logs.


3 years ago

#13 @azaozz
3 years ago

  • Type changed from enhancement to task (blessed)

#14 @rachelbaker
3 years ago

  • Owner changed from rachelbaker to joehoyle

@joehoyle I am kicking this back to you to determine a path forward for resolving the readonly settings.

#15 @joehoyle
3 years ago

Should we add an easier way to include settings as read-only? If so, how can clients know whether or not they can write to a given setting?

I don't think so no - an unsettable setting is a misnomer IMO. It may have a place in the API, but if it's informational, and not settable it shouldn't be in the settings endpoint. We have a way to markup read-only via the schema easily, which is what we do for all other fields, I just don't think it makes sense for an endpoint is which specifically for changing things.

#16 @jnylen0
3 years ago

My use-case for read-only settings is WordPress.com where we need to prevent users from changing the url and email settings. It also doesn't seem right to remove them from the settings endpoint response.

Right now we're using filters to prevent updating these settings, so it just doesn't work and it's not really obvious why.

#17 @joehoyle
3 years ago

@jnylen0 to mark the settings as read-only you could hook into register_setting_args and set the schema's readonly property to true. This would be the same we we indicate all other read-only fields in the API.

To better support readonly functionality in the settings endpoint, we could allow:

<?php
register_setting( 'some_group', 'my_read_only_test', array(
        'show_in_rest' => array(
                'type'     => 'string',
                'schema'   => array(
                        'readonly' => true,
                ),
        ),
));

Right now, you can do that and read-only will show in the Schema, however the settings controller won't respect it it's self.

@jnylen0
3 years ago

#18 @jnylen0
3 years ago

Thanks for the tip about register_setting_args and readonly @joehoyle. Needs a patch in order to work correctly - in 38490.3.diff:

  • Show arguments list for GET /wp/v2/settings (they are already present for the update endpoint)
  • Allow passing readonly settings through the schema code (get_endpoint_args_for_item_schema( WP_REST_Server::READABLE ) and get_data_for_route); otherwise settings marked as read-only are excluded from the argument list. There are no other changes to the /wp/v2?context=help response as a result of this, because elsewhere, get_endpoint_args_for_item_schema is always called with a method other than READABLE.
  • Test case for the new behavior

In general your comment makes sense to me: "an unsettable setting is a misnomer IMO [and] it shouldn't be in the settings endpoint".

There's already a way to enforce read-only behavior (the rest_pre_update_setting filter introduced in https://github.com/WP-API/WP-API/pull/2864). Since this is such a special use case, I don't think we need to make the readonly flag control this.

#19 @joehoyle
3 years ago

otherwise settings marked as read-only are excluded from the argument list.

This is intentional I think, as if it's it's read-only it shouldn't have an argument. However, that's different to the schema which you can get at OPTIONS /wp/v2/settings/schema IIRC you _will_ see the read-only setting in there. (you can also see the Schema at /wp/v2?context=help)

Last edited 3 years ago by joehoyle (previous) (diff)

@jnylen0
3 years ago

#20 @jnylen0
3 years ago

if it's it's read-only it shouldn't have an argument

These special settings should still show up in the arguments list for GET, and right now in trunk they don't. 38490.4.diff is a more minimal change that does not add the readonly property to the arguments list.

I guess this is fine, since settings marked as readonly show up in arguments for GET but not POST/PUT/PATCH.

#21 @joehoyle
3 years ago

These special settings should still show up in the arguments list for GET

Hmm so I still think no, the GET arguments are things you would pass like ?per_page=10, no such arguments exist at all for settings. POST / PUT arguments would exist for normal settings, because those are things you can specify on POST etc.

Read-only things, by definition, are not POST / PUT because you _can't_ send them back to be updated. Arguments = what you can update, or query by, schema = description of the data.

This ticket was mentioned in Slack in #core-restapi by joehoyle. View the logs.


3 years ago

#23 @jnylen0
3 years ago

  • Keywords close added

OK, that makes more sense. I agree we shouldn't change the GET args, I was getting confused on that.

I agree there's nothing left to do here. We can use existing filters and features to do what we need to do for our unique use case.

#24 @joehoyle
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

Sounds good!

Note: See TracTickets for help on using tickets.