Add REST API controllers for Abilities API#6
Conversation
|
I landed #3, which triggered GitHub automations, and accidentally closed this PR after the target branch got deleted 😅 I'll try to fix it. |
|
Reopened and changed the base branch, but it will require a rebase to remove my commits from the list of changes. |
There was a problem hiding this comment.
I reviewed all the code and tests for the list controller and left my feedback inline. I still need to check the run controller. Everything is looking great, my feedback is mostly about minor things or I had some questions/notes where I wasn't entirely sure how it works.
My only high-level, important feedback is around using a single form: id or name to uniquely identify the ability.
There was a problem hiding this comment.
Pull Request Overview
This PR implements REST API controllers for the Abilities API, enabling web-based access to both resource and tool type abilities through standardized endpoints.
Key Changes:
- Added two REST API controllers (
WP_REST_Abilities_List_ControllerandWP_REST_Abilities_Run_Controller) with full test coverage - Implemented HTTP method validation where resource abilities use GET and tool abilities use POST
- Added comprehensive validation for input/output schemas and permission checks
Reviewed Changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/rest/class-wp-rest-abilities-list-controller.php |
Implements endpoints for listing and retrieving individual abilities |
src/rest/class-wp-rest-abilities-run-controller.php |
Handles ability execution with method validation and comprehensive error handling |
src/rest/class-wp-rest-abilities-init.php |
Initializes and registers REST API routes on the rest_api_init hook |
tests/unit/REST/WPRESTAbilitiesListControllerTest.php |
Comprehensive test coverage for list controller including pagination and permissions |
tests/unit/REST/WPRESTAbilitiesRunControllerTest.php |
Extensive test coverage for run controller including method validation and error handling |
| Various test and configuration files | Supporting infrastructure for unit testing and development |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Fixed REST controller calling non-existent get_execute_callback() method. Always show run link for abilities in REST responses.
- Register run controller before list controller to fix route precedence - Support both GET (resources) and POST (tools) methods based on ability type - Add TODO comment explaining load order issue with ALLMETHODS - Remove result wrapper to match Feature API response format - Handle query params for GET requests and JSON body for POST
f98cb4c to
a15939d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #6 +/- ##
========================================
Coverage ? 90.32%
Complexity ? 103
========================================
Files ? 7
Lines ? 558
Branches ? 0
========================================
Hits ? 504
Misses ? 54
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PHPStan fixes: - Add return type annotations for void methods - Add array type hints for array parameters and returns - Cast integer header values to strings (PHPStan stubs expect strings) - Add ignore rule for WP_REST_Request generic types with explanation PHPCS compliance: - Add file documentation headers to all REST API files - Use fully qualified class names in PHPDoc comments - Add 'abilities-api' domain to all translation functions - Fix array alignment and spacing issues - Fix short ternary operators to explicit conditionals - Add periods to inline comments - Simplify elseif logic in run controller Type hints and modern PHP: - Add native PHP type hints where compatible with PHP 7.4+ - Add WP_REST_Abilities prefix to allowed prefixes in PHPCS config - Use native return types (void, array, bool) and parameter types
e1ac3e8 to
1af315d
Compare
- Add array<string, mixed> type hint for get_input_from_request return - Add public visibility to DEFAULT_PER_PAGE constant
9008781 to
db79c0d
Compare
db79c0d to
0180c4a
Compare
|
I've addressed the static analysis and PHPCS issues from this branch. The remaining ones seem to be on |
- Add test coverage for REST API controllers (List, Run, Init) - Add tests for uncovered lines in run controller (29 lines) - Add tests for init class route registration - Add malformed input tests for edge cases - Remove PHP type hints from REST controllers for WordPress compatibility - Fix test expectations to match WordPress REST API behavior - Use PHPUnit data providers for better test organization - Fix HTTP method validation tests - Add missing @return documentation tag Test coverage improvements: - WPRESTAbilitiesRunControllerTest: Full coverage with input validation, permissions, HTTP methods - WPRESTAbilitiesListControllerTest: Pagination, special characters, invalid parameters - WPRESTAbilitiesInitTest: Route registration and controller instantiation All tests passing (105 tests, 246 assertions)
0fe00d5 to
9b071cb
Compare
- WordPress core's WP_REST_Controller doesn't have type hints on methods - Adding type hints causes fatal errors due to signature mismatch - Exclude Squiz.Commenting.FunctionComment.TypeHintMissing for REST controllers - Also exclude Slevomat type hint rules for consistency - This is a common WordPress compatibility issue
| * @return \WP_REST_Response|\WP_Error Response object on success, or WP_Error object on failure. | ||
| */ | ||
| public function get_item( $request ) { | ||
| $ability = wp_get_ability( $request['name'] ); |
There was a problem hiding this comment.
Is there a reason not to use WP_REST_Request->get_param() (/ ->get_params() elsewhere )?
We lose the rest_request_parameter_order filter by relying on \ArrayAccess
There was a problem hiding this comment.
I see both forms used in REST API controllers included in WP core, sometimes even in the same handler 😅
There was a problem hiding this comment.
@gziolo I think that's a sign of tech debt, not an endorsement of pattern 😅
There was a problem hiding this comment.
We can update to use get_param() 👍🏻
Will you take care of it?
gziolo
left a comment
There was a problem hiding this comment.
Thank you for rebasing with trunk and addressing all feedback. I see the increased unit tests coverage added in 9b071cb 👏🏻
Let's land it in the current shape and address the code quality configuration challenges seperately. I also plan to open follow-up issues for TODO items included in the codebase.
Closes #2.
This PR adds initial REST API endpoints to the Abilities API with support for both resource (GET) and tool (POST) type abilities.
How
Implements the following REST API endpoints:
WP_REST_Abilities_List_Controller- Handles listing abilities and retrieving individual abilitiesGET /wp-json/wp/v2/abilities- List all registered abilitiesGET /wp-json/wp/v2/abilities/{id}- Get a specific ability by IDWP_REST_Abilities_Run_Controller- Handles executing abilitiesGET/POST /wp-json/wp/v2/abilities/{id}/run- Execute an ability (method depends on ability type)GETPOSTTesting Steps
If you enable the Ability API as a plugin:
GET /wp/v2/abilitiesGET /wp/v2/abilities/{id}GET /wp/v2/abilities/{id}/runPOST /wp/v2/abilities/{id}/run