Fix $input type hint in WP_Ability

[gziolo]

Closes #58.

The WP_Ability class uses array $input in several places.

While this is likely working for 90% of use-cases, the shape of the input depends on the input schema that is used. That input schema can signify any type that can be expressed in JSON schema - but the PHP array type hint only works for the JSON schema types object and array.

Given that $input is used with two public methods execute, has_permission, and other methods internally, the code in WP_Ability class was adjusted accordingly to use a unified approach. This changes the type hint from array to mixed to support all JSON schema types, including null, boolean, number, integer, string, array, and object.

• Updated all method signatures and documentation to use mixed instead of array for input parameters.
• Made the $input argument optional in execute_callback and permission_callback when the input_schema is not defined and the value is null.
• Updated REST API controller to properly handle mixed input types without assuming array format.
• Add new tests covering all possible input types.
• Modified test callbacks to match the new signatures where appropriate.

Testing

All unit tests should pass:

npm run test:php

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.33%. Comparing base (2796a60) to head (ce5b863).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines	Patch %	Lines
includes/abilities-api/class-wp-ability.php	58.82%	7 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk      #61      +/-   ##
============================================
- Coverage     84.28%   83.33%   -0.95%     
+ Complexity       97       96       -1     
============================================
  Files             8        8              
  Lines           509      516       +7     
============================================
+ Hits            429      430       +1     
- Misses           80       86       +6     

Flag	Coverage Δ
unit	83.33% <66.66%> (-0.95%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[gziolo]

This still needs some additional test coverage to account for the different scenarios for input values passed to execute. It uses internally has_permission, so the only case to ensure it's covered would be when the input is not set at all, and the new default null is automatically set.

[gziolo]

@galatanovidiu, I would appreciate a sanity check in relation to the MCP Adapter. Would the changes proposed introduce the need for some additional handling there, as the input type might become a simple type now? The same question applies to the input schema, but that part I'm less afraid of, unless the input in MCP has some specific requirements.

[galatanovidiu]

Ambiguous handling of null input in ability execution:

Currently, when $input is null in the do_execute() method, it gets passed to the callback as call_user_func($callback, null), meaning the callback receives null as its first argument.
This creates an ambiguity:

Case 1: Developer wants to call a callback with no arguments (e.g., wp_get_current_user())
Case 2: Developer wants to pass null as a meaningful argument to the callback explicitly

Problem scenarios:

Callbacks that expect no parameters (like wp_get_current_user()) will receive an unexpected null parameter
No way to distinguish between "no arguments" vs "null argument"

Proposed solutions:

Use Reflection API to inspect the callback's parameter signature and make intelligent decisions:

If callback expects 0 parameters and input is null: call without arguments: call_user_func($callback)
Otherwise pass input as argument: call_user_func($callback, $input)

[gziolo]

The same problem existed before when this was an array and the default value was []. I agree it would be nice to remove the ambiguity of the null value by not using it as the default value.

It would be a nice enhancement to support some built-in functions, as a possible target for execute_callback. However, it's a more nuanced topic, because do_execute would have to follow the same type mixed ...$args like call_user_func does, while it's currently a single param. It would be convenient for PHP implementers, but this becomes harder to describe with JSON schema and handle through the REST API. What is the approach you had in mind here?

[felixarntz]

Why not make the decision for whether to pass $input based on the schema? I think using reflection for this is unnecessary and less efficient.

Case in point: validate_input() already returns true without checking anything if the input schema is empty.

Similarly, we could implement this as follows: If the input schema is empty, we don't pass $input to any callbacks (execute_callback, permission_callback).

• Makes sense because if there's no input schema, you don't expect any input.
• If you at the same time pass a callback that expects an input, you're doing it wrong.

[gziolo]

The idea shared by @felixarntz that addresses the concerns should be fully implemented in 1935799. I still need to increase test coverage.

[gziolo]

In e117cac added tests that cover several types of input:

• boolean
• string
• number
• array

Still to cover:

• null
• object
• integer (the current test for number should probably use integer)

[gziolo]

@felixarntz, this is in a shape where it could use some feedback, so we can align if this is going in the right direction.

[felixarntz]

This looks good regarding getting rid of the array requirement. I left some thoughts regarding the handling of expecting no input / no parameters.

[Copilot]

Pull Request Overview

Fixes type hints for the $input parameter in the WP_Ability class and related methods, changing from array to mixed to support all JSON schema types including boolean, number, string, array, and object.

• Updated all method signatures and documentation to use mixed instead of array for input parameters
• Modified test callbacks to match the new signatures where appropriate
• Updated REST API controller to properly handle mixed input types without assuming array format

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
includes/abilities-api/class-wp-ability.php	Core changes to method signatures and documentation for execute, has_permission, and validation methods
includes/abilities-api/class-wp-abilities-registry.php	Updated PHPDoc type annotations for callback parameters
includes/abilities-api.php	Updated PHPDoc type annotations for wp_register_ability function
includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php	Modified input handling to support mixed types and updated documentation
tests/unit/abilities-api/wpAbility.php	Added comprehensive test coverage for different input types (boolean, number, string, array)
tests/unit/abilities-api/wpRegisterAbility.php	Updated mock ability class signature
tests/unit/rest-api/wpRestAbilitiesRunController.php	Updated test callback signatures to match new mixed input type

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: gziolo <[email protected]>
Co-authored-by: galatanovidiu <[email protected]>
Co-authored-by: felixarntz <[email protected]>
Co-authored-by: justlevine <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[gziolo]

It's ready for the final review. I addressed the feedback, increased test coverage, and updated existing documentation.

[felixarntz]

@gziolo The type hint fix here LGTM, but I am realizing that there's IMO a security flaw in how the validation of the input is handled. It's not introduced by this PR, but it would be critical to fix, unless I'm missing something.

cc @justlevine

[felixarntz]

Giving this another thought, should we really do it like that? I think it's problematic from a security standpoint:

• The input_schema is critical in validating that the input is expected, including from a security perspective.
• It is already required, which makes sense.
• The only reasonable explanation for providing an empty $input_schema is that the ability does not expect any input. (It makes sense that it's required regardless, to make that "opt out" explicit.) But technically, it's still possible to pass $input to the ability.
• Problem: This code interprets the lack of $input_schema as "let's not validate" - I think this is a security concern. Instead, it should interpret the lack of $input_schema as: If there is any input (i.e. $input !== null), this is invalid and it should return a WP_Error.

[gziolo]

I'm open to hardening this one. I wasn't sure what's the best way to go about it, so all it worth taking into consideration. It means it will become very strict in the sense that the only case when we allow not passing the input schema is when there is no input at all. Otherwise, even when passing null as value, we would have to validatr with input schema provided.

[felixarntz]

To me passing null as $input is the same as not passing any $input at all. If the developer provided an input schema that was only allowing null, it would be pointless to do so anyway because it doesn't have any meaning. And if the value null is being passed in a situation where passing another value is possible, that would only work in combination with providing a (oneOf) schema anyway.

So to clarify what I meant: If there's no input schema, it's fine as long as $input === null. Whether it was passed as null or whether it's just the null default shouldn't matter.

[gziolo]

So to clarify what I meant: If there's no input schema, it's fine as long as $input === null. Whether it was passed as null or whether it's just the null default shouldn't matter.

I agree with the rationale. I committed your proposed changes, and I will update the rest of the codebase accordingly to make it work this way.

[gziolo]

c377af9 should do the trick.

I will look into failing unit tests tomorrow as REST API controllers somehow depend on the old behavior.

[justlevine]

This changes the type hint from array to mixed to support all JSON schema types, including null, boolean, number, integer, string, array, and object.

Seeing the diff reinforces my concern about how PHP's mixed is broader than "all JSON schema types".

( I'm not sure why we want to allow for non-named arguments here, and still think that we should use WP_REST_Controller as the prior art here where only named inputs ('args': array<string,mixed>|null ) are supported. I cannot think of a justifiable DX we're we (humans, tools, agents, implementations) would prefer to be fully-dependent on also having the WP_Ability::$input_schema to understand the shape, instead of inferring/juggling an array key. ( e.g. get_post( $input ) is always going to less semantic than get_post( $input['user_id'] )` )

If we are going to allow more than named argments , then we can at least be accurate about the PHP type, and use null|bool|int|float|string|mixed[]|object, instead of littering the codebase with an avoidable mixed.

Last time I bring all that up I promise 🤐)

---

Regarding the input schema validation, agree that semantically it should be if ( null === $input_schema ) { return true; } while if ( empty( $input_schema ) ) { return new \WP_Error()

A few search-replaces ( $input= => $input ) and (nevermind ) LGTM

[gziolo]

@justlevine, so you suggest we replace mixed with the explicit list of the allowed types that the REST API can handle? The list of primitive types is here, which should align with the unit tests I added. The type would be defined on the REST API schema as:

(integer|number|boolean|string|array|object|null)

or more precisely

'type' => [ 'integer', 'number', 'boolean', 'string', 'array', 'object', 'null' ]

The order might be wrong as it matters here because of Type Juggling. Based on your feedback, we would also need to make the object type more structured. Although I see that it needs to implement JsonSerializable, this implies that it must contain keys that are strings.

[gziolo]

I added specific types in the REST API schema in a21105f. @justlevine, what else would you like to have included? I may not fully understand your expectations. This looks good enough for me at this point.

[felixarntz]

@gziolo This looks great to me, thank you for the updates!

Just one minor note that I think would be worth updating before merge.

[gziolo]

what else would you like to have included? I may not fully understand your expectations. This looks good enough for me at this point.

@justlevine, I'm still happy to discuss and iterate when you have a chance to respond. Now, I will land this PR as it addresses a larger issue.