Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/wp-includes/abilities-api.php
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@
*
* Ability names must follow these rules:
*
* - Include a namespace prefix (e.g., `my-plugin/my-ability`).
* - Contain 2 to 4 segments separated by forward slashes
* (e.g., `my-plugin/my-ability`, `my-plugin/resource/find`, `my-plugin/resource/sub/find`).
* - Use only lowercase alphanumeric characters, dashes, and forward slashes.
* - Use descriptive, action-oriented names (e.g., `process-payment`, `generate-report`).
*
Expand Down Expand Up @@ -225,8 +226,9 @@
* @see wp_register_ability_category()
* @see wp_unregister_ability()
*
* @param string $name The name of the ability. Must be a namespaced string containing
* a prefix, e.g., `my-plugin/my-ability`. Can only contain lowercase
* @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
Comment on lines +229 to +231

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. )

* alphanumeric characters, dashes, and forward slashes.
* @param array<string, mixed> $args {
* An associative array of arguments for configuring the ability.
Expand Down Expand Up @@ -318,7 +320,7 @@ function wp_register_ability( string $name, array $args ): ?WP_Ability {
* @see wp_register_ability()
*
* @param string $name The name of the ability to unregister, including namespace prefix
* (e.g., 'my-plugin/my-ability').
* (e.g., 'my-plugin/my-ability' or 'my-plugin/resource/find').
* @return WP_Ability|null The unregistered ability instance on success, `null` on failure.
*/
function wp_unregister_ability( string $name ): ?WP_Ability {
Expand Down Expand Up @@ -351,7 +353,7 @@ function wp_unregister_ability( string $name ): ?WP_Ability {
* @see wp_get_ability()
*
* @param string $name The name of the ability to check, including namespace prefix
* (e.g., 'my-plugin/my-ability').
* (e.g., 'my-plugin/my-ability' or 'my-plugin/resource/find').
* @return bool `true` if the ability is registered, `false` otherwise.
*/
function wp_has_ability( string $name ): bool {
Expand Down Expand Up @@ -383,7 +385,7 @@ function wp_has_ability( string $name ): bool {
* @see wp_has_ability()
*
* @param string $name The name of the ability, including namespace prefix
* (e.g., 'my-plugin/my-ability').
* (e.g., 'my-plugin/my-ability' or 'my-plugin/resource/find').
* @return WP_Ability|null The registered ability instance, or `null` if not registered.
*/
function wp_get_ability( string $name ): ?WP_Ability {
Expand Down
11 changes: 6 additions & 5 deletions src/wp-includes/abilities-api/class-wp-abilities-registry.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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 ) ) {
Copy link
Member Author

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.

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).

_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'
);
Expand Down
4 changes: 2 additions & 2 deletions src/wp-includes/abilities-api/class-wp-ability.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class WP_Ability {

/**
* The name of the ability, with its namespace.
* Example: `my-plugin/my-ability`.
* Examples: `my-plugin/my-ability`, `my-plugin/resource/find`.
*
* @since 6.9.0
* @var string
Expand Down Expand Up @@ -340,7 +340,7 @@ protected function prepare_properties( array $args ): array {

/**
* Retrieves the name of the ability, with its namespace.
* Example: `my-plugin/my-ability`.
* Examples: `my-plugin/my-ability`, `my-plugin/resource/find`.
*
* @since 6.9.0
*
Expand Down
68 changes: 68 additions & 0 deletions tests/phpunit/tests/abilities-api/wpAbilitiesRegistry.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
}

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 )

/**
* Should reject ability registration without a label.
*
Expand Down
62 changes: 62 additions & 0 deletions tests/phpunit/tests/rest-api/wpRestAbilitiesV1RunController.php
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,43 @@ private function register_test_abilities(): void {
)
);

// Ability with nested namespace (3 segments).
$this->register_test_ability(
'test/math/add',
array(
'label' => 'Nested Add',
'description' => 'Adds numbers with nested namespace',
'category' => 'math',
'input_schema' => array(
'type' => 'object',
'properties' => array(
'a' => array(
'type' => 'number',
'description' => 'First number',
),
'b' => array(
'type' => 'number',
'description' => 'Second number',
),
),
'required' => array( 'a', 'b' ),
'additionalProperties' => false,
),
'output_schema' => array(
'type' => 'number',
),
'execute_callback' => static function ( array $input ) {
return $input['a'] + $input['b'];
},
'permission_callback' => static function () {
return current_user_can( 'edit_posts' );
},
'meta' => array(
'show_in_rest' => true,
),
)
);

// Read-only ability for query params testing.
$this->register_test_ability(
'test/query-params',
Expand Down Expand Up @@ -432,6 +469,31 @@ public function test_execute_regular_ability_post(): void {
$this->assertEquals( 8, $response->get_data() );
}

/**
* Test executing an ability with a nested namespace (3 segments) via REST.
*
* @ticket 64098
*/
public function test_execute_nested_namespace_ability(): void {
$request = new WP_REST_Request( 'POST', '/wp-abilities/v1/abilities/test/math/add/run' );
$request->set_header( 'Content-Type', 'application/json' );
$request->set_body(
wp_json_encode(
array(
'input' => array(
'a' => 10,
'b' => 7,
),
)
)
);

$response = $this->server->dispatch( $request );

$this->assertEquals( 200, $response->get_status() );
$this->assertEquals( 17, $response->get_data() );
}

/**
* Test executing a read-only ability with GET.
*
Expand Down
Loading