From e5824b5a001e7d02567b48af01b4270428baa261 Mon Sep 17 00:00:00 2001 From: camilleislasse Date: Wed, 8 Oct 2025 14:42:02 +0200 Subject: [PATCH] [MCP Bundle] Fix dependency injection and standardize invokable pattern This commit fixes critical issues preventing MCP tools with constructor dependencies from working and aligns the codebase with the official MCP SDK. Changes: - Fix McpPass to use Reference objects for ServiceLocator registration Previously passed tag arrays directly, causing crashes when tools had constructor dependencies - Fix services.php to use Server::make() instead of Server::builder() The official mcp/sdk uses Server::make() returning ServerBuilder, not Builder class with builder() method - Migrate all demo MCP capabilities to invokable pattern The Bundle's registerAttributeForAutoconfiguration() only detects class-level attributes, not method-level. Changed to: * Tool: #[McpTool] on class + __invoke() method * Prompt: #[McpPrompt] on class + __invoke() method * Resource: #[McpResource] on class + __invoke() method * ResourceTemplate: #[McpResourceTemplate] on class + __invoke() method - Add LoggerInterface to CurrentTimeTool to demonstrate DI works - Update documentation examples to show invokable pattern - Improve McpPassTest to verify ServiceLocator uses References Previous tests only checked presence, not type of values --- demo/src/Mcp/Prompts/CurrentTimePrompt.php | 4 ++-- .../CurrentTimeResourceTemplate.php | 4 ++-- .../src/Mcp/Resources/CurrentTimeResource.php | 4 ++-- demo/src/Mcp/Tools/CurrentTimeTool.php | 16 ++++++++++---- docs/bundles/mcp-bundle.rst | 16 +++++++------- src/mcp-bundle/config/services.php | 6 ++--- .../src/DependencyInjection/McpPass.php | 8 ++++++- .../tests/DependencyInjection/McpPassTest.php | 22 +++++++++++++++++++ 8 files changed, 58 insertions(+), 22 deletions(-) diff --git a/demo/src/Mcp/Prompts/CurrentTimePrompt.php b/demo/src/Mcp/Prompts/CurrentTimePrompt.php index 549a28ce0..9221c4005 100644 --- a/demo/src/Mcp/Prompts/CurrentTimePrompt.php +++ b/demo/src/Mcp/Prompts/CurrentTimePrompt.php @@ -13,10 +13,10 @@ use Mcp\Capability\Attribute\McpPrompt; +#[McpPrompt(name: 'time-analysis')] class CurrentTimePrompt { - #[McpPrompt(name: 'time-analysis')] - public function getTimeAnalysisPrompt(): array + public function __invoke(): array { return [ [ diff --git a/demo/src/Mcp/ResourceTemplates/CurrentTimeResourceTemplate.php b/demo/src/Mcp/ResourceTemplates/CurrentTimeResourceTemplate.php index 177a7d04c..4454655e1 100644 --- a/demo/src/Mcp/ResourceTemplates/CurrentTimeResourceTemplate.php +++ b/demo/src/Mcp/ResourceTemplates/CurrentTimeResourceTemplate.php @@ -13,10 +13,10 @@ use Mcp\Capability\Attribute\McpResourceTemplate; +#[McpResourceTemplate(uriTemplate: 'time://{timezone}', name: 'time-by-timezone')] class CurrentTimeResourceTemplate { - #[McpResourceTemplate(uriTemplate: 'time://{timezone}', name: 'time-by-timezone')] - public function getTimeByTimezone(string $timezone): array + public function __invoke(string $timezone): array { try { $time = (new \DateTime('now', new \DateTimeZone($timezone)))->format('Y-m-d H:i:s T'); diff --git a/demo/src/Mcp/Resources/CurrentTimeResource.php b/demo/src/Mcp/Resources/CurrentTimeResource.php index 27f1d8e71..2e2e2df7c 100644 --- a/demo/src/Mcp/Resources/CurrentTimeResource.php +++ b/demo/src/Mcp/Resources/CurrentTimeResource.php @@ -13,10 +13,10 @@ use Mcp\Capability\Attribute\McpResource; +#[McpResource(uri: 'time://current', name: 'current-time-resource')] class CurrentTimeResource { - #[McpResource(uri: 'time://current', name: 'current-time-resource')] - public function getCurrentTimeResource(): array + public function __invoke(): array { return [ 'uri' => 'time://current', diff --git a/demo/src/Mcp/Tools/CurrentTimeTool.php b/demo/src/Mcp/Tools/CurrentTimeTool.php index bd52edc72..8e9ea6ec4 100644 --- a/demo/src/Mcp/Tools/CurrentTimeTool.php +++ b/demo/src/Mcp/Tools/CurrentTimeTool.php @@ -12,20 +12,28 @@ namespace App\Mcp\Tools; use Mcp\Capability\Attribute\McpTool; +use Psr\Log\LoggerInterface; /** + * Returns the current time in UTC. + * * @author Tom Hart */ +#[McpTool(name: 'current-time')] class CurrentTimeTool { + public function __construct( + private readonly LoggerInterface $logger, + ) { + } + /** - * Returns the current time in UTC. - * * @param string $format The format of the time, e.g. "Y-m-d H:i:s" */ - #[McpTool(name: 'current-time')] - public function getCurrentTime(string $format = 'Y-m-d H:i:s'): string + public function __invoke(string $format = 'Y-m-d H:i:s'): string { + $this->logger->info('CurrentTimeTool called', ['format' => $format]); + return (new \DateTime('now', new \DateTimeZone('UTC')))->format($format); } } diff --git a/docs/bundles/mcp-bundle.rst b/docs/bundles/mcp-bundle.rst index 5c641a532..129ee462c 100644 --- a/docs/bundles/mcp-bundle.rst +++ b/docs/bundles/mcp-bundle.rst @@ -36,10 +36,10 @@ Actions that can be executed:: use Mcp\Capability\Attribute\McpTool; + #[McpTool(name: 'current-time')] class CurrentTimeTool { - #[McpTool(name: 'current-time')] - public function getCurrentTime(string $format = 'Y-m-d H:i:s'): string + public function __invoke(string $format = 'Y-m-d H:i:s'): string { return (new \DateTime('now', new \DateTimeZone('UTC')))->format($format); } @@ -52,10 +52,10 @@ System instructions for AI context:: use Mcp\Capability\Attribute\McpPrompt; + #[McpPrompt(name: 'time-analysis')] class TimePrompts { - #[McpPrompt(name: 'time-analysis')] - public function getTimeAnalysisPrompt(): array + public function __invoke(): array { return [ ['role' => 'user', 'content' => 'You are a time management expert.'] @@ -70,10 +70,10 @@ Static data that can be read:: use Mcp\Capability\Attribute\McpResource; + #[McpResource(uri: 'time://current', name: 'current-time')] class TimeResource { - #[McpResource(uri: 'time://current', name: 'current-time')] - public function getCurrentTimeResource(): array + public function __invoke(): array { return [ 'uri' => 'time://current', @@ -97,10 +97,10 @@ Dynamic resources with parameters: use Mcp\Capability\Attribute\McpResourceTemplate; + #[McpResourceTemplate(uriTemplate: 'time://{timezone}', name: 'time-by-timezone')] class TimeResourceTemplate { - #[McpResourceTemplate(uriTemplate: 'time://{timezone}', name: 'time-by-timezone')] - public function getTimeByTimezone(string $timezone): array + public function __invoke(string $timezone): array { $time = (new \DateTime('now', new \DateTimeZone($timezone)))->format('Y-m-d H:i:s T'); return [ diff --git a/src/mcp-bundle/config/services.php b/src/mcp-bundle/config/services.php index 3da39d99d..fc27af04f 100644 --- a/src/mcp-bundle/config/services.php +++ b/src/mcp-bundle/config/services.php @@ -12,7 +12,7 @@ namespace Symfony\Component\DependencyInjection\Loader\Configurator; use Mcp\Server; -use Mcp\Server\Builder; +use Mcp\Server\ServerBuilder; return static function (ContainerConfigurator $container): void { $container->services() @@ -21,8 +21,8 @@ ->args(['mcp']) ->tag('monolog.logger', ['channel' => 'mcp']) - ->set('mcp.server.builder', Builder::class) - ->factory([Server::class, 'builder']) + ->set('mcp.server.builder', ServerBuilder::class) + ->factory([Server::class, 'make']) ->call('setServerInfo', [param('mcp.app'), param('mcp.version')]) ->call('setPaginationLimit', [param('mcp.pagination_limit')]) ->call('setInstructions', [param('mcp.instructions')]) diff --git a/src/mcp-bundle/src/DependencyInjection/McpPass.php b/src/mcp-bundle/src/DependencyInjection/McpPass.php index 1061de6d1..4ebcb6c53 100644 --- a/src/mcp-bundle/src/DependencyInjection/McpPass.php +++ b/src/mcp-bundle/src/DependencyInjection/McpPass.php @@ -14,6 +14,7 @@ use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface; use Symfony\Component\DependencyInjection\Compiler\ServiceLocatorTagPass; use Symfony\Component\DependencyInjection\ContainerBuilder; +use Symfony\Component\DependencyInjection\Reference; final class McpPass implements CompilerPassInterface { @@ -35,7 +36,12 @@ public function process(ContainerBuilder $container): void return; } - $serviceLocatorRef = ServiceLocatorTagPass::register($container, $allMcpServices); + $serviceReferences = []; + foreach (array_keys($allMcpServices) as $serviceId) { + $serviceReferences[$serviceId] = new Reference($serviceId); + } + + $serviceLocatorRef = ServiceLocatorTagPass::register($container, $serviceReferences); $container->getDefinition('mcp.server.builder') ->addMethodCall('setContainer', [$serviceLocatorRef]); diff --git a/src/mcp-bundle/tests/DependencyInjection/McpPassTest.php b/src/mcp-bundle/tests/DependencyInjection/McpPassTest.php index 0982e8054..1f3a1e088 100644 --- a/src/mcp-bundle/tests/DependencyInjection/McpPassTest.php +++ b/src/mcp-bundle/tests/DependencyInjection/McpPassTest.php @@ -13,8 +13,10 @@ use PHPUnit\Framework\TestCase; use Symfony\AI\McpBundle\DependencyInjection\McpPass; +use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Definition; +use Symfony\Component\DependencyInjection\Reference; /** * @covers \Symfony\AI\McpBundle\DependencyInjection\McpPass @@ -53,6 +55,18 @@ public function testCreatesServiceLocatorForAllMcpServices() $this->assertArrayHasKey('prompt_service', $services); $this->assertArrayHasKey('resource_service', $services); $this->assertArrayHasKey('template_service', $services); + + // Verify services are ServiceClosureArguments wrapping References + $this->assertInstanceOf(ServiceClosureArgument::class, $services['tool_service']); + $this->assertInstanceOf(ServiceClosureArgument::class, $services['prompt_service']); + $this->assertInstanceOf(ServiceClosureArgument::class, $services['resource_service']); + $this->assertInstanceOf(ServiceClosureArgument::class, $services['template_service']); + + // Verify the underlying values are References + $this->assertInstanceOf(Reference::class, $services['tool_service']->getValues()[0]); + $this->assertInstanceOf(Reference::class, $services['prompt_service']->getValues()[0]); + $this->assertInstanceOf(Reference::class, $services['resource_service']->getValues()[0]); + $this->assertInstanceOf(Reference::class, $services['template_service']->getValues()[0]); } public function testDoesNothingWhenNoMcpServicesTagged() @@ -115,5 +129,13 @@ public function testHandlesPartialMcpServices() $this->assertArrayHasKey('prompt_service', $services); $this->assertArrayNotHasKey('resource_service', $services); $this->assertArrayNotHasKey('template_service', $services); + + // Verify services are ServiceClosureArguments wrapping References + $this->assertInstanceOf(ServiceClosureArgument::class, $services['tool_service']); + $this->assertInstanceOf(ServiceClosureArgument::class, $services['prompt_service']); + + // Verify the underlying values are References + $this->assertInstanceOf(Reference::class, $services['tool_service']->getValues()[0]); + $this->assertInstanceOf(Reference::class, $services['prompt_service']->getValues()[0]); } }