diff --git a/includes/abilities-api/class-wp-ability.php b/includes/abilities-api/class-wp-ability.php index 473a2b3b..2f368552 100644 --- a/includes/abilities-api/class-wp-ability.php +++ b/includes/abilities-api/class-wp-ability.php @@ -190,32 +190,17 @@ public function get_meta(): array { * @since 0.1.0 * * @param array $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 ); - 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; } /** @@ -226,11 +211,12 @@ protected function validate_input( array $input = array() ): bool { * @since 0.1.0 * * @param array $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 ) ) { @@ -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 ); } @@ -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; } /** @@ -307,16 +275,23 @@ 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 ); @@ -324,11 +299,9 @@ public function execute( array $input = array() ) { 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; } /** diff --git a/includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php b/includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php index 762ce14e..dcc2d1c7 100644 --- a/includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php +++ b/includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php @@ -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 ); diff --git a/tests/unit/abilities-api/wpRegisterAbility.php b/tests/unit/abilities-api/wpRegisterAbility.php index f1337aba..41fa086e 100644 --- a/tests/unit/abilities-api/wpRegisterAbility.php +++ b/tests/unit/abilities-api/wpRegisterAbility.php @@ -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' ); @@ -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' ); @@ -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() ); } /** diff --git a/tests/unit/rest-api/wpRestAbilitiesRunController.php b/tests/unit/rest-api/wpRestAbilitiesRunController.php index 7bb47f04..82abc6c3 100644 --- a/tests/unit/rest-api/wpRestAbilitiesRunController.php +++ b/tests/unit/rest-api/wpRestAbilitiesRunController.php @@ -10,7 +10,7 @@ class Tests_REST_API_WpRestAbilitiesRunController extends WP_UnitTestCase { /** * REST Server instance. * - * @var WP_REST_Server + * @var \WP_REST_Server */ protected $server; @@ -72,9 +72,11 @@ public function set_up(): void { */ public function tear_down(): void { foreach ( wp_get_abilities() as $ability ) { - if ( str_starts_with( $ability->get_name(), 'test/' ) ) { - wp_unregister_ability( $ability->get_name() ); + if ( ! str_starts_with( $ability->get_name(), 'test/' ) ) { + continue; } + + wp_unregister_ability( $ability->get_name() ); } global $wp_rest_server; @@ -111,10 +113,10 @@ private function register_test_abilities(): void { 'output_schema' => array( 'type' => 'number', ), - 'execute_callback' => function ( array $input ) { + 'execute_callback' => static function ( array $input ) { return $input['a'] + $input['b']; }, - 'permission_callback' => function ( array $input ) { + 'permission_callback' => static function ( array $input ) { return current_user_can( 'edit_posts' ); }, 'meta' => array( @@ -145,18 +147,18 @@ private function register_test_abilities(): void { 'login' => array( 'type' => 'string' ), ), ), - 'execute_callback' => function ( array $input ) { + 'execute_callback' => static function ( array $input ) { $user_id = $input['user_id'] ?? get_current_user_id(); $user = get_user_by( 'id', $user_id ); if ( ! $user ) { - return new WP_Error( 'user_not_found', 'User not found' ); + return new \WP_Error( 'user_not_found', 'User not found' ); } return array( 'id' => $user->ID, 'login' => $user->user_login, ); }, - 'permission_callback' => function ( array $input ) { + 'permission_callback' => static function ( array $input ) { return is_user_logged_in(); }, 'meta' => array( @@ -182,10 +184,10 @@ private function register_test_abilities(): void { 'output_schema' => array( 'type' => 'string', ), - 'execute_callback' => function ( array $input ) { + 'execute_callback' => static function ( array $input ) { return 'Success: ' . $input['data']; }, - 'permission_callback' => function ( array $input ) { + 'permission_callback' => static function ( array $input ) { // Only allow if secret matches return isset( $input['secret'] ) && 'valid_secret' === $input['secret']; }, @@ -201,7 +203,7 @@ private function register_test_abilities(): void { array( 'label' => 'Null Return', 'description' => 'Returns null', - 'execute_callback' => function () { + 'execute_callback' => static function () { return null; }, 'permission_callback' => '__return_true', @@ -217,8 +219,8 @@ private function register_test_abilities(): void { array( 'label' => 'Error Return', 'description' => 'Returns error', - 'execute_callback' => function () { - return new WP_Error( 'test_error', 'This is a test error' ); + 'execute_callback' => static function () { + return new \WP_Error( 'test_error', 'This is a test error' ); }, 'permission_callback' => '__return_true', 'meta' => array( @@ -236,7 +238,7 @@ private function register_test_abilities(): void { 'output_schema' => array( 'type' => 'number', ), - 'execute_callback' => function () { + 'execute_callback' => static function () { return 'not a number'; // Invalid - schema expects number }, 'permission_callback' => '__return_true', @@ -259,7 +261,7 @@ private function register_test_abilities(): void { 'param2' => array( 'type' => 'integer' ), ), ), - 'execute_callback' => function ( array $input ) { + 'execute_callback' => static function ( array $input ) { return $input; }, 'permission_callback' => '__return_true', @@ -322,7 +324,7 @@ public function test_tool_ability_requires_post(): void { array( 'label' => 'Open Tool', 'description' => 'Tool with no permission requirements', - 'execute_callback' => function () { + 'execute_callback' => static function () { return 'success'; }, 'permission_callback' => '__return_true', @@ -363,8 +365,6 @@ public function test_resource_ability_requires_get(): void { * Test output validation against schema. * Note: When output validation fails in WP_Ability::execute(), it returns null, * which causes the REST controller to return 'rest_ability_execution_failed'. - * - * @expectedIncorrectUsage WP_Ability::validate_output */ public function test_output_validation(): void { $request = new WP_REST_Request( 'POST', '/wp/v2/abilities/test/invalid-output/run' ); @@ -376,8 +376,8 @@ public function test_output_validation(): void { $this->assertEquals( 500, $response->get_status() ); $data = $response->get_data(); - $this->assertEquals( 'rest_ability_execution_failed', $data['code'] ); - $this->assertStringContainsString( 'Ability execution failed', $data['message'] ); + $this->assertEquals( 'rest_invalid_type', $data['code'] ); + $this->assertStringContainsString( 'is not of type number.', $data['message'] ); } /** @@ -443,7 +443,7 @@ public function test_contextual_permission_check(): void { } /** - * Test handling of null return from ability. + * Test handling of null is a valid return value. */ public function test_null_return_handling(): void { $request = new WP_REST_Request( 'POST', '/wp/v2/abilities/test/null-return/run' ); @@ -452,10 +452,9 @@ public function test_null_return_handling(): void { $response = $this->server->dispatch( $request ); - $this->assertEquals( 500, $response->get_status() ); + $this->assertEquals( 200, $response->get_status() ); $data = $response->get_data(); - $this->assertEquals( 'rest_ability_execution_failed', $data['code'] ); - $this->assertStringContainsString( 'Ability execution failed', $data['message'] ); + $this->assertNull( $data ); } /** @@ -470,7 +469,7 @@ public function test_wp_error_return_handling(): void { $this->assertEquals( 500, $response->get_status() ); $data = $response->get_data(); - $this->assertEquals( 'rest_ability_execution_failed', $data['code'] ); + $this->assertEquals( 'test_error', $data['code'] ); $this->assertEquals( 'This is a test error', $data['message'] ); } @@ -586,7 +585,6 @@ public function test_post_request_with_non_array_input(): void { /** * Test ability with invalid output that fails validation. - * @expectedIncorrectUsage WP_Ability::validate_output */ public function test_output_validation_failure_returns_error(): void { // Register ability with strict output schema. @@ -605,7 +603,7 @@ public function test_output_validation_failure_returns_error(): void { ), 'required' => array( 'status' ), ), - 'execute_callback' => function ( $input ) { + 'execute_callback' => static function ( $input ) { // Return invalid output that doesn't match schema return array( 'wrong_field' => 'value' ); }, @@ -623,12 +621,11 @@ public function test_output_validation_failure_returns_error(): void { // Should return error when output validation fails $this->assertEquals( 500, $response->get_status() ); $data = $response->get_data(); - $this->assertEquals( 'rest_ability_execution_failed', $data['code'] ); + $this->assertEquals( 'rest_property_required', $data['code'] ); } /** * Test ability with invalid input that fails validation. - * @expectedIncorrectUsage WP_Ability::validate_input */ public function test_input_validation_failure_returns_error(): void { // Register ability with strict input schema. @@ -646,7 +643,7 @@ public function test_input_validation_failure_returns_error(): void { ), 'required' => array( 'required_field' ), ), - 'execute_callback' => function ( $input ) { + 'execute_callback' => static function ( $input ) { return array( 'status' => 'success' ); }, 'permission_callback' => '__return_true', @@ -661,10 +658,10 @@ public function test_input_validation_failure_returns_error(): void { $response = $this->server->dispatch( $request ); - // Should return error when input validation fails (403 due to permission check) - $this->assertEquals( 403, $response->get_status() ); + // Should return error when input validation fails. + $this->assertEquals( 400, $response->get_status() ); $data = $response->get_data(); - $this->assertEquals( 'rest_cannot_execute', $data['code'] ); + $this->assertEquals( 'rest_invalid_param', $data['code'] ); } /** @@ -677,7 +674,7 @@ public function test_ability_without_type_defaults_to_tool(): void { array( 'label' => 'No Type', 'description' => 'Ability without type', - 'execute_callback' => function ( $input ) { + 'execute_callback' => static function ( $input ) { return array( 'executed' => true ); }, 'permission_callback' => '__return_true', @@ -709,7 +706,7 @@ public function test_permission_check_passes_when_callback_not_set(): void { array( 'label' => 'No Permission Callback', 'description' => 'Ability without permission callback', - 'execute_callback' => function ( $input ) { + 'execute_callback' => static function ( $input ) { return array( 'executed' => true ); }, 'meta' => array( 'type' => 'tool' ), @@ -742,7 +739,7 @@ public function test_empty_input_handling(): void { array( 'label' => 'Resource Empty', 'description' => 'Resource with empty input', - 'execute_callback' => function ( $input ) { + 'execute_callback' => static function ( $input ) { return array( 'input_was_empty' => empty( $input ) ); }, 'permission_callback' => '__return_true', @@ -755,7 +752,7 @@ public function test_empty_input_handling(): void { array( 'label' => 'Tool Empty', 'description' => 'Tool with empty input', - 'execute_callback' => function ( $input ) { + 'execute_callback' => static function ( $input ) { return array( 'input_was_empty' => empty( $input ) ); }, 'permission_callback' => '__return_true', @@ -825,7 +822,7 @@ public function test_php_type_strings_in_input(): void { array( 'label' => 'Echo', 'description' => 'Echoes input', - 'execute_callback' => function ( $input ) { + 'execute_callback' => static function ( $input ) { return array( 'echo' => $input ); }, 'permission_callback' => '__return_true', @@ -866,7 +863,7 @@ public function test_mixed_encoding_in_input(): void { array( 'label' => 'Echo Encoding', 'description' => 'Echoes input with encoding', - 'execute_callback' => function ( $input ) { + 'execute_callback' => static function ( $input ) { return array( 'echo' => $input ); }, 'permission_callback' => '__return_true', @@ -926,7 +923,7 @@ public function test_invalid_http_methods( string $method ): void { array( 'label' => 'Method Test', 'description' => 'Test ability for HTTP method validation', - 'execute_callback' => function () { + 'execute_callback' => static function () { return array( 'success' => true ); }, 'permission_callback' => '__return_true', // No permission requirements