Allow abilities to be deprecated

[jorbin]

Currently, abilities only exist in a state of existing or not. There is no way to indicate that an ability exists but is no longer the preferred ability to use for functionality.

[jason_the_adams]

Fair point! I’m curious what you anticipate as the expected behavior of deprecated abilities. WordPress also doesn’t have a formal way of marking a function as deprecated outside of adding the _deprecated_function function to the method, which could be used here as well.

To be clear, I’m not opposed to the idea, just taking a moment to think through it. 🙂

[jorbin]

In general, I think that deprecated abilities should still be callable, but not as discoverable. A handful of ideas for this:

1. @gziolo ​suggested not including deprecated abilities by default when calling wp_get_abilities()

We support meta.show_in_rest to decide whether to expose the ability in REST API. I think it would make perfect sense to offer meta.deprecated which would remove the ability from wp_get_abilities() return set. At the same time, it should still be possible to get a deprecated ability, but we can log that fact when calling wp_get_ability() or at any other stage that makes the most sense.

2. Building on that, I think a new _deprecated_ability function could be added so that if you call ‎WP_Ability::do_execute‎ on a deprecated ability, it could trigger a loggable error. Though I could also see this as being the responsibility of what is registered for the callback.

3. I think meta.deprecated would be false by default or it could be a string that would be

4. When calling the wp-abilities/v1/abilities endpoint, an optional deprecated argument could be added which will make the deprecated abilities visable. Otherwise, it would be hidden.

5. I don't think _deprecated_function makes sense unless the actual function is being deprecated. Image an ability who's callback is a function used elsewhere that is not deprecated but there is a desire to replace it with something more robust and to do so in a way that keeps compatibility with the original ability.

[jason_the_adams]

All of these ideas sound good to me! I especially agree with the introduction of _deprecated_ability() and it's automatic usage upon execution. Since we're introducing deprecation as a formal concept, I think it's sensible to call that automatically. Otherwise, it would be an odd behavior for an ability to mark itself as deprecated and not call that.

Are we in fact aiming this for 6.9? If so, we'll need another PR for introducing basic filtering to wp_get_abilities().

[wildworks]

With the RC1 release approaching, I'm considering how to handle this ticket.

In my opinion, introducing the _deprecated_ability() seems more like a feature enhancement than a bug fix. Should we punt this ticket to 7.0?

[soyebsalar01]

Agree with this is good as a feature enhancement than a bug fix.

Punting this in 7.0 makes a sense so there's a enough time for testing.

When trunk reopens I'll be happy to test and patch for _deprecated_ability().

[wildworks]

Since the RC1 release is tomorrow, let's punt this ticket to 7.0.

[jorbin]

I think this might be worth continuing to explore with the possibility of either 6.9 or 6.9.1.

I put this as a bug since I view the lack of this functionality to be an oversight rather than a specific design decision. There is precedence for adding new functions in a minor release when it is decided that the API is incomplete without them. See [15467] for an example.

This introduces the ability for formally deprecate an Ability, which will be an important system for allowing ability iteration without resulting in multiple discoverable abilities of the same kind. It tackles this on a few fronts:

1. Adds a _deprecated_ability() method
2. Adds a meta.deprecated property to WP_Ability
3. Calls _deprecated_ability in WP_Ability::do_execute() if the ability is deprecated

To do:

• [ ] Hide deprecated abilities from wp_get_abilities()
• [ ] Add ability to fetch deprecated abilities from wp_get_abilities()
• [ ] Add parameter to Abilities endpoint for filtering by deprecated state

Some topics to discuss:

1. Should we add a way for an Ability to mark which version it was deprecated in?
2. Should we add a way for an Ability to specify which Ability should be used to replace it?
3. How do we want to handle filtering in wp_get_abiliites()?

[jason_the_adams]

Just added the draft PR above and would appreciate any thoughts on the questions I raised. I'll add my own thoughts in the PR. 😃

## Version

When looking over _deprecated_function() I saw that it has a $version parameter. I'm honestly not sure how useful that is here, especially since most abilities won't be introduced by WordPress.

If it is useful, then we'd need a way for an Ability to note the version, since this function is called automatically within do_execute(). I threw in a meta.deprecated_version in there for now, but I could also see allowing:

meta.deprecated = false;

meta.deprecated = true;

meta.deprecated = array(
        'version' => '1.2.3',
        'replacement' => 'foo/v2/my-ability'
); // implied true with further information

I'm kind of partial to the array structure as it only adds a single property to meta and is nice and clear.

## Replacement

This 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 meta.deprecated_replacement type of property, or else include it in the array structure.

## Filtering in wp_get_abilities()

We purposely ​put off filtering in a previous bit of work because we realized it shouldn't act like WP_Query or something like that since this is all in memory. We put it off as we didn't see an immediate need to provide filtering since the person could just do array_filter( wp_get_abilities(), function() { ... } to get what they want.

That argument still applies here, but if the intent is to have deprecated abilities hidden by default, then that method falls apart. So we either have to introduce filtering or else not include it and have it such that folks filter it out themselves if that's their intent. The REST endpoint would work fine this way. The issue with this is that if we introduce filtering later and want deprecated abilities filtered by default, then we're changing the function's behavior.

We could just introduce a wp_get_abilities( array $args ) which is array( 'deprecated' => false ' ) by default, so passing wp_get_abilities( array() ) would get everything. At that point we can think about further filters later. But I could even be talked into a plain $include_deprecated = false boolean parameter, as I still think that most filtering is better done with something like array_filter().

Open to thoughts!

[justlevine]

Only saw this ticket tonight (thanks @jason_the_adams ), and wanted to share a few quick thoughts before I find time to look at everything more deeply:

1. I think this should be considered an enhancement, not a "bug", and definitely not rushed into 6.9.x. I don't remember if this was ever discussed explicitly (cc @gziolo ), but at least on my part it was an intentional exclusion, and if it was brought up I'd have advocated against inclusion to let the API continue to evolve holistically despite the short initial merge window. As mentioned by others, existing _deprecated_*(), and _doing_it_wrong(), and several other ways established ways exist to communicate deprecations and API changes. Imperfect parallel, but AFAIK there's no explicit way to deprecate a REST endpoint for example, you don't even get a notice if you're hitting a v1/endpoint when a v2/endpoint has also been registered).

2. I don't think versioning - on its own at least, but maybe altogether? - is an ideal approach for this sort of API (I know we keep comparing it to REST, but IMO it's a lot more similar to Hooks, and REST becomes conceptually just an Adapter around a bunch of Abilities). Also anecdotally, most plugins I've seen and developers I've worked with don't seem to rely on REST or Blocks beyond WP core and a few "trusted" plugins, and I assume the same lack of version hygiene would limit anything programmatic (and we already have ways to inform users of changes that don't require a specific API).

3. Deprecating an entire ability is IMO the _least_ needing of a specific API (because, again, preexisting solutions). Much more urgent IMO are communicating granular deprecations that allow an ability to remain long-lived and evolve over time: input/output fields. I'm a big fan of WPGraphQL's versionless array{deprecationReason?:string, ...otherRegisterArgs} ​Deprecation API and was planning to suggest a similar pattern here (and still plan to once I have fresh eyes). Versionless also jive's more with Core's general aversion to breaking changes. And as much as I _hate_ to use AI as a reason for anything, it's a much friendlier pattern for overcoming the stale/training data problem with LLMs.

4. All that said, I'm liking the direction @jason_the_adams's draft PR in the very quick glance I took. Because of my aversion to versioning, I'd probably recommend we start out with meta.deprecated?: string|null. This would also let downstream experimentation or usage of explicit versioning for developers who prefer it (this would pair well if we decided to relax the ​only-one-forwardslash name requirement, which I thought we already did before the beta1 merge 😅). That said if we know we're gonna have multiple props, then I agree we should go with the array structure (ideally sharing the pattern used by inputs/outputs, another reason why IMO we should take as much time as needed to think about this holistically). Even w.o PHPStan array shapes, we're already nested so we're not losing any tooling vs a "top-level" meta.{deprecation_property}.

Anywhoo, I'll come back when I'm not sleep deprived and on deadline, but hopefully I made enough sense to at least buy me some time before y'all agree to try and quickly push something into 6.9rcx 🙏🤞😴

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" 🎉🙃🎉