Repository navigation
Abilities API: Add a core/settings-get ability - #12141
jorgefilipecosta wants to merge 62 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
@jorgefilipecosta can you link this to the trac ticket please? I don't have edit perms in this repo. |
073df67 to
c948c7a
Compare
Nice catch the ticket mention was added. |
c948c7a to
a28c976
Compare
|
Let's run development, review, and testing through WordPress/ai#691, then sync all agreed refinements here. |
24bee86 to
6419a95
Compare
gziolo
left a comment
There was a problem hiding this comment.
I left my feedback.
Two questions regarding core/get-site-info:
- It covers some similar settings but is scoped to the site. Should it also respect the
show_in_abilitiescheck wherever applicable? - Should it get the default value in the schema aligned to
(object) array()as here?
| */ | ||
| private function register_get_settings(): void { | ||
| // Compute once; execute_get_settings() reuses this exact structure. | ||
| $this->exposed_settings = $this->get_exposed_settings(); |
There was a problem hiding this comment.
I noticed that Core settings get registered on the rest_api_init hook:
add_action( 'rest_api_init', 'register_initial_settings', 10 );It's worth double-checking whether there won't be a race condition with wp_abilities_api_init.
The long-term question is whether get_exposed_settings() could be computed lazily inside the execute/schema callbacks instead of being cached at registration? That would remove the ordering coupling and match the sibling's live‑compute model.
There was a problem hiding this comment.
Good catch on the ordering. wp_abilities_api_init fires lazily and isn't ordered relative to rest_api_init, so register() now calls register_initial_settings() before computing the snapshot whenever rest_api_init hasn't fired (or is mid-fire before priority 10). Added the guard in ae862f0 and documented the timing in 59e1819; re-registering later on rest_api_init is harmless.
On computing it lazily: I kept the snapshot cached at registration on purpose. The input schema, output schema, and execute callback all have to derive from the exact same set of exposed settings, so computing it once avoids walking get_registered_settings() three times per request and any risk of the schema and the output drifting apart. With the ordering handled explicitly there's no coupling left to remove, so I dropped the unreachable recompute branch in 13f4d31.
There was a problem hiding this comment.
Following your later suggestion, core settings are now registered on wp_abilities_api_init, so the ability does not handle this anymore 👍
8469c64 to
6f4b639
Compare
Hi @gziolo I think the answer is yes to both, but I would prefer to do that in a separate PR to avoid this one being too huge. |
|
All the reviews comments were applied this is ready for another look. |
One bad setting value fails the whole
|
Consider wiring
|
|
Thanks @jorgefilipecosta! Nice work on this. The port is faithful to the plugin, the The remaining points are the ones I raised above. The main one is the bad-value handling, since it needs an actual fix here and in the plugin so they stay in sync. The rest are shared conventions and small polish: the |
The test filters by `title`, the name the setting is exposed under, but its comment still said `blogname`. The AI plugin's copy already says `title`.
| 'description' => '', | ||
| 'sanitize_callback' => null, | ||
| 'show_in_rest' => false, | ||
| 'show_in_abilities' => false, |
There was a problem hiding this comment.
Would it make sense to have this default to whatever show_in_rest is set to so that people don't? If someone has already chosen to make a setting available programmatically, showing it in abilities feels like a safe next step. They could still opt out, but this way extenders don't need to go through and update all of their code for it to work with the next generation feature.
This is similar to how register_post_type, publicly_queryable defaults to what public is set as, but can be overridden.
There was a problem hiding this comment.
Thank you for sharing you insights @aaronjorbin, it is something we considered in past but unfortunately it was not something we could do. Because defaulting to show_in_rest may result in exposing information that should be private in production sites.
Imagining the following scenario, a website adds an option with show_in_rest true that is private key, and then uses REST filters to make sure the key is never exposed e.g: the endpoints always return an empty string. If we default to show_in_rest we would expose the information that should be private on abilities. Given that rest is one of the abilities transform channels right after the upgrade users of a site would be able to retrieve something that is private.
The same happens to post_types meta etc... we can never default to what is set on rest.
There was a problem hiding this comment.
I elaborated on it today based on my findings from the last few days of exploration: WordPress/ai#40 (comment). @peterwilsoncc shared the same idea in a different PR, and I was initially very much in favor of defaulting to show_in_rest, as this would simplify adoption.
There was a problem hiding this comment.
The custom callback is a scenario that makes sense, thanks. And thank you @gziolo for the write up.
It doesn't need to be a blocker here, but I wish we could come up with a way to reduce the burden of maintainership on extenders as a way to also improve. Maybe a show_programmaticlly option? While I think REST and Abilties should be enough, I also would have said REST was enough not long ago.
There was a problem hiding this comment.
Maybe a show_programmaticlly option?
I raised a similar idea on yesterday's call, but suggested calling it public , in line with what Post Types use to inherit default behavior when a specific arg isn't set, and the Abilities API itself which recently introduced public to handle this exact show_in_* inheritance issue whenever a new last-mile consumer comes out.
There was a problem hiding this comment.
Agreed, seems like a good idea to offer a single argument that would cover both show_in_rest and show_in_abilities as it looks like that would work best for this generic settings 👍🏻
There was a problem hiding this comment.
Hi @gziolo, @justlevine, @aaronjorbin, I agree a single argument covering both would make adoption easier. I think it deserves its own ticket as it also changes how settings register for the REST API, so I would keep this PR as is and open a follow-up 👍
- Name the class members like the users and content classes: register_settings_get(), execute_settings_get(), get_settings_get_input_schema(), get_settings_get_output_schema(), check_permission(), and a CATEGORY constant. - Label the ability `Get Settings`, verb first like the other core abilities, and say in the description that it requires an authenticated user who can manage options. - Reject duplicate names in the `fields` filter with `uniqueItems`. - Record the ability in the `@since` of wp_register_core_abilities(). - Tests: set up in wpSetUpBeforeClass() and clean up in wpTearDownAfterClass(), drop the try/finally around the faked action context, and add messages to the assertions.
Cover the run endpoint of `core/settings-get`: a logged-out request is unauthorized, a user who cannot manage options is forbidden, an administrator receives typed settings, the `group` and comma-separated `fields` filters narrow the result, an unknown group fails input validation, and a POST request to the read-only ability is rejected.
Hi @gziolo, option A was applied, a value that fails its schema is left out and your test was added 👍 |
Hi @gziolo, the initial settings are now also registered on wp_abilities_api_init with priority 1 and $new_allowed_options is restored 👍 |
Thank you for the review @gziolo, all the feedback was applied. |
Hi @gziolo, the rename and label from WordPress/ai#1087 were carried over 👍 |
| 'description' => '', | ||
| 'sanitize_callback' => null, | ||
| 'show_in_rest' => false, | ||
| 'show_in_abilities' => false, |
There was a problem hiding this comment.
The custom callback is a scenario that makes sense, thanks. And thank you @gziolo for the write up.
It doesn't need to be a blocker here, but I wish we could come up with a way to reduce the burden of maintainership on extenders as a way to also improve. Maybe a show_programmaticlly option? While I think REST and Abilties should be enough, I also would have said REST was enough not long ago.
| path: ../../../src/wp-content/themes/twentyeleven/inc/theme-options.php | ||
| - | ||
| message: '#^Parameter \#3 \$args of function register_setting expects array\{type\?\: string, label\?\: string, description\?\: string, sanitize_callback\?\: \(callable\(\)\: mixed\)\|null, show_in_rest\?\: array\|bool, default\?\: mixed, \.\.\.\}, ''twentyeleven_theme…'' given\.$#' | ||
| message: '#^Parameter \#3 \$args of function register_setting expects array\{type\?\: string, label\?\: string, description\?\: string, sanitize_callback\?\: \(callable\(\)\: mixed\)\|null, show_in_rest\?\: array\|bool, show_in_abilities\?\: array\|bool, default\?\: mixed, \.\.\.\}, ''twentyeleven_theme…'' given\.$#' |
There was a problem hiding this comment.
Instead of adding this to the ignore, I think we should fix it in twentyeleven
There was a problem hiding this comment.
Hi @aaronjorbin, you are right we should not have this type of ignores. The problem already existed, this PR only updates the message because of the new argument. I proposed the proper fix in #14137, once it lands this ignore can be removed.
Only `register()` stays public. The permission and execute callbacks are now private methods, registered through closures defined in the class, so callers run the ability through the Abilities API, such as `wp_get_ability( 'core/settings-get' )->execute()`, which validates the input and checks permissions first. This replaces the class docblock's note that the class is not part of the public API. Public methods are covered by Core's backward compatibility commitment whatever the docblock says.
Only init() and register() stay public. The permission and execute callbacks of `core/settings-get` and `core/settings-update` are now private methods, registered through closures defined in the class, so callers run the abilities through the Abilities API, such as `wp_get_ability( 'core/settings-get' )->execute()`, which validates the input and checks permissions first. The test that calls the update callback directly now captures it from the registration arguments, through the `wp_register_ability_args` filter, so register_ability() no longer returns the instance. Ported from WordPress/wordpress-develop#12141.
The `group` and `fields` descriptions now say that an omitted filter returns every setting, that an empty `fields` list returns all settings allowed by the group filter, and that both filters together return only the settings matching both.
`core/settings-get` exposes `siteurl` as `url`, while `core/get-site-info` uses `url` for the public site address, so "Site URL." did not tell the two apart. Reuse the installation URL description `core/get-site-info` already has for `wpurl`, which adds no new string. The `/wp/v2/settings` schema uses the same description.
When two settings are exposed to abilities under the same name, the later registration silently replaced the earlier one, as in the settings endpoint, but here the name also disappears from the `fields` enum and the output schema. Trigger a `_doing_it_wrong()` notice that names both options, so plugin authors can spot the conflict. The later setting is still the one exposed.
… saved. `get_option()` returns false for an option that was never saved and has no registered default. A boolean schema accepted that as a value and an array schema read it as an empty list, where `/wp/v2/settings` answers null. Read each setting with its registered default, as the settings endpoint does, and leave it out when there is none. Registered defaults and a boolean stored as false read as before.
The single-site test run regenerates the QUnit fixture from the live `/wp/v2/settings` schema, which now describes `siteurl` as the WordPress installation URL, so the committed fixture must carry the same text or the "version-controlled files are not modified" CI step fails.
Port the core change in WordPress/wordpress-develop#12141 (b7e45852a9). The group and fields descriptions now say that an omitted filter returns every setting, that an empty fields list returns all settings allowed by the group filter, and that both filters together return only the settings matching both.
Port the core change in WordPress/wordpress-develop#12141 (e817adfe0d). When two settings are exposed to abilities under the same name, the later registration silently replaced the earlier one, so the name also disappeared from the fields enum and the output schema. Trigger a _doing_it_wrong() notice that names both options, so plugin authors can spot the conflict. The later setting is still the one exposed. The plugin escapes the message with esc_html__(), as its coding standards require, where core uses __(), and marks that difference. Core now casts a name that is not a string to a string, so the marker on the plugin's option name fallback says that instead.
Port the core change in WordPress/wordpress-develop#12141 (ed318f1a4f). get_option() returns false for an option that was never saved and has no registered default. A boolean schema accepted that as a value and an array schema read it as an empty list, where /wp/v2/settings answers null. Read each setting with its registered default, as the settings endpoint does, and leave it out when there is none. Registered defaults and a boolean stored as false read as before.
Adds a read-only `core/settings-get` ability that returns WordPress settings as a flat map of setting name to value, optionally narrowed by settings group, by setting name, or both. It requires the `manage_options` capability. The code lives in the new private `WP_Abilities_Settings` class. Settings opt in with the new `show_in_abilities` argument of `register_setting()`, which defaults to `false`. When `true`, a setting is exposed. Developed in WordPress/ai#691 and #12141. Props jorgefilipecosta, gziolo, jorbin, justlevine, dkotter, logicrays, jason_the_adams, mukesh27, ovidiu-galatan, jeffpaul. Fixes #64605. git-svn-id: https://develop.svn.wordpress.org/trunk@64311 602fd350-edb4-49c9-b593-d223f7449a82
Adds a read-only `core/settings-get` ability that returns WordPress settings as a flat map of setting name to value, optionally narrowed by settings group, by setting name, or both. It requires the `manage_options` capability. The code lives in the new private `WP_Abilities_Settings` class. Settings opt in with the new `show_in_abilities` argument of `register_setting()`, which defaults to `false`. When `true`, a setting is exposed. Developed in WordPress/ai#691 and WordPress/wordpress-develop#12141. Props jorgefilipecosta, gziolo, jorbin, justlevine, dkotter, logicrays, jason_the_adams, mukesh27, ovidiu-galatan, jeffpaul. Fixes #64605. Built from https://develop.svn.wordpress.org/trunk@64311 git-svn-id: http://core.svn.wordpress.org/trunk@63461 1a063a9b-81f0-0310-95a4-ce76da25c4cd
|
Thank you all for the reviews, the initial version of the ability was merged, but the work did not stopped any further feedback is welcome and we will keep iterating on the ability. |
What?
Port of WordPress/ai#691 (renamed in WordPress/ai#1087), part of WordPress/ai#40. Adds a read-only
core/settings-getability.register_setting()gets ashow_in_abilitiesargument,falseby default.truereuses the setting's REST name and schema, soblognameis exposed astitle, as in/wp/v2/settings. An array withnameandschemakeys is used instead.register_initial_settings()flags 19 settings, such asblogname,posts_per_page, anddefault_comment_status.core/settings-getreturns the exposed settings as a flat map of name to value, optionally filtered bygroup,fields, or both. It requiresmanage_options.wp_abilities_api_init, so the ability finds them on cron, WP-CLI, and beforerest_api_init.WP_Abilities_Settingsclass, which the plannedcore/settings-updateability (Addcore/settings-updateability ai#764) can share.Testing
Verify unit tests are passing:
Manual test steps: #12141 (comment)
AI disclosure: issues found via AI-assisted code review; the fixes and this description were drafted with AI assistance (Claude Code) and reviewed by me.
Trac ticket: https://core.trac.wordpress.org/ticket/64605