Skip to content

Allow pre-built instance handlers in element registration#375

Open
valeriudev wants to merge 2 commits into
modelcontextprotocol:mainfrom
valeriudev:fix/instance-method-handler
Open

Allow pre-built instance handlers in element registration#375
valeriudev wants to merge 2 commits into
modelcontextprotocol:mainfrom
valeriudev:fix/instance-method-handler

Conversation

@valeriudev
Copy link
Copy Markdown

Registering an element with a pre-built object instance as the handler throws at build time:

$foo = new Foo();
Server::builder()->addTool(handler: [$foo, 'bar'], name: 'foo')->build();
// Mcp\Exception\InvalidArgumentException: Invalid array handler format.
// Expected [ClassName::class, 'methodName'].  (HandlerResolver.php)

This is the only place that rejects an instance: the Handler type alias already declares array{0: object|string, 1: string}, getHandlerDescription() already formats object handlers, and the runtime invoker (ReferenceHandler) already dispatches them via its is_callable branch. HandlerResolver::resolve()'s guard simply required a string in slot 0, out of sync with that contract.

Fix: accept an object in slot 0 and normalize it to its class name for reflection — the same is_object($h[0]) ? $h[0]::class : $h[0] idiom getHandlerDescription() already uses. The guard can't be replaced by is_callable(), since is_callable([Class::class, 'instanceMethod']) is false and that lazy-instantiate form must keep working.

This unblocks handler objects with constructor dependencies that the container-less new $className() fallback cannot build.

Tests: added testResolvesValidInstanceArrayHandler; full unit suite, CS, and PHPStan are green.

Fixes #369

`HandlerResolver::resolve()` rejected `[$instance, 'methodName']` array
handlers even though the `Handler` type alias and the runtime invoker
already support them, so `Server::builder()->addTool(handler: [$obj, 'bar'])`
threw "Invalid array handler format" at build time.

Relax the array-shape guard to accept an object in slot 0 and normalize it
to its class name for reflection (the same idiom getHandlerDescription()
already uses). This unblocks dependency-injected handler objects that the
container-less `new $className()` fallback cannot build.

Fixes modelcontextprotocol#369
This is a bugfix with no new public API or BC break, so the changelog
entry belongs under 0.6.1 rather than a new 0.7.0 section.

Add an end-to-end test that registers a pre-built instance handler with a
constructor dependency the container-less new $className() fallback cannot
satisfy, then drives a real tools/call through CallToolHandler and asserts
the injected instance is the one invoked.
if (\is_array($handler)) {
if (2 !== \count($handler) || !isset($handler[0]) || !isset($handler[1]) || !\is_string($handler[0]) || !\is_string($handler[1])) {
throw new InvalidArgumentException('Invalid array handler format. Expected [ClassName::class, \'methodName\'].');
if (2 !== \count($handler) || !isset($handler[0]) || !isset($handler[1]) || !(\is_string($handler[0]) || \is_object($handler[0])) || !\is_string($handler[1])) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is_object($handler[0]) now accepts any object, including Closure and anonymous-class instances. A handler like [$closure, 'method'] would pass this guard, then $closure::class yields Closure, and the user gets Handler method "method" not found in class "Closure" instead of the format error — which is misleading.

Two options:

  • Explicitly reject Closure here (!$handler[0] instanceof \Closure) so the format-error path catches it.
  • Or accept it knowingly and document that [$closure, 'method'] is undefined behavior.

Also: the 4-clause negated condition is hard to scan. Consider extracting $isHandler = \is_string($handler[0]) || \is_object($handler[0]) or splitting into early returns.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pre-existing nit surfaced while reviewing this PR: at L78, the error message reads 'Handler method "%s::%s" must be abstract.' but the branch rejects abstract methods — it should read must not be abstract. Worth a follow-up commit while this file is open.

Comment thread CHANGELOG.md
0.6.1
-----

* Allow registering an element handler as a pre-built object instance (`[$instance, 'methodName']`) via `Builder::addTool()`, `addResource()`, `addResourceTemplate()`, and `addPrompt()`. `HandlerResolver` previously rejected instances with "Invalid array handler format" even though the `Handler` type already permitted them — this unblocks handler objects with constructor dependencies that the container-less fallback cannot build.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Allow registering an element handler as a pre-built object instance (`[$instance, 'methodName']`) via `Builder::addTool()`, `addResource()`, `addResourceTemplate()`, and `addPrompt()`. `HandlerResolver` previously rejected instances with "Invalid array handler format" even though the `Handler` type already permitted them — this unblocks handler objects with constructor dependencies that the container-less fallback cannot build.
* Allow `[$instance, 'methodName']` as an element handler in `Builder::addTool()`, `addResource()`, `addResourceTemplate()`, and `addPrompt()`. Unblocks handlers with constructor dependencies that the container-less `new $className()` fallback cannot build.

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.

[Server] Passing tool handler using instance method throws "Invalid array handler"

2 participants