Skip to content

Conversation

@CodeWithKyrian
Copy link
Contributor

This PR makes the server extensibility model revolve around JSON-RPC method handlers instead of bespoke executor abstractions.

Motivation and Context

  • Each new MCP feature previously needed its own “executor” surface area (tool caller, resource reader, prompt getter, completer) plus builder wiring before users could override behavior.
  • Custom methods or protocol extensions were effectively blocked until the SDK shipped matching executors.
  • Other official SDKs expose low-level request handlers directly; aligning with that pattern keeps PHP users on equal footing.

Changes

  • Server builder now accepts user-defined MethodHandlerInterface implementations via addMethodHandler(s); they are registered ahead of built-ins so overrides win automatically.
  • Core request handlers (CallToolHandler, ReadResourceHandler, GetPromptHandler, CompletionCompleteHandler) were refactored to talk straight to the registry/reference infrastructure, eliminating the Tool/Resource/Prompt/Completion executor layer.
  • JsonRpcHandler was simplified to operate on an injected handler list, and the builder now assembles the complete handler pipeline (including defaults) with the standard message/session factories.
  • Added a examples/custom-method-handlers scenario that demonstrates replacing the default list/call tool handlers end-to-end.
  • Updated/removed unit tests to reflect the new architecture and dropped the now-obsolete executor interfaces and classes.

Breaking Changes

  • Removed ToolCaller*, ResourceReader*, PromptGetter*, and Completer* classes/interfaces along with Server\Builder setters (setToolCaller, setResourceReader, setPromptGetter).
  • JsonRpcHandler::make() factory was deleted; construct it directly (the builder already does this).

@chr-hertel
Copy link
Member

I must say that i really like this - slimming it down with DX in mind 👍

I try to think of counter arguments - not to veto this, i think we should merge it, but to be aware of the downsides we're taking here.

  • We loose one extension point, or rather a spot where people could have switch implementations - but that's rather unlikely - the case you're optimizing here for is way more likely
  • So merging it together bring more concerns to single classes making them harder to test - but i think we can still fight redundancies

did you already do some functional tests or is that still open?

@CodeWithKyrian
Copy link
Contributor Author

Thanks for the feedback!

  • On extension point loss: I actually thought at first that someone could have a custom ToolCaller/ResourceReader/PromptGetter implementations and still rely on our registry/reference handler wiring. But a deeper look made me realize it never really worked that way. For the inbuilt ToolCaller/ResourceReader, the builder always creates those executors and injects the internal dependencies, but a custom implementation will have to be instantiated outside the builder, and thus won't have access to the registry and reference handler. Which means anyone implementing a custom ToolCaller for example will have to manually handle everything without the Registry and Reference handler. So we're not loosing anything per se IMO.

  • Regarding the handlers doing more: the code that moved is exactly what lived in ToolCaller/ResourceReader/PromptGetter/Completer. Those handlers before that move were basically super thin request routers, so they weren't doing much in the first place. The abstraction to the executors was majorly for exposing an extension point. Besides, the “real” work still happens in the registry + reference handler, so the classes are no more complex than the executors were.

  • For testing: For now I repurposed the executor unit tests so they exercise the method handlers directly, which covers the behavior that used to be delegated. I haven’t finished auditing the old executor suites to see if any edge-case assertions should be ported over, so I’ll go through them and push a follow-up if anything is missing.

Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

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

good for merging after rebase 👍

Thanks @CodeWithKyrian for this :)

@CodeWithKyrian CodeWithKyrian force-pushed the feature/custom-method-handlers branch from 2c5f6bd to 487b758 Compare October 7, 2025 22:08
@CodeWithKyrian
Copy link
Contributor Author

Good to go!

@chr-hertel chr-hertel merged commit fcf605d into modelcontextprotocol:main Oct 7, 2025
10 checks passed
@CodeWithKyrian CodeWithKyrian deleted the feature/custom-method-handlers branch October 21, 2025 16:14
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