-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,6 +136,74 @@ public function test_register_invalid_uppercase_characters_in_name() { | |
| $this->assertNull( $result ); | ||
| } | ||
|
|
||
| /** | ||
| * Should accept ability name with 3 segments (2 slashes). | ||
| * | ||
| * @ticket 64098 | ||
| * | ||
| * @covers WP_Abilities_Registry::register | ||
| */ | ||
| public function test_register_valid_name_with_three_segments() { | ||
| $result = $this->registry->register( 'test/sub/add-numbers', self::$test_ability_args ); | ||
| $this->assertInstanceOf( WP_Ability::class, $result ); | ||
| $this->assertSame( 'test/sub/add-numbers', $result->get_name() ); | ||
| } | ||
|
|
||
| /** | ||
| * Should accept ability name with 4 segments (3 slashes). | ||
| * | ||
| * @ticket 64098 | ||
| * | ||
| * @covers WP_Abilities_Registry::register | ||
| */ | ||
| public function test_register_valid_name_with_four_segments() { | ||
| $result = $this->registry->register( 'test/sub/deep/add-numbers', self::$test_ability_args ); | ||
| $this->assertInstanceOf( WP_Ability::class, $result ); | ||
| $this->assertSame( 'test/sub/deep/add-numbers', $result->get_name() ); | ||
| } | ||
|
|
||
| /** | ||
| * Should reject ability name with 5 segments (exceeds maximum of 4). | ||
| * | ||
| * @ticket 64098 | ||
| * | ||
| * @covers WP_Abilities_Registry::register | ||
| * | ||
| * @expectedIncorrectUsage WP_Abilities_Registry::register | ||
| */ | ||
| public function test_register_invalid_name_with_five_segments() { | ||
| $result = $this->registry->register( 'test/a/b/c/too-deep', self::$test_ability_args ); | ||
| $this->assertNull( $result ); | ||
| } | ||
|
|
||
| /** | ||
| * Should reject ability name with empty segments (double slashes). | ||
| * | ||
| * @ticket 64098 | ||
| * | ||
| * @covers WP_Abilities_Registry::register | ||
| * | ||
| * @expectedIncorrectUsage WP_Abilities_Registry::register | ||
| */ | ||
| public function test_register_invalid_name_with_empty_segment() { | ||
| $result = $this->registry->register( 'test//add-numbers', self::$test_ability_args ); | ||
| $this->assertNull( $result ); | ||
| } | ||
|
|
||
| /** | ||
| * Should reject ability name with trailing slash. | ||
| * | ||
| * @ticket 64098 | ||
| * | ||
| * @covers WP_Abilities_Registry::register | ||
| * | ||
| * @expectedIncorrectUsage WP_Abilities_Registry::register | ||
| */ | ||
| public function test_register_invalid_name_with_trailing_slash() { | ||
| $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 commentThe 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 )
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly deregistering does not rely on our regex. |
||
| /** | ||
| * Should reject ability registration without a label. | ||
| * | ||
|
|
||
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).
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.
Hi @justlevine, I think a limit nudges plugin authors toward flat, meaningful names rather than mirroring deep internal hierarchies into ability names. If someone needs my-plugin/commerce/order/in-process/find, that's likely a sign the ability is too granular and should be rethought.