Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's go with 0.7.0 here - it's rather a feature to me:

Suggested change
0.6.1
0.7.0

-----

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


0.6.0
-----

Expand Down
15 changes: 10 additions & 5 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,11 @@ 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\'].');
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
Author

Choose a reason for hiding this comment

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

Went with option A — explicitly rejecting \Closure so [$closure, 'method'] falls through to the format error instead of the misleading "method not found in class Closure". Also extracted the guard into a $hasValidTarget local to drop the 4-clause negation, and added a regression test (testThrowsForClosureInArrayHandler).

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 Down
13 changes: 11 additions & 2 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,14 +68,14 @@ 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 */
}

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;
}
}