WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 12 months ago

Last modified 12 months ago

#37885 closed enhancement (fixed)

Build out register_setting like register_meta

Reported by: joehoyle Owned by: rmccue
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Options, Meta APIs Keywords: needs-dev-note has-unit-tests
Focuses: Cc:

Description

In 4.6 the diligent work was done to beef out register_meta() to be much more extensible. For use in the REST API, I'd like have a similar API for registered settings.

Register settings and meta is quite similar (settings are after all just meta on the site object ;) ) so I think we can take a lot of design queues from register_meta and bring them into register_setting. However, as settings are not shared across any "object type", the way they are stored in a global can probably be a bit simpler.

Much like register_meta I propose we change the function signature to register_setting( $option_group, $option_name, $args = array() ).

I've attached an initial patch of the kind of thing I'm thinking.

Attachments (4)

37885.diff (4.3 KB) - added by joehoyle 13 months ago.
37885.2.diff (4.6 KB) - added by aaroncampbell 12 months ago.
37885.3.diff (5.5 KB) - added by rmccue 12 months ago.
Updated patch with tweaks
37885-tests.diff (1.8 KB) - added by rmccue 12 months ago.
Add tests for register_setting changes

Download all attachments as: .zip

Change History (37)

@joehoyle
13 months ago

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


13 months ago

#2 in reply to: ↑ description ; follow-up: @danielbachhuber
13 months ago

Replying to joehoyle:

However, as settings are not shared across any "object type", the way they are stored in a global can probably be a bit simpler.

Should we introduce the concept of a site object, such that settings are simply attributes on a site?

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


13 months ago

#4 follow-up: @sc0ttkclark
13 months ago

I wonder if options aren't just meta for a site, and why we don't use register_meta() here all the way and maybe just add register_setting() just wraps register_meta() and provides a quick way to register settings?

#5 @sc0ttkclark
13 months ago

Where register_meta() is going, where things like the Fields API is going, should all be to enable a solid API that all of WP can benefit from. I'd really like to see us make use of that foundation to improve things like Settings for the better, versus create new functions with their own logic and globally stored configurations.

#6 in reply to: ↑ 4 @flixos90
13 months ago

Replying to sc0ttkclark:

I wonder if options aren't just meta for a site, and why we don't use register_meta() here all the way and maybe just add register_setting() just wraps register_meta() and provides a quick way to register settings?

+1 for that. Just keep in mind that the object type there should probably be called blog because sitemeta already exists, unfortunately to denote network options. So we would wanna prevent future collisions.

#7 in reply to: ↑ 2 @joehoyle
13 months ago

Replying to danielbachhuber:

Should we introduce the concept of a site object, such that settings are simply attributes on a site?

I don't think this is the right place to do that - however this is only a still a single object type, register_meta takes object_type because it's used for posts, comments, users etc - we don't have that concept with settings.

I wonder if options aren't just meta for a site, and why we don't use register_meta() here all the way and maybe just add register_setting() just wraps register_meta() and provides a quick way to register settings?

I may have oversimplified with "settings is just meta", because it's _not_ in WordPress land - the Meta API is a totally different implementation to Options - conceptually they are the same, in practice with the WP codebase being what it is, they are different so I don't think it makes sense to actually use register_meta, I just think there's a lot of overlap so we can look to register_meta for pointers here.

Replying to sc0ttkclark:

Where register_meta() is going, where things like the Fields API is going, should all be to enable a solid API that all of WP can benefit from. I'd really like to see us make use of that foundation to improve things like Settings for the better, versus create new functions with their own logic and globally stored configurations.

I'm not sure if this is against this patch or not - the only new function here is get_registered_settings which is a helper for the global var. All this patch aims to do is add an args option to the function signature so it _can_ be used more extensively down the road. Also, virtually all configuration in WordPress is stored globally!

There's a lot to be done on solving the Meta / Settings APIs, how the Fields API fits into that, and how we can have a solid foundation for the Fields API to leverage - this is one small step towards that I think :)

#8 follow-up: @sc0ttkclark
13 months ago

There's much more in common between Settings and Meta than different. Both use key/value storage, with support for serialization.

In the register_setting() wrapper, we could implement Settings-specific things necessary to pass into register_meta() as arguments.

In fact, in the Fields API, we treat them exactly the same between all other content types in WordPress with absolutely no problems. In the future, I can totally see that the Settings API itself will become powered by the Fields API.

There are also multiple uses here for settings-related object types such as Settings (option) and Network Settings (site_option). Both require the same type of interfaces but different *storage*. They would both benefit having a uniform API, and with register_meta() at the heart of it.

I don't think we're disagreeing here, we both know what Settings needs. I'd just like to see what prevents us from utilizing the register_meta() store for this information and wrapping it with register_setting() instead of creating the new global variable for storage of all settings.

#9 in reply to: ↑ 8 @joehoyle
13 months ago

Replying to sc0ttkclark:

There's much more in common between Settings and Meta than different. Both use key/value storage, with support for serialization.

Ok, so I think this is where we are disagreeing (which I think is a good thing we've maybe nailed that down!). The reason I don't see the implementations being similar enough to literally share the code: options have: all-options, autoloading options, don't have object ids and don't follow the same schema as the other meta tables.

That's why I think wrapping register_meta is a mistake to appease the "don't write a single line of code twice" idea. register_meta is still a very simple function, I think what we'll gain by trying to fit both the options and object-meta implementations into the same function we'll lose with a leaky abstraction and multiple exceptions for options handling. get_registered_meta will now also return registered settings, I don't know any developer that would _expect_ that based off the naming.

To me, it makes no difference, and is no worse if we have:

global $wp_registered_settings = array();
global $wp_registered_meta = array( 'post' => arrray() );

or

global $wp_registered_meta = array( 'settings => array(), 'post' => array() );

Global state is not a great thing, however one global variable with x and y, or two global variables is arbitrary, afterall it's just an array in $GLOBALS :D

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


13 months ago

#11 @rmccue
13 months ago

  • Keywords commit added; dev-feedback removed

If we want to someday unify the meta and settings APIs, sure, but that's not what this ticket is about. Similar to the meta ticket, I think this is a bit off-track, and I worry we're not going to make forward movement if we keep discussing that.

This ticket is about changing the register_setting function to take extra parameters to make it more extensible. Any other goals should be new tickets.

---

Patch looks good; the change in unregister_setting should add braces on to the $sanitize_callback != '' conditional, since we're in the region. Otherwise, :thumbsup:.

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


13 months ago

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


13 months ago

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


13 months ago

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


12 months ago

#16 @aaroncampbell
12 months ago

37885.2.diff is based on 37885.diff with the following changes:

  • In register_setting() use $args['sanitize_callback'] instead of $sanitize_callback which no longer exists.
  • Added braces to two conditionals in unregister_setting() just because we're in there and that's the standard.
  • Docs on get_registered_settings() to reflect settings and not meta.

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


12 months ago

@rmccue
12 months ago

Updated patch with tweaks

#18 @rmccue
12 months ago

Uploaded an updated patch with a few final tweaks:

  • $sanitize_callback in unregister_setting() is now taken from the args used during registration.
  • Removed the return for register_setting(), which doesn't actually return anything.
  • Tweaked the phpDoc for both functions to match style, and add missing descriptions.
  • Changed the $sanitize_callback check to use ! empty, since our default is now null rather than ''

#19 @joehoyle
12 months ago

My name is Joe Hoyle, and I endorse this patch.

#20 @boonebgorges
12 months ago

My name is not Joe Hoyle, but I endorse the patch anyway.

#21 @joehoyle
12 months ago

  • Owner set to joehoyle
  • Resolution set to fixed
  • Status changed from new to closed

In 38635:

Options: Build out register_setting like register_meta.

register_setting can now be passed an array arguments to specify meta-data about the setting,
much like using the register_meta API. Of note, it will now accept a show_in_rest arg to
hint the inclusion of the setting in the REST API. get_registered_settings() is available
as a utility to get all registered settings.

Props rmccue, aaroncampbell.
Fixes #37885.

#22 @jorbin
12 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

i can haz unit tests plz?

#23 @jorbin
12 months ago

  • Keywords needs-dev-post needs-unit-tests added; has-patch commit removed

register_setting could use a changlelog @since

This also should get some love on make.wordpress.org/core

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


12 months ago

#25 @swissspidy
12 months ago

  • Keywords needs-dev-note added; needs-dev-post removed

#26 @stephenharris
12 months ago

Currently the third option for register_setting() is a callback, and for reasons for backwards compatibility that callback cannot be hooked onto sanitize_option_{$option_name} with the three arguments that are available (see #15335) - however, if we now support an $args array with sanitize_callback key then there are no backward-compatibility concerns, so we could pass all three arguments to that callback.

Admittedly it involves another if statement, and another degree of complexity - but what do people think about passing all three arguments when the callback is specified in the $args array?

#27 @rmccue
12 months ago

In 38676:

Options: Add missing since for register_setting argument.

[38635] added support for extra arguments on registration, but was not documented.

See #37885.

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


12 months ago

#29 @rmccue
12 months ago

  • Owner changed from joehoyle to rmccue
  • Status changed from reopened to assigned

Still todo: those unit tests.

As part of that, we're going to move register_setting to wp-includes/option.php instead, since with this change it becomes a much more generic API. This also facilitates potential future changes in #38176.

#30 @rmccue
12 months ago

In 38687:

Options: Move register_setting() from wp-admin to wp-includes.

With [38635], register_setting is now a more generic setting registration function and is usable outside of the admin.

See #37885.

@rmccue
12 months ago

Add tests for register_setting changes

#31 @rmccue
12 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Added patch with the tests.

#32 @joehoyle
12 months ago

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

In 38690:

Options: Add Unit tests for register_setting.

Test register_setting with old and new style of arguments.

Props rmccue.
Fixes #37885.

#33 @rmccue
12 months ago

(By the way, @joehoyle and I did Scissors-Paper-Rock on this, with the result that Joe is responsible for the dev note.)

Note: See TracTickets for help on using tickets.