Skip to content

Commit cbbccd9

Browse files
CodeWithKyrianCodeWithKyrian
committed
feat: Introduce HandlerResolver for robust MCP element handler validation
1 parent b63c79d commit cbbccd9

File tree

4 files changed

+197
-94
lines changed

4 files changed

+197
-94
lines changed

‎src/ServerBuilder.php‎

Lines changed: 14 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use PhpMcp\Server\Exception\DefinitionException;
1010
use PhpMcp\Server\Model\Capabilities;
1111
use PhpMcp\Server\State\ClientStateManager;
12+
use PhpMcp\Server\Support\HandlerResolver;
1213
use Psr\Container\ContainerInterface;
1314
use Psr\Log\LoggerInterface;
1415
use Psr\Log\NullLogger;
@@ -44,9 +45,7 @@ final class ServerBuilder
4445

4546
private array $manualPrompts = [];
4647

47-
public function __construct()
48-
{
49-
}
48+
public function __construct() {}
5049

5150
/**
5251
* Sets the server's identity. Required.
@@ -208,16 +207,16 @@ private function performManualRegistrations(Registry $registry, LoggerInterface
208207
// Register Tools
209208
foreach ($this->manualTools as $data) {
210209
try {
211-
$methodRefl = $this->validateAndGetReflectionMethod($data['handler']);
210+
$resolvedHandler = HandlerResolver::resolve($data['handler']);
212211
$def = Definitions\ToolDefinition::fromReflection(
213-
$methodRefl,
212+
$resolvedHandler['reflectionMethod'],
214213
$data['name'],
215214
$data['description'],
216215
$docBlockParser,
217216
$schemaGenerator
218217
);
219218
$registry->registerTool($def, true);
220-
$logger->debug("Registered manual tool '{$def->getName()}'");
219+
$logger->debug("Registered manual tool '{$def->getName()}' from handler {$resolvedHandler['className']}::{$resolvedHandler['methodName']}");
221220
} catch (Throwable $e) {
222221
$errorCount++;
223222
$logger->error('Failed to register manual tool', ['handler' => $data['handler'], 'name' => $data['name'], 'exception' => $e]);
@@ -227,9 +226,9 @@ private function performManualRegistrations(Registry $registry, LoggerInterface
227226
// Register Resources
228227
foreach ($this->manualResources as $data) {
229228
try {
230-
$methodRefl = $this->validateAndGetReflectionMethod($data['handler']);
229+
$resolvedHandler = HandlerResolver::resolve($data['handler']);
231230
$def = Definitions\ResourceDefinition::fromReflection(
232-
$methodRefl,
231+
$resolvedHandler['reflectionMethod'],
233232
$data['name'],
234233
$data['description'],
235234
$data['uri'],
@@ -239,7 +238,7 @@ private function performManualRegistrations(Registry $registry, LoggerInterface
239238
$docBlockParser
240239
);
241240
$registry->registerResource($def, true);
242-
$logger->debug("Registered manual resource '{$def->getUri()}'");
241+
$logger->debug("Registered manual resource '{$def->getUri()}' from handler {$resolvedHandler['className']}::{$resolvedHandler['methodName']}");
243242
} catch (Throwable $e) {
244243
$errorCount++;
245244
$logger->error('Failed to register manual resource', ['handler' => $data['handler'], 'uri' => $data['uri'], 'exception' => $e]);
@@ -249,9 +248,9 @@ private function performManualRegistrations(Registry $registry, LoggerInterface
249248
// Register Templates
250249
foreach ($this->manualResourceTemplates as $data) {
251250
try {
252-
$methodRefl = $this->validateAndGetReflectionMethod($data['handler']);
251+
$resolvedHandler = HandlerResolver::resolve($data['handler']);
253252
$def = Definitions\ResourceTemplateDefinition::fromReflection(
254-
$methodRefl,
253+
$resolvedHandler['reflectionMethod'],
255254
$data['name'],
256255
$data['description'],
257256
$data['uriTemplate'],
@@ -260,7 +259,7 @@ private function performManualRegistrations(Registry $registry, LoggerInterface
260259
$docBlockParser
261260
);
262261
$registry->registerResourceTemplate($def, true);
263-
$logger->debug("Registered manual template '{$def->getUriTemplate()}'");
262+
$logger->debug("Registered manual template '{$def->getUriTemplate()}' from handler {$resolvedHandler['className']}::{$resolvedHandler['methodName']}");
264263
} catch (Throwable $e) {
265264
$errorCount++;
266265
$logger->error('Failed to register manual template', ['handler' => $data['handler'], 'uriTemplate' => $data['uriTemplate'], 'exception' => $e]);
@@ -270,15 +269,15 @@ private function performManualRegistrations(Registry $registry, LoggerInterface
270269
// Register Prompts
271270
foreach ($this->manualPrompts as $data) {
272271
try {
273-
$methodRefl = $this->validateAndGetReflectionMethod($data['handler']);
272+
$resolvedHandler = HandlerResolver::resolve($data['handler']);
274273
$def = Definitions\PromptDefinition::fromReflection(
275-
$methodRefl,
274+
$resolvedHandler['reflectionMethod'],
276275
$data['name'],
277276
$data['description'],
278277
$docBlockParser
279278
);
280279
$registry->registerPrompt($def, true);
281-
$logger->debug("Registered manual prompt '{$def->getName()}'");
280+
$logger->debug("Registered manual prompt '{$def->getName()}' from handler {$resolvedHandler['className']}::{$resolvedHandler['methodName']}");
282281
} catch (Throwable $e) {
283282
$errorCount++;
284283
$logger->error('Failed to register manual prompt', ['handler' => $data['handler'], 'name' => $data['name'], 'exception' => $e]);
@@ -291,56 +290,4 @@ private function performManualRegistrations(Registry $registry, LoggerInterface
291290

292291
$logger->debug('Manual element registration complete.');
293292
}
294-
295-
/**
296-
* Gets a reflection method from a handler.
297-
*
298-
* @throws \InvalidArgumentException If the handler is invalid.
299-
*/
300-
private function validateAndGetReflectionMethod(array|string $handler): \ReflectionMethod
301-
{
302-
$className = null;
303-
$methodName = null;
304-
305-
if (is_array($handler)) {
306-
if (count($handler) !== 2 || ! is_string($handler[0]) || ! is_string($handler[1])) {
307-
throw new \InvalidArgumentException('Invalid array handler format. Expected [ClassName::class, \'methodName\'].');
308-
}
309-
[$className, $methodName] = $handler;
310-
if (! class_exists($className)) {
311-
throw new \InvalidArgumentException("Class '{$className}' not found for array handler.");
312-
}
313-
if (! method_exists($className, $methodName)) {
314-
throw new \InvalidArgumentException("Method '{$methodName}' not found in class '{$className}' for array handler.");
315-
}
316-
} elseif (is_string($handler) && class_exists($handler)) {
317-
$className = $handler;
318-
$methodName = '__invoke';
319-
if (! method_exists($className, $methodName)) {
320-
throw new \InvalidArgumentException("Invokable class '{$className}' must have a public '__invoke' method.");
321-
}
322-
} else {
323-
throw new \InvalidArgumentException('Invalid handler format. Expected [ClassName::class, \'methodName\'] or InvokableClassName::class string.');
324-
}
325-
326-
try {
327-
$reflectionMethod = new \ReflectionMethod($className, $methodName);
328-
if ($reflectionMethod->isStatic()) {
329-
throw new \InvalidArgumentException("Handler method '{$className}::{$methodName}' cannot be static.");
330-
}
331-
if (! $reflectionMethod->isPublic()) {
332-
throw new \InvalidArgumentException("Handler method '{$className}::{$methodName}' must be public.");
333-
}
334-
if ($reflectionMethod->isAbstract()) {
335-
throw new \InvalidArgumentException("Handler method '{$className}::{$methodName}' cannot be abstract.");
336-
}
337-
if ($reflectionMethod->isConstructor() || $reflectionMethod->isDestructor()) {
338-
throw new \InvalidArgumentException("Handler method '{$className}::{$methodName}' cannot be a constructor or destructor.");
339-
}
340-
341-
return $reflectionMethod;
342-
} catch (\ReflectionException $e) {
343-
throw new \InvalidArgumentException("Reflection error for handler '{$className}::{$methodName}': {$e->getMessage()}", 0, $e);
344-
}
345-
}
346293
}

‎src/Support/HandlerResolver.php‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace PhpMcp\Server\Support;
6+
7+
use InvalidArgumentException;
8+
use ReflectionMethod;
9+
use ReflectionException;
10+
11+
/**
12+
* Utility class to validate and resolve MCP element handlers.
13+
*/
14+
class HandlerResolver
15+
{
16+
/**
17+
* Validates and resolves a handler to its class name, method name, and ReflectionMethod instance.
18+
*
19+
* A handler can be:
20+
* - An array: [ClassName::class, 'methodName']
21+
* - A string: InvokableClassName::class (which will resolve to its '__invoke' method)
22+
*
23+
* @param array|string $handler The handler to resolve.
24+
* @return array{className: class-string, methodName: string, reflectionMethod: ReflectionMethod}
25+
* An associative array containing 'className', 'methodName', and 'reflectionMethod'.
26+
*
27+
* @throws InvalidArgumentException If the handler format is invalid, the class/method doesn't exist,
28+
* or the method is unsuitable (e.g., static, private, abstract).
29+
*/
30+
public static function resolve(array|string $handler): array
31+
{
32+
$className = null;
33+
$methodName = null;
34+
35+
if (is_array($handler)) {
36+
if (count($handler) !== 2 || !isset($handler[0]) || !isset($handler[1]) || !is_string($handler[0]) || !is_string($handler[1])) {
37+
throw new InvalidArgumentException('Invalid array handler format. Expected [ClassName::class, \'methodName\'].');
38+
}
39+
[$className, $methodName] = $handler;
40+
if (!class_exists($className)) {
41+
throw new InvalidArgumentException("Handler class '{$className}' not found for array handler.");
42+
}
43+
if (!method_exists($className, $methodName)) {
44+
throw new InvalidArgumentException("Handler method '{$methodName}' not found in class '{$className}' for array handler.");
45+
}
46+
} elseif (is_string($handler) && class_exists($handler)) {
47+
$className = $handler;
48+
$methodName = '__invoke';
49+
if (!method_exists($className, $methodName)) {
50+
throw new InvalidArgumentException("Invokable handler class '{$className}' must have a public '__invoke' method.");
51+
}
52+
} else {
53+
throw new InvalidArgumentException('Invalid handler format. Expected [ClassName::class, \'methodName\'] or InvokableClassName::class string.');
54+
}
55+
56+
try {
57+
$reflectionMethod = new ReflectionMethod($className, $methodName);
58+
59+
if ($reflectionMethod->isStatic()) {
60+
throw new InvalidArgumentException("Handler method '{$className}::{$methodName}' cannot be static.");
61+
}
62+
if (!$reflectionMethod->isPublic()) {
63+
throw new InvalidArgumentException("Handler method '{$className}::{$methodName}' must be public.");
64+
}
65+
if ($reflectionMethod->isAbstract()) {
66+
throw new InvalidArgumentException("Handler method '{$className}::{$methodName}' cannot be abstract.");
67+
}
68+
if ($reflectionMethod->isConstructor() || $reflectionMethod->isDestructor()) {
69+
throw new InvalidArgumentException("Handler method '{$className}::{$methodName}' cannot be a constructor or destructor.");
70+
}
71+
72+
return [
73+
'className' => $className,
74+
'methodName' => $methodName,
75+
'reflectionMethod' => $reflectionMethod,
76+
];
77+
} catch (ReflectionException $e) {
78+
// This typically occurs if class_exists passed but ReflectionMethod still fails (rare)
79+
throw new InvalidArgumentException("Reflection error for handler '{$className}::{$methodName}': {$e->getMessage()}", 0, $e);
80+
}
81+
}
82+
}

‎tests/Unit/ServerBuilderTest.php‎

Lines changed: 6 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -20,26 +20,18 @@
2020

2121
class DummyHandlerClass
2222
{
23-
public function handle()
24-
{
25-
}
23+
public function handle() {}
2624
}
2725
class DummyInvokableClass
2826
{
29-
public function __invoke()
30-
{
31-
}
27+
public function __invoke() {}
3228
}
3329
class HandlerWithDeps
3430
{
35-
public function __construct(public LoggerInterface $log)
36-
{
37-
}
31+
public function __construct(public LoggerInterface $log) {}
3832

3933
#[McpTool(name: 'depTool')]
40-
public function run()
41-
{
42-
}
34+
public function run() {}
4335
}
4436

4537
beforeEach(function () {
@@ -55,8 +47,6 @@ function getBuilderProperty(ServerBuilder $builder, string $propertyName)
5547
return $property->getValue($builder);
5648
}
5749

58-
// --- Configuration Method Tests ---
59-
6050
it('sets server info', function () {
6151
$this->builder->withServerInfo('MyServer', '1.2.3');
6252
expect(getBuilderProperty($this->builder, 'name'))->toBe('MyServer');
@@ -101,8 +91,6 @@ function getBuilderProperty(ServerBuilder $builder, string $propertyName)
10191
expect(getBuilderProperty($this->builder, 'loop'))->toBe($loop);
10292
});
10393

104-
// --- Manual Registration Storage Tests ---
105-
10694
it('stores manual tool registration data', function () {
10795
$handler = [DummyHandlerClass::class, 'handle'];
10896
$name = 'my-tool';
@@ -149,8 +137,6 @@ function getBuilderProperty(ServerBuilder $builder, string $propertyName)
149137
expect($manualPrompts[0]['name'])->toBe($name);
150138
});
151139

152-
// --- Build Method Validation Tests ---
153-
154140
it('throws exception if build called without server info', function () {
155141
$this->builder
156142
// ->withDiscoveryPaths($this->tempBasePath) // No longer needed
@@ -170,8 +156,6 @@ function getBuilderProperty(ServerBuilder $builder, string $propertyName)
170156
[' ', '1.0'],
171157
]);
172158

173-
// --- Default Dependency Resolution Tests ---
174-
175159
it('resolves default Logger correctly when building', function () {
176160
$server = $this->builder
177161
->withServerInfo('Test', '1.0')
@@ -239,8 +223,7 @@ function getBuilderProperty(ServerBuilder $builder, string $propertyName)
239223
expect($config->loop)->toBeInstanceOf(LoopInterface::class);
240224
expect($config->container)->toBe($container);
241225
expect($config->capabilities)->toBeInstanceOf(Capabilities::class);
242-
243-
}); // REMOVED skip
226+
});
244227

245228
it('successfully creates Server with custom dependencies', function () {
246229
$myLoop = Mockery::mock(LoopInterface::class);
@@ -265,8 +248,7 @@ function getBuilderProperty(ServerBuilder $builder, string $propertyName)
265248
expect($config->cache)->toBe($myCache);
266249
expect($config->capabilities)->toBe($myCaps);
267250
expect($server->getRegistry()->allPrompts()->count())->toBe(1);
268-
269-
}); // REMOVED skip
251+
});
270252

271253
it('throws DefinitionException if manual tool registration fails', function () {
272254
$container = new BasicContainer();
@@ -275,10 +257,8 @@ function getBuilderProperty(ServerBuilder $builder, string $propertyName)
275257
$this->builder
276258
->withServerInfo('FailRegServer', '1.0')
277259
->withContainer($container)
278-
// Use a method that doesn't exist on the mock class
279260
->withTool([DummyHandlerClass::class, 'nonExistentMethod'], 'badTool')
280261
->build();
281-
282262
})->throws(DefinitionException::class, '1 error(s) occurred during manual element registration');
283263

284264
it('throws DefinitionException if manual resource registration fails', function () {
@@ -290,5 +270,4 @@ function getBuilderProperty(ServerBuilder $builder, string $propertyName)
290270
->withContainer($container)
291271
->withResource([DummyHandlerClass::class, 'handle'], 'invalid-uri-no-scheme') // Invalid URI
292272
->build();
293-
294273
})->throws(DefinitionException::class, '1 error(s) occurred during manual element registration');

0 commit comments

Comments
 (0)