-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Adds support for simple Ability versioning #10593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Adds support for simple Ability versioning #10593
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. |
|
I'm tempted to also add two methods to
|
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. |
| */ | ||
| public function register( string $name, array $args ): ?WP_Ability { | ||
| if ( ! preg_match( '/^[a-z0-9-]+\/[a-z0-9-]+$/', $name ) ) { | ||
| if ( ! preg_match( '/^[a-z0-9-]+\/([a-z0-9-]+\/)?[a-z0-9-]+$/', $name ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the example, where the second path segment is v1, the regex would seem to rather be:
| if ( ! preg_match( '/^[a-z0-9-]+\/([a-z0-9-]+\/)?[a-z0-9-]+$/', $name ) ) { | |
| if ( ! preg_match( '/^[a-z0-9-]+\/(v[0-9]+\/)?[a-z0-9-]+$/', $name ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about doing that but wasn't sure if we should have a direct opinion about how people version things. We're also not enforcing a a.b.c structure, either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having an opinion may make it easier to in the future just return the latest version for some ability. If we don't enforce a structure our only option is to rely on string sorting which has unexpected results e.g: v3 is higher than v13. I expected because of keeping context small/relevant for LLMS having a function that just returns the last version of each ability will be something common.
I'm tempted to force an integer value for the version as it seems to be the simplest approach with obvious outcomes when sorted. We could also support something like v[0-9]+(.[0-9]+){,2} if we want to be more flexible and support vX.Y, vX.Y.Z, where X, Y, Z are considered integers for sorting purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, and honestly I want to do that. This allows for things like version_compare being used. If someone doesn't follow that then I guess it could have odd behavior.
This does bring my deprecation PR to mind as well, which could refer to a version.
I feel like there are two directions we could go:
- No strict boundaries around versioning; just let implementers do their own thing
- Enforce PHP versioning so we can use versioning functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should enforce a versioning system but I don't think we need the full php version system something like v10.X.Y where both X and Y are optional should be enough, consumers of abilities like the JS client package, or something consuming the rest API in a random programming language may also need to compare versions, and having the full php versioning system with RC etc adds more complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If start with a strict subset in the future we can expand, but starting broad and then stricting may not be possible that's also the reason I would prefer a stricter approach at the beginning.
| /** | ||
| * Should reject ability name with more than two slashes. | ||
| * | ||
| * @ticket 64098 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update the ticket number to 64345.
jorgefilipecosta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is in a good shape, I think after updating the regex to be stricter we can merge this update.
|
Copy-pasting from Slack thread: I don't think we should encourage versioned abilities. It goes against WordPress Core philosophies, and I think it's going to lead to a somewhat messy ecosystem. FWIW: REST endpoints tend to have versions, but they're almost never used. In other words, the fact that it's called |
felixarntz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking as requesting changes due to the ongoing discussion whether we should do this - in practice this is probably more of a "requesting to close PR" feedback, but all pending the discussion outcome.
|
Strong +1 to @felixarntz and @justlevine on keeping the Ability identifier stable. From an agent perspective (MCP tools / AI agents), stable identifiers are critical for autonomy and long-term usability. Hard versioning risk (wp/v1/generate-featured-image): Schema-evolution approach (wp/generate-featured-image + metadata): The agent calls the same Ability name. The system can respond with a warning or schema change (e.g. “field X is deprecated, use Y”). The agent (or client library) can self-heal by updating the payload and retrying, without changing the identifier it was trained on. This matches what we’re seeing in most agent/tool ecosystems: Identifier = intent / capability (stable). Schema + metadata = what evolves (version, deprecation, preferred args). Keeping the Ability name evergreen and pushing evolution into the schema/metadata gives us better discovery, fewer hard breaks, and a much cleaner story for AI and non-AI clients alike. |
|
Shared my feedback directly to trac, since PR comments are one-directional: https://core.trac.wordpress.org/ticket/64345#comment:5 |
|
Thanks for all the really great thoughts! I definitely see where folks are coming from. To help make things concrete, let me frame an example: In my mind there are two sides to this, and they approach the same issue:
Let's say I'm GiveWP and I have a givewp/create-donation ability. Later, we add (true story) Campaigns, and make it so that every donation must be associated with a campaign via a campaign_id. Ok, so now we need to update the ability. This means two things: First, I'll need to create a new ability so the old one still works to at least some extent, or for a certain period of time. This presents the new dilemma: what do I name the ability? givewp/create-donation-with-campaign? givewp/cteate-donation-final? It's still the same thing, it just has different input and output parameters. Versioning helps with the first via givewp/v1/create-donation and then givewp/v2/create-donation. Deprecation helps with the second with the ability to mark an ability with deprecation and an ID to the new one. Now, one could argue that in this case GiveWP should somehow make the Ability backwards compatibility. I'm not sure how that would be possible, but in any case it forces architectural philosophy on the developer. We don't know them or their market, so I want to avoid that. I like @justlevine's (on trac) creative approach to include versioning as part of a single ability, but this actually touches on a point I just made elsewhere today: for maximize an Ability's chances of working well multi-contextually, simplicity is key. Having Abilities that grow in complexity like that hurts discoverability and reduces the chances of it working easily in many contexts. As a note, it "subdomains" in the Ability name is effectively what's being proposed here, at least in its simplest form. I'm leery about trying to shove in too much of a versioning concept, which is why I'm presenting a "versioned" Ability as actually multiple Abilities. It's not one that's versioned; it's many that include an optional version as part of the namespace. I emphasize optional because if in core, for example, we philosophically do not want to version and commit to backwards compatibility, then we can omit the version and call it a day. The closest I get to a concept of versioning is (and I definitely won't die on this hill) the idea of a "latest" Ability. That is, if we have I'd love to hear from folks using this GiveWP example as reference, or any other scenarios others want to introduce, to help this conversation be as concrete as possible. 😄 |
|
Sharing here for discussion an alternative approach following the same pattern we use for blocks in Gutenberg, which has been working for many years. The attributes of a block may also change, some removed, some added, but we need to keep supporting blocks created using old versions relying on old attributes, etc. So it is a very similar scenario. Basically, abilities would not specify a version; they would be registered normally but could have an array with old deprecated abilities: Consumers of an ability relying on a specific input or output schema: e.g., because they programmatically use the output or because they document the input schema in an LLM prompt, would specify the version of the schemas they are relying on: If the expected schema does not match the current one, the system would look for a deprecated version matching the expected schemas and use the corresponding callbacks. If no schemas are specified, it would use the current version, provided the input args match its schema; otherwise, it would automatically use a deprecated version whose schema matches the input args. For example renaming an input parameter would be easy deprecation calling the new callback mapping the old name to the new one. For people using the old input things would still automatically work even if they don't specify the schema they are expecting version etc. |
|
Ohhh, now that's fascinating, @jorgefilipecosta. I'll have to chew on that. 🤔 |
Trac ticket: 64345
This adds the ability for an Ability name to have a version. Both of these would be valid names:
wp/generate-featured-imagewp/v1/generate-featured-imageAs noted in the Trac ticket, this was intended, but we didn't catch that the validation regex rejects this.