Repository navigation
Abilities API: Add a core/settings-get ability - #12141
jorgefilipecosta wants to merge 46 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
|
| // Compute once; execute_get_settings() reuses this exact structure. | ||
| $this->exposed_settings = $this->get_exposed_settings(); | ||
|
|
||
| $settings = $this->exposed_settings; |
There was a problem hiding this comment.
If there are no settings registered, then there is no need to register the ability as it won't return anything anyway.
There was a problem hiding this comment.
The ability is now not registered when no setting is exposed 👍
| * | ||
| * @access private | ||
| */ | ||
| final class WP_Settings_Abilities { |
There was a problem hiding this comment.
Nitpick, here and in other open PRs, it might make more sense to follow naming conventions from other places and use WP_Abilities_ prefix and follow with class-wp-abilities- as file name. This would better mirror how it would look when using namespaces: WordPress/Abilities/Settings.
There was a problem hiding this comment.
Renamed to WP_Abilities_Settings 👍
|
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 AI plugin named the write ability `core/settings-update` (see WordPress/ai#764), so update the comments that still called it `core/manage-settings`.
An empty PHP array is encoded as `[]`, so an ability whose output schema describes an object, such as `core/settings-get` when its filters match no setting, answered with a JSON array. When the output schema describes an object, send an empty object instead, so the response matches the schema. PHP callers still receive the plain array. Suggested in WordPress/ai#764, where `core/settings-update` worked around it with an `(object)` cast.
This reverts commit 60c7a8f. Keep `core/settings-get` the same as in the AI plugin, which answers an empty result as a plain array. Sending empty object results as `{}` is a general Abilities API change, which can be proposed on its own.
Abilities can initialize before or without `rest_api_init`, where the initial settings are registered, for example on cron or WP-CLI. So far `WP_Abilities_Settings::register()` registered them itself. Register them from a `wp_abilities_api_init` callback at priority 1 instead, before the core abilities, so the settings ability no longer handles the settings bootstrap. The callback keeps the existing checks: it does nothing once the REST API has registered the settings, and it restores `$new_allowed_options`, so saving Settings > General does not try to save `admin_email` (see WordPress/ai#1080).
|
Tested All exposed settings. Values are typed, and match await wp.apiFetch( { path: '/wp-abilities/v1/abilities/core/settings-get/run' } );{
"blogname": "My WordPress Website",
"blogdescription": "",
"siteurl": "http://127.0.0.1:9412",
"admin_email": "admin@localhost.com",
"timezone_string": "",
"date_format": "F j, Y",
"time_format": "g:i a",
"start_of_week": 1,
"WPLANG": "en_US",
"use_smilies": true,
"default_category": 1,
"default_post_format": "0",
"posts_per_page": 10,
"show_on_front": "posts",
"page_on_front": 0,
"page_for_posts": 0,
"default_ping_status": "open",
"default_comment_status": "open"
}By group: await wp.apiFetch( { path: '/wp-abilities/v1/abilities/core/settings-get/run?input[group]=reading' } );{ "posts_per_page": 10, "show_on_front": "posts", "page_on_front": 0, "page_for_posts": 0 }By setting name: await wp.apiFetch( { path: '/wp-abilities/v1/abilities/core/settings-get/run?input[fields][]=blogname&input[fields][]=posts_per_page' } );{ "blogname": "My WordPress Website", "posts_per_page": 10 }Both, which returns their intersection: await wp.apiFetch( { path: '/wp-abilities/v1/abilities/core/settings-get/run?input[group]=reading&input[fields][]=blogname&input[fields][]=posts_per_page' } );{ "posts_per_page": 10 }A stored value that fails its schema is left out. After storing a value outside the enum, e.g. with await wp.apiFetch( { path: '/wp-abilities/v1/abilities/core/settings-get/run?input[group]=discussion' } );{ "default_comment_status": "open" }
Invalid input (400): await wp.apiFetch( { path: '/wp-abilities/v1/abilities/core/settings-get/run?input[group]=nope' } );
// ability_invalid_input: Ability "core/settings-get" has invalid input. Reason: input[group] is not one of general, writing, reading, and discussion.A user without await wp.apiFetch( { path: '/wp-abilities/v1/abilities/core/settings-get/run' } );
// rest_ability_cannot_execute: Sorry, you are not allowed to execute this ability.Tested and written with AI assistance (Claude Code). |
…ts." This reverts commit ed2bfee. `core/get-site-info`, `core/get-user-info`, and `core/get-environment-info` shipped in 7.1 with an `array()` input default. As an object, the default reaches `wp_ability_normalize_input` filters as a `stdClass`, so a filter that reads it as an array breaks, and a filter that changes it changes the schema default for every later call. Aligning these defaults is a general Abilities API change, which can be proposed on its own.
`cast_value()` cast each stored value to its type before validating it, so
some values were read differently from `/wp/v2/settings`: a stored `'false'`
came back as `true`, a `stdClass` as `{}`, a list with gaps as a JSON
object, and `'abc'` in an integer setting as `0` instead of being left out.
As `WP_REST_Settings_Controller::prepare_value()` does, validate the stored
value against its schema, leave it out when the schema rejects it, and
sanitize it otherwise. Object values are still cast to objects, so an empty
one is sent as `{}`.
A plain `get_option()` already returns the registered default through
`filter_default_option`, so the exposed settings no longer keep it.
…get. A setting registered with a type outside the JSON types, such as `foo`, was added to the output schema and triggered `_doing_it_wrong()` on every run. As `WP_REST_Settings_Controller::get_registered_options()` does, only expose settings of a type the settings endpoint supports.
`wp_page_for_privacy_policy` is registered in the `reading` group next to `page_on_front` and `page_for_posts`, which `core/settings-get` already exposes. Flag it with `show_in_abilities` too.
- Start the exposed settings as an empty array, which removes the unreachable null check in `execute_get_settings()`. - Collect the groups and the output schema properties with `array_column()` and `wp_list_pluck()` instead of a loop. - Read the setting type and group without the checks that `register_setting()` already guarantees, and the exposed name as the settings endpoint does. - Check `$show['schema']` with `isset()` alone. - Use the `site` category directly, as the other core abilities do, instead of a constant used once.
"Settings Get" follows the ability name but does not read as a label. The other core abilities start with the verb, as in "Get Site Information". The AI plugin uses the same label (see WordPress/ai#764).
Describe the `register_setting()` argument by its shape and the one rule integrators need, registering the setting on `init` or earlier. Drop a comment that would go stale once more settings abilities are added.
- Start the exposed settings as an empty array, which removes the unreachable null check and the (array) casts. - Collect the groups and the output schema properties with array_column() and wp_list_pluck() instead of a loop. - Read the setting type and group without the checks that register_setting() already guarantees, and the exposed name as the settings endpoint does. - Check $show['schema'] with isset() alone. - Use the site category directly instead of a constant. - Add null to the setting type directly in update_value_schema(), now that the type is always one the settings endpoint supports. Matches the core port in WordPress/wordpress-develop#12141.
This reverts commit 7f149df. The AI plugin labels the ability "Settings Get", in line with its other object-first abilities, since WordPress/ai#1087, which this PR carries over.
The other core abilities default their object input to `array()`, so use the same default here, instead of an object.
- Start the exposed settings as an empty array, which removes the unreachable null check and the (array) casts. - Collect the groups and the output schema properties with array_column() and wp_list_pluck() instead of a loop. - Read the setting type and group without the checks that register_setting() already guarantees, and the exposed name as the settings endpoint does. - Check $show['schema'] with isset() alone. - Use the site category directly instead of a constant. - Add null to the setting type directly in update_value_schema(), now that the type is always one the settings endpoint supports. Matches the core port in WordPress/wordpress-develop#12141.
|
Hi @gziolo, thank you for the reviews, the feedback was applied:
Let me know if there is anything else 👍 |
Restore the default that 656b453 changed to `array()`. An object is serialized as `{}` wherever the schema is read, and it keeps the class identical to the AI plugin's, which supports WordPress 7.0, where the abilities endpoints send an empty array default as `[]`.
|
jorgefilipecosta#91 – we should consider offering a solid strategy around settings registration that is predictable not only for WP core registered settings but also for those coming from plugins. I propsed we enforce that enabling |
WordPress stores `false` as `''`, which `rest_is_boolean()` rejects, so `core/settings-get` left out every boolean setting whose value is false, including a Settings API checkbox saved unchecked. The settings endpoint answers `null` for it. Read `''` as `false` for a boolean setting before validating it.
`WP_REST_Settings_Controller::get_registered_options()` runs `rest_default_additional_properties_to_false()` on every setting schema, so objects reject properties they do not declare. `core/settings-get` skipped that step, so it returned a stored object with an undeclared property whole, where the settings endpoint answers `null`. Close the schema in `value_schema()`, so the ability reads values, and describes them in its output schema, as the endpoint does.
The input schema accepts an object, but `execute_get_settings()` replaced anything other than an array with an empty one, so a PHP caller that passed `(object) array( 'group' => 'reading' )` got every setting back. Normalize the input with `rest_sanitize_object()`.
… exposure. Set the `public` meta flag, as the other core abilities do. This carries over WordPress/ai#1122. In core, `public` also enables `show_in_rest`. When `show_in_abilities` is `true`, a setting now uses the same name and schema as in `show_in_rest`. An array is still used as is. All core settings use `true`, so they share their names and schemas with the REST API settings endpoint. For example, `blogname` is exposed as `title` and `WPLANG` as `language`, and the email and enum schemas are no longer repeated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I pushed 749c2eb with two changes. 1. The ability is now public.
2. When Because of this, all core settings now use only
The other settings already had the same name in both APIs. One small side effect: New tests check that the names match the REST API, and that an array in Two follow-ups:
|
…fault.
Match the other core abilities. wp_prepare_json_schema_for_client()
already sends an empty object default as `{}`, for both the REST API
and the AI client. With an array, the execute callback also gets an
array when no input is given.
Add tests for how wp_prepare_json_schema_for_client() handles an
empty default on the root object schema.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…a separate PR. They cover existing behavior of the helper, so they do not belong to this change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`set_up_before_class()` swaps the core abilities registration hooks and clears the `rest_api_init` count, and only `tear_down_after_class()` put them back. The first test of a run snapshots the hooks, and every test then restores that snapshot, so whenever this class ran first, its changes leaked into every later test. Put the hooks and the count back right after registering the abilities.
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. Likeshow_in_rest, it can be an array withnameandschemakeys.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/v2/settingsreads them: validated against their schema, left out when the schema rejects them, and sanitized otherwise.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 (Add a core/settings-update ability 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