Skip to content

dev!: handle property registration inside WP_Ability #54

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 5, 2025
Merged

dev!: handle property registration inside WP_Ability #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3982ca4
dev!: handle property registration inside WP_Ability.
justlevine Sep 2, 2025
342e414
Merge branch 'trunk' into dev/WP_Ability-property-registration
gziolo Sep 5, 2025
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
78 changes: 12 additions & 66 deletions includes/abilities-api/class-wp-abilities-registry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,87 +85,33 @@ public function register( string $name, array $properties = array() ): ?WP_Abili
return null;
}

if ( empty( $properties['label'] ) || ! is_string( $properties['label'] ) ) {
_doing_it_wrong(
__METHOD__,
esc_html__( 'The ability properties must contain a `label` string.' ),
'0.1.0'
);
return null;
}

if ( empty( $properties['description'] ) || ! is_string( $properties['description'] ) ) {
_doing_it_wrong(
__METHOD__,
esc_html__( 'The ability properties must contain a `description` string.' ),
'0.1.0'
);
return null;
}

if ( isset( $properties['input_schema'] ) && ! is_array( $properties['input_schema'] ) ) {
_doing_it_wrong(
__METHOD__,
esc_html__( 'The ability properties should provide a valid `input_schema` definition.' ),
'0.1.0'
);
return null;
}

if ( isset( $properties['output_schema'] ) && ! is_array( $properties['output_schema'] ) ) {
_doing_it_wrong(
__METHOD__,
esc_html__( 'The ability properties should provide a valid `output_schema` definition.' ),
'0.1.0'
);
return null;
}

if ( empty( $properties['execute_callback'] ) || ! is_callable( $properties['execute_callback'] ) ) {
_doing_it_wrong(
__METHOD__,
esc_html__( 'The ability properties must contain a valid `execute_callback` function.' ),
'0.1.0'
);
return null;
}

if ( isset( $properties['permission_callback'] ) && ! is_callable( $properties['permission_callback'] ) ) {
// The class is only used to instantiate the ability, and is not a property of the ability itself.
if ( isset( $properties['ability_class'] ) && ! is_a( $properties['ability_class'], WP_Ability::class, true ) ) {
_doing_it_wrong(
__METHOD__,
esc_html__( 'The ability properties should provide a valid `permission_callback` function.' ),
esc_html__( 'The ability properties should provide a valid `ability_class` that extends WP_Ability.' ),
'0.1.0'
);
return null;
}
$ability_class = $properties['ability_class'] ?? WP_Ability::class;
unset( $properties['ability_class'] );

if ( isset( $properties['meta'] ) && ! is_array( $properties['meta'] ) ) {
_doing_it_wrong(
__METHOD__,
esc_html__( 'The ability properties should provide a valid `meta` array.' ),
'0.1.0'
try {
// WP_Ability::validate_properties() will throw an exception if the properties are invalid.
$ability = new $ability_class(
$name,
$properties
);
return null;
}

if ( isset( $properties['ability_class'] ) && ! is_a( $properties['ability_class'], WP_Ability::class, true ) ) {
} catch ( \InvalidArgumentException $e ) {
_doing_it_wrong(
__METHOD__,
esc_html__( 'The ability properties should provide a valid `ability_class` that extends WP_Ability.' ),
esc_html( $e->getMessage() ),
'0.1.0'
);
return null;
}

// The class is only used to instantiate the ability, and is not a property of the ability itself.
$ability_class = $properties['ability_class'] ?? WP_Ability::class;
unset( $properties['ability_class'] );

$ability = new $ability_class(
$name,
$properties
);

$this->registered_abilities[ $name ] = $ability;
return $ability;
}
Expand Down
83 changes: 72 additions & 11 deletions includes/abilities-api/class-wp-ability.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,21 +100,12 @@ class WP_Ability {
* @param array<string,mixed> $properties An associative array of properties for the ability. This should
* include `label`, `description`, `input_schema`, `output_schema`,
* `execute_callback`, `permission_callback`, and `meta`.
*
* @phpstan-param array{
* label: string,
* description: string,
* input_schema?: array<string,mixed>,
* output_schema?: array<string,mixed>,
* execute_callback: callable( array<string,mixed> $input): (mixed|\WP_Error),
* permission_callback?: ?callable( array<string,mixed> $input ): (bool|\WP_Error),
* meta?: array<string,mixed>,
* ...<string, mixed>,
* } $properties
*/
public function __construct( string $name, array $properties ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small part of me thinks that we should change all the references of properties to args to bring it inline with other WP core naming, now that we've broken the direct dependency to add a validator. 🤷

I could add it in this PR but i didnt want to obfuscate the discussion on #53

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have now ability_class which isn't strictly a property of the object, so I don't mind changing to args at this point. If you want to take care of it, let's put it into a separate PR to keep the current refactor lean.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to changing this to use $args.

$this->name = $name;

$this->validate_properties( $properties );

foreach ( $properties as $property_name => $property_value ) {
Comment on lines +107 to 109
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative approach is to replace the validate_*() pattern with the prepare_*(): array|WP_Error() , and then throw in the constructor.

This would improve the DX both mapping and validation in the same step, but I'm not sure how flexible we want things to be at this initial stage vs once we've given the initial API + round of feedback time to percolate, so I went with the approach in the diff.

e.g. (pseudocode)

$properties = $this->prepare_args( $args );

if ( is_wp_error( $properties ) ) {
  throw \InvalidArgumentException( $properties->getMessage() );
}

// This is still outside the function so extenders don't need to reimplement it.
foreach( $properties as $name => $value ) {
  ...
}

Copy link
Contributor Author

@justlevine justlevine Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gziolo / @felixarntz would love some specific thoughts on this if you have them. The more I think about it the more I feel like protected function prepare_properties( array<string,mixed> ): array<validated-shape>|WP_Error is the better approach 🤔

Copy link
Member

@gziolo gziolo Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so you prefer to have prepare_properties that returns WP_Error as soon as something goes wrong, and it gets translated to an exception, or the properties returned gets passed to the loop. That sounds good to me, to avoid having a method that only throws an exception when someting goes wrong. In fact, it might be simpler to ever introduce a filter if folks want to change this properties as an alternative to what I proposed here:

The filter used with block types register_block_type_args comes to my find as a good reference:

https://github.com/WordPress/wordpress-develop/blob/9b6c234bc6a78ce4a57929ea5413d0b08a3d706e/src/wp-includes/class-wp-block-type.php#L565-L573

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thoughts exactly. After the chat, I'll push a change with that approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not strongly opposed, but I also don't see the benefit of using prepare_properties and returning a WP_Error from it if something is wrong. Can you clarify why that's better?

To me it seems unnecessarily complex to rely on a method that can return WP_Error, only to turn that into an InvalidArgumentException, only to catch that and turn it into a _doing_it_wrong().

Why not simply throw the exceptions?

Copy link
Contributor Author

@justlevine justlevine Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarifying that my focus is on returning the array of (prepared) args instead of void. 😅

@@felixarntz I agree with you from a code quality POV

From an implementer POV my thought was less "exceptions should be exceptional" and more "let's do it the WP™️ way 💪" :

  • most the other parts of the exposed API return WP_Errors to be handled instead of throwing.
  • In general WordPress developers are more comfortable returning error objects than throwing.
  • most importantly: it gives me a justification to leave the return type off the method signature (since php7.4 doesn't support union return types) since I didn't want to argue about why a legacy project wanting to migrate to this API but keep their existing DTO object isn't a justifiable use case for polymorphic tech debt at this early stage. 😅

(I drafted this before you both aligned yesterday on #53 and only saw after I pushed, so in my head this was still very much a proposal ).

My takeaway from this conversation is that we're good not caring about that last point (between overloading and the filter if someone really, realy thinks shimming a DTO or VO into this api is a good idea, they have ways), so if y'all don't think it's outweighed by the first two (I don't), I'll use prepare_*( array<string,mixed> ): array<valid-shape> and just throw

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #54 (comment).

if ( ! property_exists( $this, $property_name ) ) {
_doing_it_wrong(
Expand Down Expand Up @@ -202,6 +193,76 @@ public function get_meta(): array {
return $this->meta;
}

/**
* Validates the properties used to instantiate the ability.
*
* Errors are thrown as exceptions instead of \WP_Errors to allow for simpler handling and overloading. They are then
* caught and converted to a WP_Error when by WP_Abilities_Registry::register().
*
* @since n.e.x.t
*
* @see WP_Abilities_Registry::register()
*
* @param array<string,mixed> $properties An associative array of properties to validate.
*
* @return void
* @throws \InvalidArgumentException if the properties are invalid.
*
* @phpstan-assert array{
* label: string,
* description: string,
* input_schema?: array<string,mixed>,
* output_schema?: array<string,mixed>,
* execute_callback: callable( array<string,mixed> $input): (mixed|\WP_Error),
* permission_callback?: ?callable( array<string,mixed> $input ): (bool|\WP_Error),
* meta?: array<string,mixed>,
* ...<string, mixed>,
* } $properties
*/
protected function validate_properties( array $properties ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only changes here are that they're now InvalidArgumentExceptions() instead of _doing_it_wrong(). The conditionals and error message are identical to what was in WP_Abilities_API::register()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double-checked with the implementation @felixarntz used in the demo plugin and it seems to be compatible even with the strict checks for madatory properties: label, description and execute callback.

https://github.com/felixarntz/wp-ai-sdk-chatbot-demo/blob/326266fd62fc805ceac0dfd4fe71cd3cc3e7cad3/includes/Abilities/Abstract_Ability.php#L32-L48

if ( empty( $properties['label'] ) || ! is_string( $properties['label'] ) ) {
throw new \InvalidArgumentException(
esc_html__( 'The ability properties must contain a `label` string.' )
);
}

if ( empty( $properties['description'] ) || ! is_string( $properties['description'] ) ) {
throw new \InvalidArgumentException(
esc_html__( 'The ability properties must contain a `description` string.' )
);
}

if ( isset( $properties['input_schema'] ) && ! is_array( $properties['input_schema'] ) ) {
throw new \InvalidArgumentException(
esc_html__( 'The ability properties should provide a valid `input_schema` definition.' )
);
}

if ( isset( $properties['output_schema'] ) && ! is_array( $properties['output_schema'] ) ) {
throw new \InvalidArgumentException(
esc_html__( 'The ability properties should provide a valid `output_schema` definition.' )
);
}

if ( empty( $properties['execute_callback'] ) || ! is_callable( $properties['execute_callback'] ) ) {
throw new \InvalidArgumentException(
esc_html__( 'The ability properties must contain a valid `execute_callback` function.' )
);
}

if ( isset( $properties['permission_callback'] ) && ! is_callable( $properties['permission_callback'] ) ) {
throw new \InvalidArgumentException(
esc_html__( 'The ability properties should provide a valid `permission_callback` function.' )
);
}

if ( isset( $properties['meta'] ) && ! is_array( $properties['meta'] ) ) {
throw new \InvalidArgumentException(
esc_html__( 'The ability properties should provide a valid `meta` array.' )
);
}
}

/**
* Validates input data against the input schema.
*
Expand Down
18 changes: 18 additions & 0 deletions tests/unit/abilities-api/wpAbilitiesRegistry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,4 +383,22 @@ public function test_get_all_registered() {
$this->assertSame( $ability_two_name, $result[ $ability_two_name ]->get_name() );
$this->assertSame( $ability_three_name, $result[ $ability_three_name ]->get_name() );
}

/**
* Direct instantiation of WP_Ability with invalid properties should throw an exception.
*
* @covers WP_Ability::__construct
* @covers WP_Ability::validate_properties
*/
public function test_wp_ability_invalid_properties_throws_exception() {
$this->expectException( \InvalidArgumentException::class );
new WP_Ability(
'test/invalid',
array(
'label' => '',
'description' => '',
'execute_callback' => null,
)
);
}
}