Move HttpTransport hook registration to boot() - #222
Conversation
Add boot() to McpTransportInterface so transports register WordPress hooks after construction. McpTransportFactory calls boot() immediately after instantiation. Closes WordPress#52.
|
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #222 +/- ##
=========================================
Coverage 87.86% 87.86%
- Complexity 1245 1246 +1
=========================================
Files 53 53
Lines 4037 4039 +2
=========================================
+ Hits 3547 3549 +2
Misses 490 490
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
abhi3315
left a comment
abhi3315
left a comment
There was a problem hiding this comment.
Thanks for working on this. I found one issue I think blocks merging: adding boot() to the existing public interface breaks current custom transports. The other comments are smaller test and documentation fixes.
One documentation gap isn’t on a changed line: docs/architecture/overview.md still shows the old interface and a custom transport without the new lifecycle. It should match whichever contract is chosen. The branch also predates 0.6.1, so it will need an update from trunk and fresh CI before merge.
| $this->assertSame( 16, has_action( 'rest_api_init', array( $transport, 'register_routes' ) ) ); | ||
| } | ||
|
|
||
| private function createTransportContext(): McpTransportContext { |
There was a problem hiding this comment.
These could go in tests/phpunit/Integration/HttpTransportTest.php instead. Its set_up() already builds the server, context and an HttpTransport, so you'd just assert against $this->transport and drop this helper. There are already five other copies of createTransportContext() in the suite.
Assertions look fine. This is only about placement.
| * | ||
| * Called by McpTransportFactory after construction so constructors stay free of hook side effects. | ||
| */ | ||
| public function boot(): void { |
There was a problem hiding this comment.
"so constructors stay free of hook side effects" is only true of this one. new McpServer( ..., array( HttpTransport::class ), ... ) still registers the hook before the constructor returns, through setup_components() -> initialize_transports() -> boot().
#52 is about this constructor, so the PR does close it. The plural here just overstates it. Maybe narrow it to HttpTransport::__construct().
Also worth adding @since n.e.x.t here and on the interface method.
| $this->context = $context; | ||
| } | ||
|
|
||
| public function boot(): void { |
There was a problem hiding this comment.
This calls register_routes() directly, but HttpTransport::boot() adds a hook. That matters under WP-CLI.
McpAdapter.php:62-66 hooks init at 20 under WP-CLI and rest_api_init at 15 otherwise. So on the CLI path this runs before rest_api_init has fired, and register_rest_route() hits core's ! did_action( 'rest_api_init' ) check and calls _doing_it_wrong().
Same shape as HttpTransport works. 16 because the adapter itself runs at 15:
public function boot(): void {
add_action( 'rest_api_init', array( $this, 'register_routes' ), 16 );
}Not new in this PR, the constructor on trunk did the same thing. But line 166 now says boot() registers hooks, so the example doesn't match it.
| new $mcp_transport( $context ); | ||
| $context = $this->create_transport_context(); | ||
| $transport = new $mcp_transport( $context ); | ||
| $transport->boot(); |
There was a problem hiding this comment.
If you go with the separate interface (see my comment on McpTransportInterface.php), this becomes:
if ( $transport instanceof BootableTransportInterface ) {
$transport->boot();
}| * Register WordPress hooks here rather than in the constructor. | ||
| * Called by McpTransportFactory immediately after instantiation. | ||
| */ | ||
| public function boot(): void; |
There was a problem hiding this comment.
I think this one blocks. Adding boot() here breaks any existing custom transport that doesn't already have a public boot(): void. PHP throws a fatal when the class is declared and you can't catch it. Under autoload that happens on the class_exists() at line 48, before the interface check at :67.
There is a real implementation outside this repo. wp-media/mcp-oauth v1.1 has OAuthHttpTransport implements McpRestTransportInterface with no boot(). Its ^0.5 constraint means Composer won't pull it in on 0.6.x, so it isn't broken today, but it shows the interface does get implemented downstream. #159 is the closer precedent, that reverted a property visibility change because it broke a downstream transport.
A separate interface avoids touching the existing contract:
interface BootableTransportInterface {
public function boot(): void;
}HttpTransport implements both, and the factory checks instanceof before calling boot().
| new $mcp_transport( $context ); | ||
| $context = $this->create_transport_context(); | ||
| $transport = new $mcp_transport( $context ); | ||
| $transport->boot(); |
There was a problem hiding this comment.
No test asserts that the factory calls boot(). The new tests call HttpTransport::boot() directly. I removed this line locally and the suite still passed, OK (1004 tests, 3876 assertions). Codecov marks it covered because it runs, but nothing checks it.
A counter on DummyTransport::boot() would test it. Needs to be static since the factory never returns the instance, and reset per test since the fixture is shared:
DummyTransport::$boot_count = 0;
$this->transport_factory->initialize_transports( array( DummyTransport::class ) );
$this->assertSame( 1, DummyTransport::$boot_count );If boot() moves to its own interface, worth checking the other case too, that a transport with only the old interface still initializes.
2 participants
Summary
Follow-up to #52 (review comment on #48): HttpTransport no longer registers
est_api_init in its constructor.
boot()toMcpTransportInterfacefor WordPress hook registration after constructionMcpTransportFactorycallsboot()immediately after instantiating each transportHttpTransport::boot()keeps the existingrest_api_initpriority (16) behaviorMotivation
Constructors should set up dependencies only; hook registration is explicit and easier to test in isolation. Happy to adjust naming or scope if maintainers prefer a lighter-touch approach (e.g. factory-only hook without an interface change).
Closes #52
Test plan
HttpTransportBootTestconfirms the constructor does not register hooks andboot()registersrest_api_initat priority 16rest_url( 'mcp/mcp-adapter-default-server' ))