Abilities: Add support for ability deprecation - #10507
Conversation
|
Hi @JasonTheAdams! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
|
Some topics to discuss:
|
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. |
VersionWhen looking over If it is useful, then we'd need a way for an Ability to note the version, since this function is called automatically within meta.deprecated = false;
meta.deprecated = true;
meta.deprecated = array(
'version' => '1.2.3',
'replacement' => 'foo/v2/my-ability'
); // implied true with further informationI'm kind of partial to the array structure as it only adds a single property to ReplacementThis does feel more valuable, as it would be nice for someone using an Ability to know which Ability is intended to be its successor, if there is one available. As above, we could have a Filtering in
|
|
Hey @JasonTheAdams I touched a bit on your more general comments over on https://core.trac.wordpress.org/ticket/64209#comment:13, but tl;dr I agree with your comparison to ability filtering, and think any deprecation pattern should also be done as a holistic enhancement once the needs are more clearly defined. Which yeah will prob only come after/alongside a basic mechanism for filtering (but IMO no need to tunnel vision that in the rush to ship this) (Syndication is one directional to Trac, replying here to signpost future visitors.) PS: congrats on your "first contribution to WordPress" 🎉🙃🎉 |
bbda8b6 to
e21c3ee
Compare
|
The branch has been updated and rebased onto the latest
The structured metadata follows the general pattern used by Tests cover validation, defaults, execution notices, exact-name execution, discovery filtering, and REST API behavior. The new and changed API documentation uses A fresh CI run was triggered by the updated branch. Feedback welcome@justlevine and @JasonTheAdams, could you please review this revised version when you have time? I would especially value your feedback on the |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/wp-includes/abilities-api/class-wp-ability.php:399
replacementis documented and exposed as a namespaced ability name, but this validation accepts any non-empty string (for example,not an ability). That leaves consumers and deprecation notices with an unusable migration target. Validate this field with the same ability-name rule used byWP_Abilities_Registry::register(); existence need not be required because registration order can vary.
if ( ! is_string( $args['meta']['deprecated'][ $key ] ) || '' === $args['meta']['deprecated'][ $key ] ) {
throw new InvalidArgumentException(
sprintf(
/* translators: %s: Deprecation metadata key. */
__( 'The ability deprecation `%s` value should be a non-empty string.' ),
$key
)
);
src/wp-includes/rest-api/endpoints/class-wp-rest-abilities-v1-list-controller.php:410
- The schema accepts
trueas a deprecation-status filter, but registered abilities can only storefalseor an array.get_items()forwards the resulting boolean to_wp_get_abilities_match_meta(), whose strict comparison meansmeta[deprecated]=truecan never match a deprecated ability. Normalizetrueto the existingarray()sentinel before callingwp_get_abilities()(and cover that REST query), or constrain this schema and provide another REST-representable way to request only deprecated abilities.
'deprecated' => array(
'description' => __( 'Limit results by deprecation status or details.' ),
'type' => array( 'boolean', 'object' ),
|
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. |
jorgefilipecosta
left a comment
jorgefilipecosta
left a comment
There was a problem hiding this comment.
It seems to be on a good path left 3 possible enhancements I noticed while reviewing code.
|
Thank you @jorgefilipecosta for the thorough review. All three points are now addressed in the latest commit.
I also added descriptions to the |
Test reportTested against a fresh checkout of this branch (4 commits on trunk, PHP 8.5, MySQL). Automated (this PR's own PHPUnit suite): 164 tests / 417 assertions, all passing unmodified — Manual verification, registering a real deprecated ability (
No discrepancies or regressions found — everything matches the PR description. 👍 |
0b8152c to
be9e9a4
Compare
There was a problem hiding this comment.
Thanks so much for polishing this up for 7.2 @gziolo 🙌
Have some feedback regarding the implementation details, the main two concerns boil down to:
- what a user wants to do with (a list of) deprecated abilities.
- when a user should be informed that the ability is deprecated.
The rest is is more syntactic (why false|array<string, string> over ?array<string, string>, why a global _deprecated_ability() function that we don't want anyone else to call vs a private method, etc).
Either way, the proposed changes themselves are relatively minor, so I'm hopeful now that I'm back home and focused that we can discuss these and make any agreed-upon changes quickly, with more than enough time to include in 7.2.
I'll also create a followup PR for deprecating the specific input/output args, which IMO
- seems like a much bigger need than deprecating an entire ability
- seems like a conceptual prerequisite (not programmatic) for this PR, (since abilities should ideally be versionless, with deprecation of the entire ability as a last resort versus evolving the schema/changing the implementation details. Ideally we'd ship both for 7.2, and I'd be very hesitant to ship this one without that.)
Running abilities through the REST API is the main use case, so shipping the feature without it felt incomplete.
I hate to be pedantic, but running abilities via REST is not the main use case. Abilities are a functional primitive, not a transport, and the main use case is to be able to programmatically consume them without caring about the implementation details (which is why my other feedback is what it is).
Our REST API adapter implementation is just that: a convenience adapter of secondary importance.
(If folks haven't yet, I strongly recommend people watch Nik's amazing Abilities API for Humans talk ).
Practically, handling the deprecation message for REST is low effort in current implementation, so I see no reason to remove it now, but if some conflict arises in the future (e.g. as a result of changing when in the lifecycle the deprecation message is output) then I would be fine just suppressing the error and saving the custom keys for a followup PR.
| * @param string $replacement Optional. The ability that should be used instead. Default empty string. | ||
| * @param string $message Optional. Additional migration guidance. Default empty string. | ||
| */ | ||
| function _deprecated_ability( string $ability_name, string $version = '', string $replacement = '', string $message = '' ): void { |
There was a problem hiding this comment.
- Why do we need a
_deprecated_ability()at all? Is there- a reason not to map to the existing
_deprecated_function()and_deprecated_argument()? - a reason expose it to the global namespace instead of keeping it as a private implementation detail of
WP_Ability? Or phrased differently: when would userland code call_deprecated_ability()directly?
- a reason not to map to the existing
- If we do need a specific, global
_deprecated_ability()function, I'd recommend we colocate it in wp-includes/functions.php with the other_deprecated_*()functions.
There was a problem hiding this comment.
I agree with the location, since they're all in the same place. I think having a distinct method makes sense as we're calling Abilities a primitive, so I think having a primitive deprecation function makes sense.
There was a problem hiding this comment.
I think having a distinct method makes sense as we're calling Abilities a primitive, so I think having a primitive deprecation function makes sense.
Can you clarify this point? The function as written and implemented is should never be called by the end user, but is only meant to be called internally by our WP_Ability class. The existence of the class (or a private method on it) doesn't make it any less of a primitive.
So we if don't want anyone calling the function directly:
- Why would we pollute the global namespace with it?
- Why would we make it self-descriptive to a bunch of functions that are meant to be called by the end user?
(Prolly also worthwhile to note that there's no _deprecated_block() function for the same reasons as above; deprecations are defined via args)
| * @since 7.2.0 | ||
| * | ||
| * @param string $ability_name The ability that was executed. | ||
| * @param string $version Optional. The version of the ability provider that deprecated the ability. |
There was a problem hiding this comment.
If we decide we do need an _deprecated_ability(), let's take inspiration from the other _deprecated_*() functions and make $version required.
| * @param string $version Optional. The version of the ability provider that deprecated the ability. | |
| * @param string $version The version of the ability provider that deprecated the ability. |
There was a problem hiding this comment.
What's the importance of including a version here outside of consistency with other deprecation methods? Do we like that it's required there?
There was a problem hiding this comment.
A version tells the end user when the behaviour they were previously relying on changed, which makes it easier for them to find out what changed (this is helpful for humans, but a requirement for AI where the training data is often outdated).
Unlike a replacement (there might not be one) or a message (might not need one), there's always a version where the change is made, so it's low-effort to add, which makes the friction of needing to explicitly pass an empty string (if you really don't want to disclose what version you deprecated something) a good thing.
Lastly, going from non-optional arg to optional is a non-breaking change, so if in the future there's a concrete reason to knock down any Chesterton Fences that determined $version should be a requirement elsewhere, we can always change to $version = '' later. We can go from optional to non-optional if the method is private (not even protected, let alone a global as it is now).
| * @type false|array<string, string> $deprecated { | ||
| * Optional. Deprecation details. Set to an array to mark the ability as deprecated. At least one | ||
| * supported detail must be provided. Deprecated abilities remain available by exact name and can | ||
| * be explicitly included or excluded from discovery through meta filtering. Default false. |
There was a problem hiding this comment.
Is there a reason to default this to false? Considering we support WP7.4+, IMO much better to make this nullable, so a signature can strict-type it as : ?array since union return types aren't supported until 8.0.
| * @type false|array<string, string> $deprecated { | |
| * Optional. Deprecation details. Set to an array to mark the ability as deprecated. At least one | |
| * supported detail must be provided. Deprecated abilities remain available by exact name and can | |
| * be explicitly included or excluded from discovery through meta filtering. Default false. | |
| * @type null|array<string, string> $deprecated { | |
| * Optional. Deprecation details. Set to an array to mark the ability as deprecated. At least one | |
| * supported detail must be provided. Deprecated abilities remain available by exact name and can | |
| * be explicitly included or excluded from discovery through meta filtering. Default null. |
There was a problem hiding this comment.
Unless it's an important "WordPress way" thing, I agree with returning null instead of false.
|
|
||
| /* | ||
| * Normalize the `deprecated` meta filter shorthand. Stored values are `false` | ||
| * or an array of details, so `true` becomes an empty set of conditions that | ||
| * matches any deprecated ability. | ||
| */ | ||
| if ( isset( $meta['deprecated'] ) && true === $meta['deprecated'] ) { | ||
| $meta['deprecated'] = array(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Noting:
- The Fable-generated doc block is neither helpful (more than half of it is self-documenting), nor follows either 80ch or 120ch standards for breaks.
- The Fable-generated code block right below it doesn't adhere to the code-patterns used right above it for normalization either.
If this is behaviorally intentional, I'd make it an implementation detail of _wp_get_abilities_match_meta(), to keep the functionality self-contained. E.g.:
function _wp_get_abilities_match_meta( array $meta, array $conditions ): bool {
foreach ( $conditions as $key => $value ) {
if ( ! array_key_exists( $key, $meta ) ) {
return false;
}
// Support `deprecated: true` as a shorthand to match all deprecated abilities.
if ( 'deprecated' === `$key` && true === $value ) {
$value = [];
}
... rest of functionHowever, since those are telltale signs of AI generated-code lacking intentionality, I'm unclear as to how much of the rest of the implementation and use of _wp_get_abilities_match_meta() is intentional design vs AI-driven.
Behaviorally, I'd assume the following holistic shape when querying for abilities:
- By default,
deprecatedabilities are excluded. - Users can
$args['include_deprecated'] = trueif they want to get ALL abilities (per their filters), including those that have been deprecated. - There is no practical need to filter for only deprecated abilities. If someone wants to do that, they can
array_filter( $abilities_including_deprecated, 'my_only_deprecated_abilities_callback' )on the results themselves, or even use theitem_include_callback. - There is definitely no need to explicitly support filtering by a specific
meta['deprecated']['message', 'version', 'replacement']value. If someone really wants to do that, they can do it for free (without us growing the API) using the existingmetafilter, or again withitem_include_callbackor by manually filtering the results.
If there's human-led reasons for the current approach, I'd love to hear them and dive in to the discrepancies. If it's just "AI Slop" and lacking human intentionality, then 👆 is what I recommend we align to.
There was a problem hiding this comment.
cc: @gziolo (updated, hope that makes my concerns + intent clearer)
| * @type false|array<string, string> $deprecated { | ||
| * Optional. Deprecation details. Set to an array to mark the ability as deprecated. At least one | ||
| * supported detail must be provided. Deprecated abilities remain available by exact name and can | ||
| * be explicitly included or excluded from discovery through meta filtering. Default false. | ||
| * | ||
| * @type string $since Optional. Version of the ability provider that deprecated the ability. | ||
| * @type string $replacement Optional. Namespaced ability to use instead. | ||
| * @type string $message Optional. Additional migration guidance. | ||
| * } |
There was a problem hiding this comment.
Per #10507 (comment),
| * @type false|array<string, string> $deprecated { | |
| * Optional. Deprecation details. Set to an array to mark the ability as deprecated. At least one | |
| * supported detail must be provided. Deprecated abilities remain available by exact name and can | |
| * be explicitly included or excluded from discovery through meta filtering. Default false. | |
| * | |
| * @type string $since Optional. Version of the ability provider that deprecated the ability. | |
| * @type string $replacement Optional. Namespaced ability to use instead. | |
| * @type string $message Optional. Additional migration guidance. | |
| * } | |
| * @type null|array<string, string> $deprecated { | |
| * Optional. Deprecation details. Set to an array to mark the ability as deprecated. At least one | |
| * supported detail must be provided. Deprecated abilities remain available by exact name and can | |
| * be explicitly included or excluded from discovery through meta filtering. Default null. | |
| * | |
| * @type string $since Version of the ability provider that deprecated the ability. | |
| * @type string $replacement Optional. Namespaced ability to use instead. | |
| * @type string $message Optional. Additional migration guidance. | |
| * } |
|
|
||
| $deprecated = $this->get_meta_item( 'deprecated', false ); | ||
| if ( is_array( $deprecated ) ) { | ||
| _deprecated_ability( |
There was a problem hiding this comment.
Per #10507 (comment), I'm seeing no reason why _deprecated_ability()can't just beprivate function log_deprecated_ability(): void` instead of polluting the global namespace with something that seems should only be called by us in this part of the lifecycle.
| */ | ||
| do_action( 'wp_ability_invoked', $this->name, $input, $this ); | ||
|
|
||
| $deprecated = $this->get_meta_item( 'deprecated', false ); |
There was a problem hiding this comment.
-
Should the deprecation come before the
do_action()call too? I'm leaning toward yes, in line withdo_action_deprecated()and my general assumption regarding WP lifecycle. -
Bigger question, is
::execute()the right time to log the deprecation, or should it be when it's retrieved from the registry, somewhere else instead/additionally?I'm not entirely sure where the best place would be, just that
::execute()feels too late: Abilities are a developer API, which means that the developer should be warned that they're reaching for a deprecated ability when they reach for it (the ability itself, if trying to introspect the input/output schema, when perms are run, or something), and not just downstream when their users are executing it. Ideally we'd find the single earliest spot of ensured interaction over repeat warnings in the same lifecycle. 🤔
| if ( 'replacement' === $key && ! preg_match( '/^[a-z0-9-]+\/[a-z0-9-]+$/', $args['meta']['deprecated'][ $key ] ) ) { | ||
| throw new InvalidArgumentException( | ||
| __( 'The ability deprecation `replacement` value should be a namespaced ability name, i.e. "my-plugin/my-ability". It can only contain lowercase alphanumeric characters, dashes and the forward slash.' ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Please revert this. Even beyond #10507 (comment), it's a waste of regex. At least a ! wp_has_ability() would be nonspeculative, but even that is IMO overkill for a deprecation array.
| ), | ||
| 'additionalProperties' => true, | ||
| ), | ||
| 'deprecated' => array( |
There was a problem hiding this comment.
Per #10507 (comment) I think this is the wrong shape and use case. Filtering for
- only deprecated abilities
- deprecated abilities by the specific
sinceversion (remember, this is end-user not WordPress version),replacementormessage
Is not a common enough case to justify the inclusion for an initial API, especially when the actual common path (listing ALL abilities including deprecated ones) isn't included.
be9e9a4 to
7801e87
Compare
- Support `true` as a shorthand for the `deprecated` meta filter in `wp_get_abilities()`, so REST API and PHP callers can request only deprecated abilities. - Validate that the deprecation `replacement` detail is a namespaced ability name. - Handle the `deprecated_ability_run` action while serving REST API requests. The notice becomes an `X-WP-DeprecatedAbility` header instead of corrupting the JSON response when `WP_DEBUG` is enabled. - Document the `deprecated` collection parameter details in the REST API schema. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7801e87 to
cc6f70c
Compare
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
Thanks for the detailed review, @justlevine. Before another implementation pass, I would like to agree on the design decisions.
Moving _deprecated_ability() to functions.php and firing the notice before the action hook both make sense to me. I would keep the function global, consistent with the other _deprecated_*() helpers and potentially useful to adapters wrapping abilities. Do you see a concrete problem with that?
The bigger decision is discovery. We previously discussed excluding deprecated abilities by default. I changed that during the rebase because hiding them could break clients relying on discovery without giving them a deprecation signal. Including them by default with an opt-out felt safer for backward compatibility. Listing all abilities is already supported in both PHP and REST.
The two models would be:
- Current:
falsemeans active. Discovery includes all abilities. Filtering onfalsereturns active abilities, while thetrueshorthand returns deprecated ones. - Proposed:
nullmeans active. Discovery excludes deprecated abilities, withinclude_deprecatedto include them. We remove thetrueshorthand. Callers wanting only deprecated abilities can use an array condition or callback.
Both models reuse the existing meta matcher for filtering by individual details. Which trade-off do you prefer?
Replacement validation addressed @jorgefilipecosta’s review, so I would like us to align before removing it. Jorge, what do you think?
Execution-time notices follow the Trac discussion with @aaronjorbin and @JasonTheAdams. Retrieval-time notices could also fire during listing or introspection. Could you suggest a specific location and clarify which interactions should trigger a notice?
Input/output deprecation deserves its own ticket. Does anything in this PR constrain that work later?
Could you distinguish blockers from suggestions and identify the concrete problem each blocker addresses? That would give us a clear next implementation pass.
Finally, descriptions like “AI slop” make the discussion harder to engage with and do not help identify what needs changing. Please keep the feedback focused on behavior and trade-offs.
I go more into it in this comment reply, but yeah the problem I see is that per on the current implementation and unlike other Curious what potential you see to "adapters wrapping abilities" although my assumption is that anything there can be handled via a
Can you clarify what you mean by "relying on discovery"? Perhaps a concrete case of what could break? I cant think of anything that doesn't also obviate the need for an explicit deprecation path (vs e.g. a regular As there's currently no way to programmatically deprecate abilities, there are no abilities that are currently deprecated anyone is fetching, which mean there's no behavior change for anything currently using
I didn't follow this at all. Maybe it's because I don't see the value in the former, or the "trade-offs" in the latter? If the rest of my comment hasn't obviated this, please clarify.
Yeah, like I noted in my comment, I'm not entirely sure where it should be, just raising my concern that
As I noted on the ticket itself, other parallel conversation, and my previous reply, input/output deprecation are part-and-parcel to the discussion, if anything then a prerequisite to this specific part of ability deprecation. The only part of this PR that cant be accomplished via a user adding their own If anything work on that could potentially constrain what is done here but nothing that sticks out beyond the existing smells about what's global/public vs private.
I'm not sure what you mean, or what you would like me to do differently, but here's my attempt at categorization:
If there's something specific you want me to go deeper into, please @me on the comment and I'm happy to dive in. Usually I'm asked to be less verbose😅
Apologies, I thought we as a team had moved passed any subjective associations with the phrase. To clarify my comment, the
These two signs of
(The rest of that comment explains the behavior I'd expect holistically to make it easy to contrast. |
|
@justlevine, it sounds like you still have some design ideas you’d like to explore. Would you be willing to take ownership of this PR and carry it through to completion, including resolving the remaining design questions and implementing the changes? |
This PR adds a standard way to deprecate an ability without removing it.
Proposed changes
meta.deprecatedproperty. Its default value isfalse.since,replacement, andmessage._deprecated_ability()and show a deprecation notice when the ability runs.wp_get_abilities()or REST API filters.Why use structured metadata?
falseclearly means that an ability is active. An array keeps all migration details together and makes them available to PHP, REST API clients, and other tools without parsing a message.This follows the general pattern used by
@wordpress/deprecated, which also stores deprecation details as structured options. The proposed ability API uses a smaller set of fields that fits server-side ability metadata.Tests
The tests cover metadata defaults and validation, execution notices, exact-name execution, discovery filtering, and REST API behavior.
Trac ticket: https://core.trac.wordpress.org/ticket/64209