-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Abilities API: Allow nested namespace ability names (2-4 segments) #10848
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?
Abilities API: Allow nested namespace ability names (2-4 segments) #10848
Conversation
Expand ability name validation from exactly 2 segments (namespace/ability) to 2-4 segments, enabling names like my-plugin/resource/find and my-plugin/resource/sub/find. Update the validation regex, error messages, docblocks, and add corresponding unit and REST API tests.
| */ | ||
| 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-]+){1,3}$/', $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'm not sure if we should limit to 4 segments or just allow unlimited segments, but I fear unlimited segments may endup being overused, and it better ti be more restrictive than than the oposite. In the future we can always go from 4 to unlimited but we can not go from unlimited to 4.
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.
@jorgefilipecosta I tend to agree with you (especially re the directionality of future compat) but just to follow the thought through: what is the concern of "too many" nested segments? Are there hypothetical scenarios you can think of where we would benefit from implementing a fixed limit now or as things evolve?
(PS: If there's a limit, I think 4 is a good number and practical guardrail. More than 3 layers of nesting and you likely want to recheck your assumptions about using Abilities; 2 semantic levels for folks who want to experiment with a versioning antipattern also seems fair).
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. |
| * @param string $name The name of the ability. Must contain 2 to 4 segments separated | ||
| * by forward slashes, e.g., `my-plugin/my-ability` or | ||
| * `my-plugin/resource/my-ability`. Can only contain lowercase |
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 would keep the number of segments here as an implementation detail, and out of the param description. E.g. Must be the fully-namespaced string identifier, e.g. my-plugin/my-abilityormy-plugin/resource/my-ability`. (and then also drop the lowercase etc, this function is unregistering an existing ability the requirement is that an ability exists with that identical identifier. )
| $result = $this->registry->register( 'test/add-numbers/', self::$test_ability_args ); | ||
| $this->assertNull( $result ); | ||
| } | ||
|
|
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.
Do we also need to test deregistering?
(IMO no since it's just string matching to something in the registry, no coersion. Related: https://github.com/WordPress/wordpress-develop/pull/10848/files#r2756209961 )
justlevine
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.
LGTM - left some comments to confirm no implicit assumptions
This PR Expands ability name validation from exactly 2 segments (
namespace/ability) to 2-4 segments, enabling names likemy-plugin/resource/findandmy-plugin/resource/sub/find.Details
The current ability naming convention only allows a single namespace separator (
my-plugin/my-ability). This is too restrictive for organizing abilities into logical resource groups. With this change, plugins can register abilities likecore/posts/find,core/posts/create, ormy-plugin/resource/sub/action.The validation regex in
WP_Abilities_Registry::register()changes from:to:
This allows the first segment plus 1-3 additional slash-delimited segments (2-4 total). The REST API route patterns in both controllers already accepted multiple slashes, so no route changes were needed.
This PR comes from a suggestion from @justlevine at #10665 (comment) :
And we plan to use this new type of naming for post abilities.
cc: @justlevine, @JasonTheAdams
Test plan
npm run test:php -- --group abilities-apiand verify all tests passcore/get-site-info, etc.) continue to worktest/math/addcan be registered and executed via REST