Skip to content

dev!: return WP_Error from WP_Ability methods #23

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
gziolo merged 5 commits into from
Aug 19, 2025
Merged

dev!: return WP_Error from WP_Ability methods #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a3637cc
dev!: return `\WP_Error` from `WP_Ability` methods
justlevine Aug 16, 2025
751a3b6
docs: fix return on validate_output
justlevine Aug 16, 2025
b46d6b6
Merge branch 'trunk' into dev/wp_ability-return-error
gziolo Aug 19, 2025
f515cc9
Update class-wp-rest-abilities-run-controller.php
gziolo Aug 19, 2025
3885a9e
Update wpRestAbilitiesRunController.php
gziolo Aug 19, 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
95 changes: 34 additions & 61 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 @@ -190,32 +190,17 @@ public function get_meta(): array {
* @since 0.1.0
*
* @param array<string,mixed> $input Optional. The input data to validate.
* @return bool Returns true if valid, false if validation fails.
* @return true|\WP_Error Returns true if valid or the WP_Error object if validation fails.
*/
protected function validate_input( array $input = array() ): bool {
protected function validate_input( array $input = array() ) {
$input_schema = $this->get_input_schema();
if ( empty( $input_schema ) ) {
return true;
}

$valid_input = rest_validate_value_from_schema( $input, $input_schema );
Copy link
Member

@gziolo gziolo Aug 18, 2025

Choose a reason for hiding this comment

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

It looks like it could return the value here because rest_validate_value_from_schema returns true or WP_Error.

Copy link
Member

@gziolo gziolo Aug 18, 2025

Choose a reason for hiding this comment

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

Thinking a bit more about it after seeing the updated unit tests, we should remap these WP_Errors so the names are more tied to the abilty, for example:

rest_invalid_type -> ability_input_invalid_type

Could we replace the prefix?

if ( is_wp_error( $valid_input ) ) {
_doing_it_wrong(
__METHOD__,
esc_html(
sprintf(
/* translators: %1$s ability name, %2$s error message. */
__( 'Invalid input provided for ability "%1$s": %2$s.' ),
$this->name,
$valid_input->get_error_message()
)
),
'0.1.0'
);
return false;
}

return true;
return is_wp_error( $valid_input ) ? $valid_input : true;
}

/**
Expand All @@ -226,11 +211,12 @@ protected function validate_input( array $input = array() ): bool {
* @since 0.1.0
*
* @param array<string,mixed> $input Optional. The input data for permission checking.
* @return bool Whether the ability has the necessary permission.
* @return true|\WP_Error Whether the ability has the necessary permission.
*/
public function has_permission( array $input = array() ): bool {
if ( ! $this->validate_input( $input ) ) {
return false;
public function has_permission( array $input = array() ) {
$is_valid = $this->validate_input( $input );
if ( is_wp_error( $is_valid ) ) {
return $is_valid;
}

if ( ! is_callable( $this->permission_callback ) ) {
Expand All @@ -250,16 +236,13 @@ public function has_permission( array $input = array() ): bool {
*/
protected function do_execute( array $input ) {
if ( ! is_callable( $this->execute_callback ) ) {
_doing_it_wrong(
__METHOD__,
esc_html(
/* translators: %s ability name. */
sprintf( __( 'Ability "%s" does not have a valid execute callback.' ), $this->name )
),
'0.1.0'
return new \WP_Error(
'ability_invalid_execute_callback',
/* translators: %s ability name. */
sprintf( __( 'Ability "%s" does not have a valid execute callback.' ), $this->name )
);
return null;
}

return call_user_func( $this->execute_callback, $input );
}

Expand All @@ -269,32 +252,17 @@ protected function do_execute( array $input ) {
* @since 0.1.0
*
* @param mixed $output The output data to validate.
* @return bool Returns true if valid, false if validation fails.
* @return true|\WP_Error Returns true if valid, or a WP_Error object if validation fails.
*/
protected function validate_output( $output ): bool {
protected function validate_output( $output ) {
$output_schema = $this->get_output_schema();
if ( empty( $output_schema ) ) {
return true;
}

$valid_output = rest_validate_value_from_schema( $output, $output_schema );
if ( is_wp_error( $valid_output ) ) {
_doing_it_wrong(
__METHOD__,
esc_html(
sprintf(
/* translators: %1$s ability name, %2$s error message. */
__( 'Invalid output provided for ability "%1$s": %2$s.' ),
$this->name,
$valid_output->get_error_message()
)
),
'0.1.0'
);
return false;
}

return true;
return is_wp_error( $valid_output ) ? $valid_output : true;
Copy link
Member

@gziolo gziolo Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Similar feedback as for input handling since we use rest_validate_value_from_schema which happens to be tied to much to REST API at this level.

}

/**
Expand All @@ -307,28 +275,33 @@ protected function validate_output( $output ): bool {
* @return mixed|\WP_Error The result of the ability execution, or WP_Error on failure.
*/
public function execute( array $input = array() ) {
if ( ! $this->has_permission( $input ) ) {
_doing_it_wrong(
__METHOD__,
esc_html(
/* translators: %s ability name. */
sprintf( __( 'Ability "%s" does not have necessary permission.' ), $this->name )
),
'0.1.0'
$has_permissions = $this->has_permission( $input );

if ( true !== $has_permissions ) {
if ( is_wp_error( $has_permissions ) ) {
// Don't leak the error to someone without the correct perms.
_doing_it_wrong(
__METHOD__,
esc_html( $has_permissions->get_error_message() ),
'0.1.0'
);
}

return new \WP_Error(
'ability_invalid_permissions',
/* translators: %s ability name. */
sprintf( __( 'Ability "%s" does not have necessary permission.' ), $this->name )
);
return null;
}

$result = $this->do_execute( $input );
if ( is_wp_error( $result ) ) {
return $result;
}

if ( ! $this->validate_output( $result ) ) {
return null;
}
$is_valid = $this->validate_output( $result );

return $result;
return is_wp_error( $is_valid ) ? $is_valid : $result;
}

/**
Expand Down
14 changes: 1 addition & 13 deletions includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,19 +146,7 @@ public function run_ability( $request ) {
$result = $ability->execute( $input );

if ( is_wp_error( $result ) ) {
return new \WP_Error(
'rest_ability_execution_failed',
$result->get_error_message(),
array( 'status' => 500 )
);
}

if ( is_null( $result ) ) {
return new \WP_Error(
'rest_ability_execution_failed',
__( 'Ability execution failed. Please check permissions and input parameters.' ),
array( 'status' => 500 )
);
return $result;
}

$output_validation = $this->validate_output( $ability, $result );
Copy link
Member

@gziolo gziolo Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Both $this->validate_input() and $this->validate_output() calls should no longer be needed as they are handled internally inside $ability->execute() with similar WP_Errors. We would need to differentiate mostly the error names to the proper status code in the response: 400 vs 500.

Expand Down
78 changes: 43 additions & 35 deletions tests/unit/abilities-api/wpRegisterAbility.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,6 @@ public function test_register_valid_ability(): void {

/**
* Tests executing an ability with no permissions.
*
* @expectedIncorrectUsage WP_Ability::execute
*/
public function test_register_ability_no_permissions(): void {
do_action( 'abilities_api_init' );
Expand All @@ -162,42 +160,47 @@ public function test_register_ability_no_permissions(): void {
)
)
);
$this->assertNull(
$result->execute(
array(
'a' => 2,
'b' => 3,
)

$actual = $result->execute(
array(
'a' => 2,
'b' => 3,
)
);
$this->assertWPError(
$actual,
'Execution should fail due to no permissions'
);
$this->assertEquals( 'ability_invalid_permissions', $actual->get_error_code() );
}

/**
* Tests executing an ability with input not matching schema.
*
* @expectedIncorrectUsage WP_Ability::validate_input
* @expectedIncorrectUsage WP_Ability::execute
*/
public function test_execute_ability_no_input_schema_match(): void {
do_action( 'abilities_api_init' );

$result = wp_register_ability( self::$test_ability_name, self::$test_ability_properties );

$this->assertNull(
$result->execute(
array(
'a' => 2,
'b' => 3,
'unknown' => 1,
)
$this->setExpectedIncorrectUsage( 'WP_Ability::execute' );

$actual = $result->execute(
array(
'a' => 2,
'b' => 3,
'unknown' => 1,
)
);

$this->assertWPError(
$actual,
'Execution should fail due to input not matching schema'
);
$this->assertEquals( 'ability_invalid_permissions', $actual->get_error_code() );
}

/**
* Tests executing an ability with output not matching schema.
*
* @expectedIncorrectUsage WP_Ability::validate_output
*/
public function test_execute_ability_no_output_schema_match(): void {
do_action( 'abilities_api_init' );
Expand All @@ -207,35 +210,40 @@ public function test_execute_ability_no_output_schema_match(): void {
};
$result = wp_register_ability( self::$test_ability_name, self::$test_ability_properties );

$this->assertNull(
$result->execute(
array(
'a' => 2,
'b' => 3,
)
$actual = $result->execute(
array(
'a' => 2,
'b' => 3,
)
);
$this->assertWPError(
$actual,
'Execution should fail due to output not matching schema',
);
$this->assertEquals( 'rest_invalid_type', $actual->get_error_code() );
}

/**
* Tests permission callback receiving input not matching schema.
*
* @expectedIncorrectUsage WP_Ability::validate_input
*/
public function test_permission_callback_no_input_schema_match(): void {
do_action( 'abilities_api_init' );

$result = wp_register_ability( self::$test_ability_name, self::$test_ability_properties );

$this->assertFalse(
$result->has_permission(
array(
'a' => 2,
'b' => 3,
'unknown' => 1,
)
$actual = $result->has_permission(
array(
'a' => 2,
'b' => 3,
'unknown' => 1,
)
);

$this->assertWPError(
$actual,
'Permission check should fail due to input not matching schema'
);
$this->assertEquals( 'rest_additional_properties_forbidden', $actual->get_error_code() );
}

/**
Expand Down
Loading