-
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 |
|---|---|---|
|
|
@@ -43,9 +43,10 @@ final class WP_Abilities_Registry { | |
| * | ||
| * @see wp_register_ability() | ||
| * | ||
| * @param string $name The name of the ability. The name must be a string containing a namespace | ||
| * prefix, i.e. `my-plugin/my-ability`. It can only contain lowercase | ||
| * alphanumeric characters, dashes and the forward slash. | ||
| * @param string $name The name of the ability. The name must contain 2 to 4 segments | ||
| * separated by forward slashes, e.g. `my-plugin/my-ability` or | ||
| * `my-plugin/resource/my-ability`. It can only contain lowercase | ||
| * alphanumeric characters, dashes, and forward slashes. | ||
| * @param array<string, mixed> $args { | ||
| * An associative array of arguments for the ability. | ||
| * | ||
|
|
@@ -78,11 +79,11 @@ final class WP_Abilities_Registry { | |
| * @return WP_Ability|null The registered ability instance on success, null on failure. | ||
| */ | ||
| 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 ) ) { | ||
|
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. 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 commentThe 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). |
||
| _doing_it_wrong( | ||
| __METHOD__, | ||
| __( | ||
| 'Ability name must be a string containing a namespace prefix, i.e. "my-plugin/my-ability". It can only contain lowercase alphanumeric characters, dashes and the forward slash.' | ||
| 'Ability name must contain 2 to 4 segments separated by forward slashes, e.g. "my-plugin/my-ability" or "my-plugin/resource/my-ability". It can only contain lowercase alphanumeric characters, dashes, and forward slashes.' | ||
| ), | ||
| '6.9.0' | ||
| ); | ||
|
|
||
| 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 ) |
||
| /** | ||
| * 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 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. )