Skip to content

Encapsulation and type safety improvements#154

Open
galatanovidiu wants to merge 6 commits intotrunkfrom
fix/encapsulation-and-type-safety
Open

Encapsulation and type safety improvements#154
galatanovidiu wants to merge 6 commits intotrunkfrom
fix/encapsulation-and-type-safety

Conversation

@galatanovidiu
Copy link
Copy Markdown
Contributor

Why

The v0.5.0 code review identified five internal structure issues that weaken type safety and encapsulation without serving any API purpose:

  1. RegisterAbilityAsMcpPrompt.php is the only file missing declare(strict_types=1), allowing silent type coercion in a domain layer class.

  2. McpTransportContext uses dynamic property assignment ($this->$name = $value in a foreach loop), which silently accepts typos, creates dynamic properties on PHP 8.2+, and bypasses the type system entirely.

  3. McpServer::$error_handler and ::$observability_handler are public despite having getter methods. The properties are not part of the frozen public API (the getters are), but ~15 internal call sites access them directly — inconsistent and mutable from outside.

  4. HttpRequestHandler::$transport_context is public with no getter, forcing HttpTransport to reach into the handler's internals via $this->request_handler->transport_context.

  5. PromptsHandler static properties lack typed declarations, relying on PHPDoc @var annotations instead of PHP's native type system.

None of these touch the frozen public API surface. All changes are internal to the plugin.

What

Type safety (RegisterAbilityAsMcpPrompt, PromptsHandler):

  • Added declare(strict_types=1) to the only file missing it
  • Added explicit array and string type declarations to PromptsHandler static properties

Encapsulation (McpServer, HttpRequestHandler):

  • Made McpServer::$error_handler and ::$observability_handler private
  • Updated ~15 internal call sites across handlers, transport, and tests to use get_error_handler() / get_observability_handler()
  • Made HttpRequestHandler::$transport_context private and added get_transport_context() getter
  • Updated 5 call sites in HttpTransport

Constructor validation (McpTransportContext):

  • Replaced unsafe dynamic foreach assignment with explicit key validation
  • Required keys (7) must be present — throws InvalidArgumentException listing missing keys
  • Unknown keys are rejected — throws InvalidArgumentException listing offending keys
  • 13 new tests covering valid construction, missing keys, unknown keys, and optional key handling

Test plan

  • 13 new unit tests for McpTransportContext key validation
  • All existing tests updated to use getter methods instead of direct property access
  • Full suite: 988 tests, 3800 assertions, 0 failures
  • PHPStan Level 8: 0 errors
  • PHPCS: 0 violations

Copilot AI review requested due to automatic review settings March 26, 2026 13:02
@github-actions
Copy link
Copy Markdown

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: galatanovidiu <ovidiu-galatan@git.wordpress.org>

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

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 26, 2026

Codecov Report

❌ Patch coverage is 88.67925% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.04%. Comparing base (da6d892) to head (d7519c4).

Files with missing lines Patch % Lines
includes/Handlers/Prompts/PromptsHandler.php 62.50% 3 Missing ⚠️
includes/Handlers/Tools/ToolsHandler.php 80.00% 1 Missing ⚠️
includes/Transport/HttpTransport.php 80.00% 1 Missing ⚠️
...es/Transport/Infrastructure/HttpRequestHandler.php 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk     #154      +/-   ##
============================================
+ Coverage     87.76%   88.04%   +0.27%     
- Complexity     1234     1236       +2     
============================================
  Files            54       54              
  Lines          3996     4022      +26     
============================================
+ Hits           3507     3541      +34     
+ Misses          489      481       -8     
Flag Coverage Δ
unit 88.04% <88.67%> (+0.27%) ⬆️

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.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR strengthens internal type safety and encapsulation across the MCP adapter by removing dynamic property assignment, tightening handler access, and adding validation + tests for McpTransportContext.

Changes:

  • Added strict typing / native property types in domain + handlers (declare(strict_types=1), typed static properties).
  • Made internal handler properties private (McpServer, HttpRequestHandler) and updated call sites to use getters.
  • Reworked McpTransportContext construction to validate required/unknown keys and added a new unit test suite for constructor validation.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tests/Unit/Transport/Infrastructure/McpTransportContextTest.php Adds constructor validation tests for McpTransportContext.
tests/Unit/Servers/DefaultServerFactoryTest.php Updates assertions to use McpServer getters for handlers.
tests/Unit/McpServerTest.php Updates to use get_error_handler() after handler encapsulation.
tests/Integration/ErrorHandlingIntegrationTest.php Updates integration assertions to use get_error_handler().
includes/Transport/Infrastructure/McpTransportContext.php Replaces dynamic assignment with explicit validation + property assignment.
includes/Transport/Infrastructure/HttpRequestHandler.php Makes transport context private and introduces get_transport_context().
includes/Transport/HttpTransport.php Updates call sites to use HttpRequestHandler::get_transport_context().
includes/Handlers/Tools/ToolsHandler.php Switches error logging to McpServer::get_error_handler().
includes/Handlers/Resources/ResourcesHandler.php Switches error logging to McpServer::get_error_handler().
includes/Handlers/Prompts/PromptsHandler.php Adds typed static properties; updates error/observability usage to getters.
includes/Domain/Prompts/RegisterAbilityAsMcpPrompt.php Adds missing declare(strict_types=1).
includes/Core/McpServer.php Makes handler properties private; documents get_error_handler().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Add missing declare(strict_types=1) to RegisterAbilityAsMcpPrompt.php
— the only file in the codebase without it. Add explicit array and
string type declarations to PromptsHandler static properties.
Make McpServer::$error_handler and ::$observability_handler private —
these properties are not part of the frozen public API. All ~15
internal call sites updated to use the existing get_error_handler()
and get_observability_handler() getters.

Make HttpRequestHandler::$transport_context private and add a
get_transport_context() getter. Update HttpTransport call sites.
Replace unsafe dynamic property assignment with explicit key
validation. Missing required keys and unknown keys now throw
InvalidArgumentException with descriptive messages. Prevents
silent failures from typos and PHP 8.2+ dynamic property issues.
Default error_handler to the server's handler when not provided,
preventing uninitialized typed property access on PHP 8.2+.
Remove misleading `/* translators: */` comments from non-i18n strings.
Align McpTransportContextTest method names with the dominant
codebase convention (955/986 methods use all-lowercase snake_case).
Cover 5 previously-uncovered diff lines to raise patch coverage
from 86.79% to 96.23%. The three handler catch blocks are exercised
via result filters that throw, simulating faulty third-party plugins.
Two PromptsHandler edge cases (non-array message, missing content type)
are covered by abilities returning malformed data.
@galatanovidiu galatanovidiu force-pushed the fix/encapsulation-and-type-safety branch from 7e015d8 to d7519c4 Compare March 27, 2026 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants