Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

All notable changes to `mcp/sdk` will be documented in this file.

0.6.1
-----

* 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
-----

Expand Down
23 changes: 17 additions & 6 deletions src/Capability/Discovery/HandlerResolver.php
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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. must be abstractmust not be abstract, with the assertion in testThrowsForAbstractMethodHandler updated to match

Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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).
Expand All @@ -41,10 +45,17 @@ public static function resolve(\Closure|array|string $handler): \ReflectionMetho
}

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));
}
Expand All @@ -70,7 +81,7 @@ public static function resolve(\Closure|array|string $handler): \ReflectionMetho
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));
Expand Down
22 changes: 19 additions & 3 deletions tests/Unit/Capability/Discovery/HandlerResolverTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ public function testResolvesValidArrayHandler(): void
$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;
Expand All @@ -59,17 +68,24 @@ public function testResolvesStaticMethodsForManualRegistration(): void
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);
Expand Down Expand Up @@ -129,7 +145,7 @@ public function testThrowsForDestructorAsHandler(): void
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']);
}

Expand Down
36 changes: 36 additions & 0 deletions tests/Unit/Server/BuilderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,25 @@ public function testCustomReferenceHandlerIsUsedForToolCalls(): void
$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
{
Expand Down Expand Up @@ -166,3 +185,20 @@ private function callTool(Server $server, string $toolName): mixed
$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;
}
}