Refactor error handling: Replace exceptions with WP_Error pattern #71
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Replace exception-based error handling with WordPress-native WP_Error pattern in the Core layer components. Changes: - McpAdapter::create_server() now returns self|WP_Error instead of throwing exceptions for validation failures (invalid error handler, observability handler, timing, and duplicate server IDs) - DefaultServerFactory::create() adds is_wp_error() check and logs errors - McpComponentRegistry removes 4 try-catch blocks from register_tools(), add_tool(), register_resources(), and register_prompts() - McpTransportFactory logs errors and continues processing other transports instead of throwing exceptions - Update McpTransportFactoryTest to expect graceful error handling Error codes introduced: - invalid_error_handler - invalid_observability_handler - invalid_timing - duplicate_server_id All 357 tests passing. Related to #24
Convert all domain validators and validate() methods to return WP_Error instead of throwing exceptions, following WordPress conventions. Changes: - All validator methods (validate_tool_data, validate_tool_instance, validate_tool_uniqueness, and equivalents for Resources and Prompts) now return bool|WP_Error instead of void - McpTool::get_ability() returns WP_Ability|WP_Error instead of throwing InvalidArgumentException - McpTool::validate(), McpResource::validate(), and McpPrompt::validate() return self|WP_Error instead of throwing - ToolsHandler::handle_tool_call() adds is_wp_error() check after calling get_ability() - Add assertWPError() and assertNotWPError() helper methods to TestCase - Update 37 validator tests across 3 test files to expect WP_Error instead of exceptions Error codes introduced: - tool_validation_failed, tool_not_unique - resource_validation_failed, resource_not_unique - prompt_validation_failed, prompt_not_unique - ability_not_found All 357 tests passing with 1521 assertions. Related to #24
…ng exceptions Convert all RegisterAbilityAs* classes to return WP_Error instead of throwing exceptions when converting WordPress abilities to MCP components. Changes: - RegisterAbilityAsMcpTool::make() returns McpTool|WP_Error - RegisterAbilityAsMcpResource::make() returns McpResource|WP_Error - RegisterAbilityAsMcpResource::get_uri() returns string|WP_Error instead of throwing InvalidArgumentException - RegisterAbilityAsMcpResource::get_data() propagates WP_Error from get_uri() - RegisterAbilityAsMcpPrompt::make() returns McpPrompt|WP_Error - Remove all @throws annotations from registration classes Error propagation pattern: - Check is_wp_error() at each method in the chain - Return WP_Error immediately if found - Allow from_array() validation errors to bubble up naturally This completes the elimination of all exception throws in the registration layer, with errors now properly propagated as WP_Error objects. All 357 tests passing with 1521 assertions. Related to #24
Add is_wp_error() checks in handlers after get_ability() calls to properly handle errors propagated from the registration and domain layers. Changes: - PromptsHandler::get_prompt() checks is_wp_error() after get_ability() for non-builder prompts - ResourcesHandler::read_resource() checks is_wp_error() after get_ability() - Both handlers log errors and return proper MCP error responses with metadata including error_code and failure_reason - Error responses include 'ability_retrieval_failed' as failure_reason This completes the refactoring to eliminate all exception-based error handling in favor of WordPress's native WP_Error pattern. The codebase now has: - Zero exception throws in production code (7 eliminated) - Zero try-catch blocks for InvalidArgumentException (4 removed) - Consistent WP_Error propagation from Core → Domain → Registration → Handlers All 357 tests passing with 1521 assertions. Fixes #24
Ensure consistency across all MCP component types by making get_ability() return WP_Ability|WP_Error instead of nullable WP_Ability. Changes: - McpResource::get_ability() now returns WP_Ability|WP_Error instead of ?WP_Ability (null) - McpPrompt::get_ability() now returns WP_Ability|WP_Error instead of ?WP_Ability (null) - McpPromptBuilder anonymous class get_ability() returns WP_Error with 'builder_has_no_ability' error code - Update 2 tests to expect WP_Error instead of null from builder prompts This ensures PHPStan type checking passes and all get_ability() methods across the codebase follow the same error handling pattern. All 357 tests passing with 1523 assertions. PHPStan Level 8 analysis passing with no errors. Related to #24
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #71 +/- ##
=============================================
+ Coverage 68.32% 79.74% +11.41%
- Complexity 825 858 +33
=============================================
Files 46 46
Lines 3084 3080 -4
=============================================
+ Hits 2107 2456 +349
+ Misses 977 624 -353
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:
|
…d McpResource files. Update error logging in DefaultServerFactory to escape error messages and codes.
…e cast to string.
Introduces comprehensive unit test suites for the `ToolsHandler` and `ResourcesHandler` to verify their core functionality. These tests cover success paths, error conditions like missing parameters or components not being found, and permission handling. Additionally, enhances the `McpComponentRegistry` tests to ensure that non-string inputs are correctly skipped during registration and that no observability events are recorded when the feature is disabled.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors error handling across the plugin by replacing PHP exceptions with WordPress's native WP_Error pattern, aligning with WordPress core conventions. The changes ensure graceful error handling and better error recovery throughout the plugin.
Key Changes:
- Replaced exception-based error handling with
WP_Errorreturns in all validation, registration, and retrieval methods - Updated handlers to check for
WP_Errorinstances usingis_wp_error()before proceeding with operations - Modified test suite to expect
WP_Errorobjects instead of exceptions
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/Prompts/McpPromptBuilderTest.php | Updated test to verify WP_Error return instead of null for builder-based prompts |
| tests/Unit/Handlers/ToolsHandlerTest.php | New comprehensive test file for ToolsHandler with error scenarios |
| tests/Unit/Handlers/ResourcesHandlerTest.php | New comprehensive test file for ResourcesHandler with error scenarios |
| tests/Unit/Handlers/PromptsHandlerTest.php | Added metadata assertion to success test |
| tests/Unit/Domain/Tools/McpToolValidatorTest.php | Converted exception-based tests to WP_Error assertions |
| tests/Unit/Domain/Resources/McpResourceValidatorTest.php | Converted exception-based tests to WP_Error assertions |
| tests/Unit/Domain/Prompts/McpPromptValidatorTest.php | Converted exception-based tests to WP_Error assertions |
| tests/Unit/Core/McpTransportFactoryTest.php | Updated test to expect graceful error handling instead of exceptions |
| tests/Unit/Core/McpComponentRegistryTest.php | Added tests for error handling and non-string input filtering |
| tests/TestCase.php | Added assertWPError() and assertNotWPError() helper methods |
| tests/Integration/BuilderPromptExecutionTest.php | Updated to verify WP_Error return for builder-based prompts |
| includes/Servers/DefaultServerFactory.php | Added error checking and logging after server creation |
| includes/Handlers/Tools/ToolsHandler.php | Added WP_Error check after get_ability() call |
| includes/Handlers/Resources/ResourcesHandler.php | Added WP_Error check after get_ability() call |
| includes/Handlers/Prompts/PromptsHandler.php | Added WP_Error check after get_ability() call |
| includes/Domain/Tools/RegisterAbilityAsMcpTool.php | Changed return type to support WP_Error |
| includes/Domain/Tools/McpToolValidator.php | Replaced exception throws with WP_Error returns |
| includes/Domain/Tools/McpTool.php | Changed get_ability() to return WP_Error instead of throwing |
| includes/Domain/Resources/RegisterAbilityAsMcpResource.php | Changed return type to support WP_Error and added error propagation |
| includes/Domain/Resources/McpResourceValidator.php | Replaced exception throws with WP_Error returns |
| includes/Domain/Resources/McpResource.php | Changed get_ability() to return WP_Error instead of throwing |
| includes/Domain/Prompts/RegisterAbilityAsMcpPrompt.php | Changed return type to support WP_Error |
| includes/Domain/Prompts/McpPromptValidator.php | Replaced exception throws with WP_Error returns |
| includes/Domain/Prompts/McpPromptBuilder.php | Changed get_ability() to return WP_Error with descriptive message |
| includes/Domain/Prompts/McpPrompt.php | Changed get_ability() to return WP_Error instead of throwing |
| includes/Core/McpTransportFactory.php | Removed exception throw, added error logging and graceful continuation |
| includes/Core/McpComponentRegistry.php | Replaced try-catch blocks with is_wp_error() checks throughout |
| includes/Core/McpAdapter.php | Changed exception throws to WP_Error returns in create_server() |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Implement proper distinction between protocol errors and tool execution errors according to MCP spec (https://modelcontextprotocol.io/specification/2025-06-18/server/tools#error-handling): Protocol errors (JSON-RPC format with 'error' key): - Tool not found - Missing required parameters - Internal errors (ability retrieval failures) Tool execution errors (isError: true format): - Permission denied - Permission check failures - Execution failures - WP_Error from ability execution Changes: - ToolsHandler::call_tool() now distinguishes error types based on failure_reason metadata - Permission denied, permission check failures, and execution errors now return {content: [{type: 'text', text: '...'}], isError: true} - Protocol errors still return JSON-RPC error format - Update 7 tests to expect isError format for tool execution errors - Remove impossible is_wp_error() check (PHPStan was correct) All 357 tests passing with 1527 assertions. PHPStan Level 8 analysis passing with no errors. Fixes #69
Add proper WP_Error detection after ability execution in PromptsHandler and ResourcesHandler to prevent crashes and return graceful error responses.
Updates test expectations for permission-related errors in ToolsHandler to use isError format instead of JSON-RPC error format. Per MCP spec: "Any errors that originate from the tool SHOULD be reported inside the result object, with isError set to true, not as an MCP protocol-level error response. Otherwise, the LLM would not be able to see that an error occurred and self-correct." Permission errors originate from the tool's permission check phase, so they should be returned as tool execution errors (isError: true) rather than protocol-level errors. This allows LLMs to observe and potentially handle permission failures. Changes: - test_call_tool_permission_exception_returns_error: Now expects isError - test_call_tool_permission_denied_returns_error: Now expects isError - Added spec-based comments explaining the error handling approach
Member
|
I'm really curious about this. Is throwing exceptions truly never the solution in WordPress? I searched the WP code, and found there are 166 PHP files that contains I can't find a document on this, so I asked ChatGPT, and it seems to have given sensible criteria. Funnily enough, it actually quotes the Abilities API work for some of its reasoning. Hahah! |
Contributor
Author
@JasonTheAdams I’m usually in the “throw exceptions” camp, but in this codebase we were mostly throwing, catching immediately, and then logging or returning an MCP error. This refactor is a win: using WP_Error aligns with WP conventions, removes boilerplate try/catch (e.g., in McpComponentRegistry/handlers), and gives us consistent error codes while keeping behavior MCP-spec compliant. |
Improves test robustness by adding assertions to ensure abilities are registered before being used in tests. This prevents unexpected errors and provides more informative feedback when abilities are not correctly registered.
Improves the stability of prompt registration by wrapping the prompt builder instantiation and build process in a try-catch block. This prevents a single prompt builder exception from halting the entire component registration process. When an exception occurs, it logs the error and records a failure event, allowing the system to continue registering other components.
Adds a return type declaration to the `get_ability` method for better type hinting.
Enhances developer experience by providing more informative error messages and adding new tests to ensure proper error handling in various scenarios. Adds a mechanism to capture `_doing_it_wrong` calls for more robust testing of developer-facing errors. This allows for assertions that specific functions trigger `_doing_it_wrong` with the expected messages, leading to more reliable detection of incorrect API usage. Specifically, the changes cover: - Creating servers outside of the `mcp_adapter_init` hook. - Using duplicate server IDs. - Using non-existent or invalid transport classes. - Cloning and waking up the Plugin singleton. These improvements help developers identify and fix integration issues more easily, resulting in a more stable and maintainable system.
Ensures that errors during prompt building are caught more broadly by catching `\Throwable` instead of just `\Exception`. This allows the system to handle a wider range of potential issues that might arise when constructing prompts, improving stability and providing more informative error logs.
Ensures abilities are registered within the `wp_abilities_api_init` hook. This is necessary as the WordPress abilities API checks if the hook is currently running. Adds a helper function `register_ability_in_hook` to facilitate registering abilities correctly.
Refactors the way abilities are registered in tests to ensure proper cleanup and prevent potential duplicate registrations when the `wp_abilities_api_init` hook is fired multiple times. It achieves this by using a static function for the callback and registers the ability using the `register_ability_in_hook` method.
Improves the testing framework for ability registration by adding assertions to verify that abilities are correctly registered before use. This change aims to prevent unexpected errors during tests and provides clearer feedback when registration issues occur, contributing to a more reliable testing environment.
JasonTheAdams
approved these changes
Nov 5, 2025
Member
JasonTheAdams
left a comment
JasonTheAdams
left a comment
There was a problem hiding this comment.
Thanks for explaining! LGTM!
Implements validation checks for MCP components (tools and prompts) during registration, ensuring that components meet the required criteria before being added to the system. This change ensures that the validation flag on the McpComponentRegistry is respected by directly calling the validators, and that components are validated during registration and when added individually. This improves the reliability and consistency of the MCP system by preventing the registration of invalid components. It also records component registration failures for observability purposes. Adds the ability to set the MCP server on the McpResource.
Implements additional tests for error scenarios in MCP components, including handling WP_Error and exceptions during ability execution, resource reading, and tool calls. This change ensures that the system correctly identifies and reports errors, improving the reliability of the MCP framework. It also verifies that appropriate error messages and metadata are returned, contributing to better observability and debugging capabilities.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
The plugin was using PHP exceptions for error handling, which contradicts WordPress conventions where
WP_Erroris the standard pattern.What
Refactored all error handling from exceptions to WordPress's native
WP_Errorpattern across the entire plugin.Changes Made
Core Layer (
includes/Core/)McpAdapter::create_server()now returnsself|WP_Error(removed 5 exception throws)McpComponentRegistryremoved 4 try-catch blocks, now usesis_wp_error()checksMcpTransportFactorylogs errors using_doing_it_wrongand continues instead of throwingDomain Layer (
includes/Domain/)get_ability()methods returnWP_Ability|WP_Errorinstead of throwingMcpToolValidator,McpResourceValidator,McpPromptValidator) returnbool|WP_Errorvalidate()methods returnself|WP_ErrorWP_Errorinstead of throwing exceptionsHandlers Layer (
includes/Handlers/)is_wp_error()checks afterget_ability()calls\Throwableto catch unexpected PHP errorsTesting
assertWPError()helper to TestCaseWP_Errorinstead of exceptionsBenefits
Closes #24