[Server] Allow pre-built instance handlers in element registration (#375)
* Allow pre-built instance handlers in element registration `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 #369 * Use 0.6.1 changelog heading and test instance handler end-to-end 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. * Address review: reject Closure array handlers, fix abstract-method message - HandlerResolver: extract the array-handler guard into a readable $hasValidTarget check that accepts strings and objects but excludes \Closure, so [$closure, 'method'] falls through to the format error instead of the misleading "method not found in class Closure". - Fix the abstract-method rejection message ("must be abstract" -> "must not be abstract"), a pre-existing typo in the same branch. - Tighten the CHANGELOG entry wording. - Cover Closure rejection with a regression test. * Move instance-handler entry under 0.7.0 changelog heading Per review: registering pre-built instance handlers is a feature, not a patch, so it belongs in a minor release heading.
This commit is contained in:
parent
1587ac5bd2
commit
55f7884368
4 changed files with 77 additions and 9 deletions
|
|
@ -2,6 +2,11 @@
|
|||
|
||||
All notable changes to `mcp/sdk` will be documented in this file.
|
||||
|
||||
0.7.0
|
||||
-----
|
||||
|
||||
* 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.
|
||||
|
||||
0.6.0
|
||||
-----
|
||||
|
||||
|
|
|
|||
|
|
@ -11,11 +11,14 @@
|
|||
|
||||
namespace Mcp\Capability\Discovery;
|
||||
|
||||
use Mcp\Capability\Registry\ElementReference;
|
||||
use Mcp\Exception\InvalidArgumentException;
|
||||
|
||||
/**
|
||||
* Utility class to validate and resolve MCP element handlers.
|
||||
*
|
||||
* @phpstan-import-type Handler from ElementReference
|
||||
*
|
||||
* @author Kyrian Obikwelu <koshnawaza@gmail.com>
|
||||
*/
|
||||
class HandlerResolver
|
||||
|
|
@ -25,11 +28,12 @@ class HandlerResolver
|
|||
*
|
||||
* A handler can be:
|
||||
* - A Closure: function() { ... }
|
||||
* - An array: [ClassName::class, 'methodName'] (instance method)
|
||||
* - An array: [ClassName::class, 'methodName'] (resolved on a new or container-provided instance)
|
||||
* - An array: [$instance, 'methodName'] (method on a pre-built object instance)
|
||||
* - An array: [ClassName::class, 'staticMethod'] (static method, if callable)
|
||||
* - A string: InvokableClassName::class (which will resolve to its '__invoke' method)
|
||||
*
|
||||
* @param \Closure|array{0: string, 1: string}|string $handler the handler to resolve
|
||||
* @param Handler $handler the handler to resolve
|
||||
*
|
||||
* @throws InvalidArgumentException If the handler format is invalid, the class/method doesn't exist,
|
||||
* or the method is unsuitable (e.g., private, abstract).
|
||||
|
|
@ -41,10 +45,17 @@ class HandlerResolver
|
|||
}
|
||||
|
||||
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\'].');
|
||||
// A Closure in slot 0 must fall through to the format error rather than
|
||||
// be treated as an instance, where "$closure::class" would yield the
|
||||
// misleading class name "Closure".
|
||||
$target = $handler[0] ?? null;
|
||||
$hasValidTarget = (\is_string($target) || \is_object($target)) && !$target instanceof \Closure;
|
||||
|
||||
if (2 !== \count($handler) || !$hasValidTarget || !isset($handler[1]) || !\is_string($handler[1])) {
|
||||
throw new InvalidArgumentException('Invalid array handler format. Expected [ClassName::class, \'methodName\'] or [$instance, \'methodName\'].');
|
||||
}
|
||||
[$className, $methodName] = $handler;
|
||||
[$classOrObject, $methodName] = $handler;
|
||||
$className = \is_object($classOrObject) ? $classOrObject::class : $classOrObject;
|
||||
if (!class_exists($className)) {
|
||||
throw new InvalidArgumentException(\sprintf('Handler class "%s" not found for array handler.', $className));
|
||||
}
|
||||
|
|
@ -70,7 +81,7 @@ class HandlerResolver
|
|||
throw new InvalidArgumentException(\sprintf('Handler method "%s::%s" must be public.', $className, $methodName));
|
||||
}
|
||||
if ($reflectionMethod->isAbstract()) {
|
||||
throw new InvalidArgumentException(\sprintf('Handler method "%s::%s" must be abstract.', $className, $methodName));
|
||||
throw new InvalidArgumentException(\sprintf('Handler method "%s::%s" must not be abstract.', $className, $methodName));
|
||||
}
|
||||
if ($reflectionMethod->isConstructor() || $reflectionMethod->isDestructor()) {
|
||||
throw new InvalidArgumentException(\sprintf('Handler method "%s::%s" cannot be a constructor or destructor.', $className, $methodName));
|
||||
|
|
|
|||
|
|
@ -38,6 +38,15 @@ class HandlerResolverTest extends TestCase
|
|||
$this->assertEquals(ValidHandlerClass::class, $resolved->getDeclaringClass()->getName());
|
||||
}
|
||||
|
||||
public function testResolvesValidInstanceArrayHandler(): void
|
||||
{
|
||||
$handler = [new ValidHandlerClass(), 'publicMethod'];
|
||||
$resolved = HandlerResolver::resolve($handler);
|
||||
$this->assertInstanceOf(\ReflectionMethod::class, $resolved);
|
||||
$this->assertEquals('publicMethod', $resolved->getName());
|
||||
$this->assertEquals(ValidHandlerClass::class, $resolved->getDeclaringClass()->getName());
|
||||
}
|
||||
|
||||
public function testResolvesValidInvokableClassStringHandler(): void
|
||||
{
|
||||
$handler = ValidInvokableClass::class;
|
||||
|
|
@ -59,17 +68,24 @@ class HandlerResolverTest extends TestCase
|
|||
public function testThrowsForInvalidArrayHandlerFormatCount(): void
|
||||
{
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
$this->expectExceptionMessage("Invalid array handler format. Expected [ClassName::class, 'methodName'].");
|
||||
$this->expectExceptionMessage('Invalid array handler format. Expected [ClassName::class, \'methodName\'] or [$instance, \'methodName\'].');
|
||||
HandlerResolver::resolve([ValidHandlerClass::class]); /* @phpstan-ignore argument.type */
|
||||
}
|
||||
|
||||
public function testThrowsForInvalidArrayHandlerFormatTypes(): void
|
||||
{
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
$this->expectExceptionMessage("Invalid array handler format. Expected [ClassName::class, 'methodName'].");
|
||||
$this->expectExceptionMessage('Invalid array handler format. Expected [ClassName::class, \'methodName\'] or [$instance, \'methodName\'].');
|
||||
HandlerResolver::resolve([ValidHandlerClass::class, 123]); /* @phpstan-ignore argument.type */
|
||||
}
|
||||
|
||||
public function testThrowsForClosureInArrayHandler(): void
|
||||
{
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
$this->expectExceptionMessage('Invalid array handler format. Expected [ClassName::class, \'methodName\'] or [$instance, \'methodName\'].');
|
||||
HandlerResolver::resolve([static fn () => null, 'method']);
|
||||
}
|
||||
|
||||
public function testThrowsForNonExistentClassInArrayHandler(): void
|
||||
{
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
|
|
@ -129,7 +145,7 @@ class HandlerResolverTest extends TestCase
|
|||
public function testThrowsForAbstractMethodHandler(): void
|
||||
{
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
$this->expectExceptionMessage('Handler method "Mcp\Tests\Unit\Capability\Discovery\AbstractHandlerClass::abstractMethod" must be abstract.');
|
||||
$this->expectExceptionMessage('Handler method "Mcp\Tests\Unit\Capability\Discovery\AbstractHandlerClass::abstractMethod" must not be abstract.');
|
||||
HandlerResolver::resolve([AbstractHandlerClass::class, 'abstractMethod']);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -83,6 +83,25 @@ final class BuilderTest extends TestCase
|
|||
$this->assertSame('intercepted', $result);
|
||||
}
|
||||
|
||||
#[TestDox('A pre-built instance handler with constructor dependencies is registered and invoked on that instance')]
|
||||
public function testPreBuiltInstanceHandlerIsInvokedOnTheGivenInstance(): void
|
||||
{
|
||||
// The handler's constructor requires an argument the container-less
|
||||
// `new $className()` fallback can never satisfy. If the tool call
|
||||
// succeeds, it can only be because the pre-built instance — carrying
|
||||
// its injected dependency — was the one invoked.
|
||||
$handler = new GreetingService('World');
|
||||
|
||||
$server = Server::builder()
|
||||
->setServerInfo('test', '1.0.0')
|
||||
->addTool(handler: [$handler, 'greet'], name: 'greet', description: 'Greets using the injected name')
|
||||
->build();
|
||||
|
||||
$result = $this->callTool($server, 'greet');
|
||||
|
||||
$this->assertSame('Hello, World', $result);
|
||||
}
|
||||
|
||||
#[TestDox('enableExtension() registers an extension and announces its capability payload')]
|
||||
public function testEnableExtensionRegistersExtension(): void
|
||||
{
|
||||
|
|
@ -166,3 +185,20 @@ final class BuilderTest extends TestCase
|
|||
$this->fail('CallToolHandler not found in request handlers');
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A handler whose constructor dependency cannot be satisfied by the
|
||||
* container-less `new $className()` fallback, so it must be registered as a
|
||||
* pre-built instance.
|
||||
*/
|
||||
final class GreetingService
|
||||
{
|
||||
public function __construct(private string $name)
|
||||
{
|
||||
}
|
||||
|
||||
public function greet(): string
|
||||
{
|
||||
return 'Hello, '.$this->name;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue