Allow pre-built instance handlers in element registration#375
Allow pre-built instance handlers in element registration#375valeriudev wants to merge 2 commits into
Conversation
`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])) { |
There was a problem hiding this comment.
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
Closurehere (!$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.
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
| * 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. |
Registering an element with a pre-built object instance as the handler throws at build time:
This is the only place that rejects an instance: the
Handlertype alias already declaresarray{0: object|string, 1: string},getHandlerDescription()already formats object handlers, and the runtime invoker (ReferenceHandler) already dispatches them via itsis_callablebranch.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]idiomgetHandlerDescription()already uses. The guard can't be replaced byis_callable(), sinceis_callable([Class::class, 'instanceMethod'])isfalseand 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