Skip to content

Commit fb95300

Browse files
justlevinegziolo
justlevine
and
gziolo
authored
dev!: return WP_Error from WP_Ability methods (#23)
* dev!: return `\WP_Error` from `WP_Ability` methods * docs: fix return on validate_output * Update class-wp-rest-abilities-run-controller.php * Update wpRestAbilitiesRunController.php --------- Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
1 parent e61dab5 commit fb95300

File tree

4 files changed

+115
-149
lines changed

4 files changed

+115
-149
lines changed

‎includes/abilities-api/class-wp-ability.php‎

Lines changed: 34 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -190,32 +190,17 @@ public function get_meta(): array {
190190
* @since 0.1.0
191191
*
192192
* @param array<string,mixed> $input Optional. The input data to validate.
193-
* @return bool Returns true if valid, false if validation fails.
193+
* @return true|\WP_Error Returns true if valid or the WP_Error object if validation fails.
194194
*/
195-
protected function validate_input( array $input = array() ): bool {
195+
protected function validate_input( array $input = array() ) {
196196
$input_schema = $this->get_input_schema();
197197
if ( empty( $input_schema ) ) {
198198
return true;
199199
}
200200

201201
$valid_input = rest_validate_value_from_schema( $input, $input_schema );
202-
if ( is_wp_error( $valid_input ) ) {
203-
_doing_it_wrong(
204-
__METHOD__,
205-
esc_html(
206-
sprintf(
207-
/* translators: %1$s ability name, %2$s error message. */
208-
__( 'Invalid input provided for ability "%1$s": %2$s.' ),
209-
$this->name,
210-
$valid_input->get_error_message()
211-
)
212-
),
213-
'0.1.0'
214-
);
215-
return false;
216-
}
217202

218-
return true;
203+
return is_wp_error( $valid_input ) ? $valid_input : true;
219204
}
220205

221206
/**
@@ -226,11 +211,12 @@ protected function validate_input( array $input = array() ): bool {
226211
* @since 0.1.0
227212
*
228213
* @param array<string,mixed> $input Optional. The input data for permission checking.
229-
* @return bool Whether the ability has the necessary permission.
214+
* @return true|\WP_Error Whether the ability has the necessary permission.
230215
*/
231-
public function has_permission( array $input = array() ): bool {
232-
if ( ! $this->validate_input( $input ) ) {
233-
return false;
216+
public function has_permission( array $input = array() ) {
217+
$is_valid = $this->validate_input( $input );
218+
if ( is_wp_error( $is_valid ) ) {
219+
return $is_valid;
234220
}
235221

236222
if ( ! is_callable( $this->permission_callback ) ) {
@@ -250,16 +236,13 @@ public function has_permission( array $input = array() ): bool {
250236
*/
251237
protected function do_execute( array $input ) {
252238
if ( ! is_callable( $this->execute_callback ) ) {
253-
_doing_it_wrong(
254-
__METHOD__,
255-
esc_html(
256-
/* translators: %s ability name. */
257-
sprintf( __( 'Ability "%s" does not have a valid execute callback.' ), $this->name )
258-
),
259-
'0.1.0'
239+
return new \WP_Error(
240+
'ability_invalid_execute_callback',
241+
/* translators: %s ability name. */
242+
sprintf( __( 'Ability "%s" does not have a valid execute callback.' ), $this->name )
260243
);
261-
return null;
262244
}
245+
263246
return call_user_func( $this->execute_callback, $input );
264247
}
265248

@@ -269,32 +252,17 @@ protected function do_execute( array $input ) {
269252
* @since 0.1.0
270253
*
271254
* @param mixed $output The output data to validate.
272-
* @return bool Returns true if valid, false if validation fails.
255+
* @return true|\WP_Error Returns true if valid, or a WP_Error object if validation fails.
273256
*/
274-
protected function validate_output( $output ): bool {
257+
protected function validate_output( $output ) {
275258
$output_schema = $this->get_output_schema();
276259
if ( empty( $output_schema ) ) {
277260
return true;
278261
}
279262

280263
$valid_output = rest_validate_value_from_schema( $output, $output_schema );
281-
if ( is_wp_error( $valid_output ) ) {
282-
_doing_it_wrong(
283-
__METHOD__,
284-
esc_html(
285-
sprintf(
286-
/* translators: %1$s ability name, %2$s error message. */
287-
__( 'Invalid output provided for ability "%1$s": %2$s.' ),
288-
$this->name,
289-
$valid_output->get_error_message()
290-
)
291-
),
292-
'0.1.0'
293-
);
294-
return false;
295-
}
296264

297-
return true;
265+
return is_wp_error( $valid_output ) ? $valid_output : true;
298266
}
299267

300268
/**
@@ -307,28 +275,33 @@ protected function validate_output( $output ): bool {
307275
* @return mixed|\WP_Error The result of the ability execution, or WP_Error on failure.
308276
*/
309277
public function execute( array $input = array() ) {
310-
if ( ! $this->has_permission( $input ) ) {
311-
_doing_it_wrong(
312-
__METHOD__,
313-
esc_html(
314-
/* translators: %s ability name. */
315-
sprintf( __( 'Ability "%s" does not have necessary permission.' ), $this->name )
316-
),
317-
'0.1.0'
278+
$has_permissions = $this->has_permission( $input );
279+
280+
if ( true !== $has_permissions ) {
281+
if ( is_wp_error( $has_permissions ) ) {
282+
// Don't leak the error to someone without the correct perms.
283+
_doing_it_wrong(
284+
__METHOD__,
285+
esc_html( $has_permissions->get_error_message() ),
286+
'0.1.0'
287+
);
288+
}
289+
290+
return new \WP_Error(
291+
'ability_invalid_permissions',
292+
/* translators: %s ability name. */
293+
sprintf( __( 'Ability "%s" does not have necessary permission.' ), $this->name )
318294
);
319-
return null;
320295
}
321296

322297
$result = $this->do_execute( $input );
323298
if ( is_wp_error( $result ) ) {
324299
return $result;
325300
}
326301

327-
if ( ! $this->validate_output( $result ) ) {
328-
return null;
329-
}
302+
$is_valid = $this->validate_output( $result );
330303

331-
return $result;
304+
return is_wp_error( $is_valid ) ? $is_valid : $result;
332305
}
333306

334307
/**

‎includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -146,19 +146,7 @@ public function run_ability( $request ) {
146146
$result = $ability->execute( $input );
147147

148148
if ( is_wp_error( $result ) ) {
149-
return new \WP_Error(
150-
'rest_ability_execution_failed',
151-
$result->get_error_message(),
152-
array( 'status' => 500 )
153-
);
154-
}
155-
156-
if ( is_null( $result ) ) {
157-
return new \WP_Error(
158-
'rest_ability_execution_failed',
159-
__( 'Ability execution failed. Please check permissions and input parameters.' ),
160-
array( 'status' => 500 )
161-
);
149+
return $result;
162150
}
163151

164152
$output_validation = $this->validate_output( $ability, $result );

‎tests/unit/abilities-api/wpRegisterAbility.php‎

Lines changed: 43 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,6 @@ public function test_register_valid_ability(): void {
143143

144144
/**
145145
* Tests executing an ability with no permissions.
146-
*
147-
* @expectedIncorrectUsage WP_Ability::execute
148146
*/
149147
public function test_register_ability_no_permissions(): void {
150148
do_action( 'abilities_api_init' );
@@ -162,42 +160,47 @@ public function test_register_ability_no_permissions(): void {
162160
)
163161
)
164162
);
165-
$this->assertNull(
166-
$result->execute(
167-
array(
168-
'a' => 2,
169-
'b' => 3,
170-
)
163+
164+
$actual = $result->execute(
165+
array(
166+
'a' => 2,
167+
'b' => 3,
171168
)
172169
);
170+
$this->assertWPError(
171+
$actual,
172+
'Execution should fail due to no permissions'
173+
);
174+
$this->assertEquals( 'ability_invalid_permissions', $actual->get_error_code() );
173175
}
174176

175177
/**
176178
* Tests executing an ability with input not matching schema.
177-
*
178-
* @expectedIncorrectUsage WP_Ability::validate_input
179-
* @expectedIncorrectUsage WP_Ability::execute
180179
*/
181180
public function test_execute_ability_no_input_schema_match(): void {
182181
do_action( 'abilities_api_init' );
183182

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

186-
$this->assertNull(
187-
$result->execute(
188-
array(
189-
'a' => 2,
190-
'b' => 3,
191-
'unknown' => 1,
192-
)
185+
$this->setExpectedIncorrectUsage( 'WP_Ability::execute' );
186+
187+
$actual = $result->execute(
188+
array(
189+
'a' => 2,
190+
'b' => 3,
191+
'unknown' => 1,
193192
)
194193
);
194+
195+
$this->assertWPError(
196+
$actual,
197+
'Execution should fail due to input not matching schema'
198+
);
199+
$this->assertEquals( 'ability_invalid_permissions', $actual->get_error_code() );
195200
}
196201

197202
/**
198203
* Tests executing an ability with output not matching schema.
199-
*
200-
* @expectedIncorrectUsage WP_Ability::validate_output
201204
*/
202205
public function test_execute_ability_no_output_schema_match(): void {
203206
do_action( 'abilities_api_init' );
@@ -207,35 +210,40 @@ public function test_execute_ability_no_output_schema_match(): void {
207210
};
208211
$result = wp_register_ability( self::$test_ability_name, self::$test_ability_properties );
209212

210-
$this->assertNull(
211-
$result->execute(
212-
array(
213-
'a' => 2,
214-
'b' => 3,
215-
)
213+
$actual = $result->execute(
214+
array(
215+
'a' => 2,
216+
'b' => 3,
216217
)
217218
);
219+
$this->assertWPError(
220+
$actual,
221+
'Execution should fail due to output not matching schema',
222+
);
223+
$this->assertEquals( 'rest_invalid_type', $actual->get_error_code() );
218224
}
219225

220226
/**
221227
* Tests permission callback receiving input not matching schema.
222-
*
223-
* @expectedIncorrectUsage WP_Ability::validate_input
224228
*/
225229
public function test_permission_callback_no_input_schema_match(): void {
226230
do_action( 'abilities_api_init' );
227231

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

230-
$this->assertFalse(
231-
$result->has_permission(
232-
array(
233-
'a' => 2,
234-
'b' => 3,
235-
'unknown' => 1,
236-
)
234+
$actual = $result->has_permission(
235+
array(
236+
'a' => 2,
237+
'b' => 3,
238+
'unknown' => 1,
237239
)
238240
);
241+
242+
$this->assertWPError(
243+
$actual,
244+
'Permission check should fail due to input not matching schema'
245+
);
246+
$this->assertEquals( 'rest_additional_properties_forbidden', $actual->get_error_code() );
239247
}
240248

241249
/**

0 commit comments

Comments
 (0)