Add a core/settings ability#691
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
jorgefilipecosta
force-pushed
the
add/core-settings-ability
branch
from
4c74702 to
413c6a4
Compare
jorgefilipecosta
force-pushed
the
add/core-settings-ability
branch
2 times, most recently
from
52920c8 to
a431013
Compare
✅ WordPress Plugin Check Report
📊 ReportAll checks passed! No errors or warnings found. 🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check |
|
@jeffpaul or @dkotter, what are the conditions for this new (and other planned) ability to get included in the AI plugin? Code-wise it's shaping up nicely according to what we discussed in #40 and in https://core.trac.wordpress.org/ticket/64605. The idea would be to land it in the plugin first for early testing, and then put it in WordPress core. |
| */ | ||
| public static function settings_map(): array { | ||
| return array( | ||
| // General. |
There was a problem hiding this comment.
Noting that some settings will get duplicated between core/get-site-info and core/settings.
It seems like we will have to follow up in WP core to replicate show_in_abilities check inside core/get-site-info once that concept is there.
More broadly, show_in_abilities might probably need more variety, for example:
`show_in_abilities` => true,
`show_in_abilities` => false,
`show_in_abilities` => 'read',
`show_in_abilities` => 'write',There was a problem hiding this comment.
Yah show_in_abilities will pass by some evolution in some cases like show_in_rest it may also need to pass schema information, and read/write distinction may also be a good approach. Would you want me to already include read/write distinction as part of this PR or do you think this is something for the future?
There was a problem hiding this comment.
For this PR, we don't need to do anything yet, as true is more than enough. However, I can see how core/manage-settings might benefit from a more granular decision process of what should be editable. Some settings should not be changed in isolation, as that could break the site.
jorgefilipecosta
force-pushed
the
add/core-settings-ability
branch
from
a9811cb to
66c05ed
Compare
Mirrors the fixes from the core/settings review (#691) that also apply here: - Memoize the exposed post types so the input schema and the permission/execute callbacks derive from a single walk of the registered post types. - Default the input schema to an empty object so the type:object default serializes as {}. - Use __() instead of esc_html__(), and @SInCE x.x.x per CONTRIBUTING.md. - Harden input/value handling (type guards, capability resolver, non-negative int helper) so the new code passes PHPStan at the strictest level. The `content` ability-category fallback is kept on purpose: unlike `site`, `content` is a new category not present on the plugin's minimum WordPress (7.0).
|
Hi @gziolo, thank you a lot for the review I think I applied/answered all the feedback. |
'settings' reads more naturally than 'slugs' for an ability about settings:
executeAbility( 'core/settings', { settings: [ 'blogname' ] } ).
Add an e2e sample plugin that registers a setting with show_in_abilities, map it in .wp-env.test.json, and assert the core/settings ability returns it.
- Simplify the input schema: replace the group-XOR-name `oneOf` with optional,
combinable `group` and `fields` filters (rename `settings` -> `fields`, matching
core/get-site-info). Default to an empty object so the type:object schema default
serializes as {}.
- Memoize the exposed settings so the input/output schema and execute() derive from a
single walk of get_registered_settings().
- Cast object-typed values to objects so they serialize as {} (not []) and satisfy the
output schema validated by execute().
- Drop the redundant `site` ability-category fallback (core registers it and the plugin
requires WP 7.0).
- Use __() instead of esc_html__() for ability strings, and @SInCE x.x.x per CONTRIBUTING.md.
- Harden value handling so the new code passes PHPStan at the strictest level.
- Tests: assert keys order-insensitively and cover combined group+fields filtering.
Co-authored-by: Darin Kotter <darin.kotter@gmail.com>
jorgefilipecosta
force-pushed
the
add/core-settings-ability
branch
from
7761b51 to
fe8c56a
Compare
…dation - Correct the get_settings_input_schema() @param annotations from list[] to list<string> ($groups and $field_names are lists of strings). - Fold the show_in_abilities sample setting into the existing e2e-request-mocking plugin instead of a separate e2e-sample-settings plugin, and drop the now-unused plugin from .wp-env.test.json.
Generalize the former e2e-request-mocking plugin into a shared E2E support plugin now that it also registers test fixtures: - Rename the directory and main file to tests/e2e-testing/ and update the plugin header name to "E2E Testing". - Point .wp-env.test.json and the architecture doc at the new path. - Update the activate/deactivate slug (e2e-test-request-mocking -> e2e-testing) in the global setup and specs, plus fixture-path comments.
- Document the show_in_abilities timing contract: the core/settings ability snapshots the exposed set at wp_abilities_api_init, so a setting must be flagged before that hook to be picked up. - Drop the unreachable recompute branch in execute_get_settings(); the cache is always populated at registration. Keep a defensive null guard so the unexpected case is explicit and stays PHPStan-max clean.
|
Hi @gziolo your feedback was applied.
It was documented on Show_In_Abilities class.
Fixed 👍 |
|
Hi @dkotter all your feedback was applied. Thank your for the review! |
Co-authored-by: Dovid Levine <justlevine@gmail.com>
Address review feedback: make Show_In_Abilities instance-based instead of all public static, matching the rest of the plugin (e.g. the Posts ability). The class is now final; register() and mark_setting() are instance methods (mark_setting stays public as a register_setting_args callback), and settings_map() is private. Main bootstraps it via ( new Show_In_Abilities() )->register(), and the tests hold the instance so the filter is detached through the same object on tear down.
Address review feedback: make Settings final and tighten member visibility so the intended surface is enforced rather than only documented via @internal. Only the externally-invoked entry points stay public static (init(), register(), execute_get_settings(), has_permission()); register_get_settings() and the shared helpers become private static, and CATEGORY becomes a private const. Method bodies and the static cache are unchanged, keeping parity with the core WP_Settings_Abilities class (mirrored there in lockstep).
|
Thank you a lot for the review @justlevine and raising good points I applied/answered your feedback 👍 |
Per review feedback, move register() and the ability callbacks (execute_get_settings(), has_permission()) from public static to public instance methods, and the internal helpers to private instance methods. The exposed-settings cache is now an instance property. init() stays a public static entry point and hooks a fresh instance. Mirrored in the core PR.
Drop the static init() entry point; Main now bootstraps the ability with ( new Settings_Ability() )->init(), matching the rest of the plugin's instance-based wiring. The class is now fully instance-based apart from the CATEGORY constant.
|
@justlevine I think I applied the remaining point of removing static from settings. |
Bring SettingsTest in line with core's wpRegisterCoreSettingsAbility: adopt the core test_core_settings_* method names, add the assertions core makes but the plugin was missing (show_in_rest + destructive on the registration test; schema type, the default key, and the general/ posts_per_page enum values on the input-schema test), and add a test that a custom show_in_abilities-flagged setting is exposed in the input fields enum, the output schema, and execute() output (previously the plugin had no output_schema coverage at all). Also document the settings_map() <-> core register_initial_settings() sync contract in Show_In_Abilities.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #691 +/- ##
=============================================
+ Coverage 74.45% 76.59% +2.14%
- Complexity 1740 1822 +82
=============================================
Files 85 87 +2
Lines 7521 7735 +214
=============================================
+ Hits 5600 5925 +325
+ Misses 1921 1810 -111
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Yup. Codewise LGTM, I have some upstream concerns regarding the naming aspects of |
| } | ||
|
|
||
| $group = isset( $input['group'] ) && is_string( $input['group'] ) ? $input['group'] : ''; | ||
| $fields = isset( $input['fields'] ) && is_array( $input['fields'] ) ? $input['fields'] : array(); |
There was a problem hiding this comment.
I was checking WP CLI and their wp option list command. Options are a lower-level concept than settings, but they have some things in common. I have some second thoughts about using fields in this specific context, as an individual setting has more fields than a value, for example: group.
On one hand, the current approach aligns closely with core/get-site-info and how it operates, but that's a curated set of site settings.
On the other hand, if we look at #739 and core/content, it's evident that fields are used there for individual rows rather than filtering which rows to pick.
5 participants
Summary
Part of: #40
Adds the
core/settingsability to the plugin, matching the equivalent WordPress core change in WordPress/wordpress-develop#12141. Read-only; returns settings flaggedshow_in_abilitiesas a flatname => valuemap (metadata in the output schema), filterable bygrouporslugs(mutually exclusive). Requiresmanage_options.Settings— registerscore/settings. Kept near-identical to core'sWP_Settings_Abilities(differences marked with// Plugin:comments). It registers at priority 11 and unregisters any core-provided copy first, so the plugin's version wins when both are present.Show_In_Abilities— seeds theshow_in_abilitiesflag onto a curated set of core settings via theregister_setting_argsfilter, so the ability returns data on a stock site before core ships the flag. Object-type-agnostic, for future reuse (post types, meta).Test plan
Prerequisite: the client-side Abilities API (
@wordpress/abilities) is only loaded in the block editor once an AI experiment is enabled — the experiment's editor script declares the@wordpress/abilities+@wordpress/core-abilitiesmodules as dependencies, which adds them to the editor's import map. So first enable an experiment such as Excerpt Generation and open a new post (post-new.php). Then, in the browser console:name => valuemap with typed valuesgroup/slugsfilters work; supplying both is rejectedmanage_options) is deniedIntegration tests added under
tests/Integration/Includes/Abilities/. An end-to-end test (tests/e2e/specs/abilities/core-settings.spec.js) drives the ability through the client-side Abilities API from the editor (with an experiment enabled).Core PR: WordPress/wordpress-develop#12141