Skip to content

Conversation

camilleislasse
Copy link

@camilleislasse camilleislasse commented Oct 8, 2025

Q A
Bug fix? yes
New feature? no
Docs? yes
Issues #755
License MIT

** [MCP Bundle] Fix dependency injection and method-based / invokable support**

This commit fixes critical issues preventing MCP tools with constructor
dependencies from working and enables method-based attributes support.

Changes:

  • Fix McpPass to use Reference objects for ServiceLocator registration
    Previously passed tag arrays directly, causing crashes when tools had
    constructor dependencies

  • Fix McpBundle::registerMcpAttributes() closure signature
    Added missing object $attribute and \Reflector $reflector parameters.
    Without these parameters, Symfony's AttributeAutoconfigurationPass only
    scans class-level attributes and skips method-level ones entirely

  • Add LoggerInterface to CurrentTimeTool to demonstrate DI works

  • Update documentation to reflect both patterns are supported
    Added examples showing invokable and method-based patterns both work
    with full DI support

  • Improve McpPassTest to verify ServiceLocator uses References
    Previous tests only checked presence, not type of values

@carsonbot carsonbot added Bug Something isn't working MCP Bundle Issues & PRs about the MCP SDK integration bundle Status: Needs Review labels Oct 8, 2025
@chr-hertel
Copy link
Member

Hi @camilleislasse,

thanks for working on this - is the best next step! 👍

Can you add your findings about when #[Mcp*] work on methods vs. classes with the bundle to the docs? i think for now that should be a warning there.

@camilleislasse
Copy link
Author

Hi @camilleislasse,

thanks for working on this - is the best next step! 👍

Can you add your findings about when #[Mcp*] work on methods vs. classes with the bundle to the docs? i think for now that should be a warning there.

80da7cb

@OskarStark
Copy link
Contributor

I get the note, but couldn't we make it work to support both?

@camilleislasse
Copy link
Author

I get the note, but couldn't we make it work to support both?

I have find a way :

By changing the closure signature from:

static function (ChildDefinition $definition) use ($tag): void {

To:

static function (ChildDefinition $definition, object $attribute, \Reflector $reflector) use ($tag): void {

Symfony now scans both class-level attributes (invokable pattern) and method-level attributes (method-based pattern). The \Reflector parameter tells Symfony to also check methods, properties, and parameters - not just classes.

@camilleislasse camilleislasse force-pushed the fix/mcp-di-invokable-pattern branch 4 times, most recently from 13cab83 to bd87ca4 Compare October 14, 2025 16:33
@camilleislasse camilleislasse changed the title [MCP Bundle] Fix dependency injection and standardize invokable pat… [MCP Bundle] Fix dependency injection and method-based / invokable support Oct 14, 2025
@camilleislasse camilleislasse force-pushed the fix/mcp-di-invokable-pattern branch from bd87ca4 to dc04f24 Compare October 14, 2025 16:39
@chr-hertel
Copy link
Member

That's great news - thanks @camilleislasse!

Comment on lines 116 to 142
.. warning::

The MCP Bundle also ** supports the invokable pattern** where attributes are placed on classes
with an ``__invoke()`` method.

** Invokable ** :

#[McpTool(name: 'my-tool')]
class MyTool
{
public function __invoke(string $param): string
{
// Implementation
}
}

** Method Based ** :

class MyTools
{
#[McpTool(name: 'tool-one')]
public function toolOne(): string { }

#[McpTool(name: 'tool-two')]
public function toolTwo(): string { }
}

Copy link
Member

Choose a reason for hiding this comment

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

This should be a regular section like "Defining Capabilities" or "Attribute Usage" or other, instead of a warning

@chr-hertel
Copy link
Member

does it work for you though?
i currently get this error while running the branch locally with inspector:
image

@camilleislasse
Copy link
Author

camilleislasse commented Oct 15, 2025

i currently get this error while running the branch locally with inspector:

does it work for you though? i currently get this error while running the branch locally with inspector: image

Yes working, I look

Capture d’écran 2025-10-15 à 10 04 46
  1. Do you have the MonologBundle installed?
  2. Did you run composer install and cache:clear after pulling the branch?

Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

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

Needed to rebase the branch for making it work - but it works 👍

Thanks @camilleislasse!

@carsonbot carsonbot changed the title [MCP Bundle] Fix dependency injection and method-based / invokable support [MCP Bundle] Fix dependency injection and method-based / invokable support Oct 20, 2025
@chr-hertel chr-hertel force-pushed the fix/mcp-di-invokable-pattern branch from a6614eb to bdd050c Compare October 20, 2025 20:02
@chr-hertel chr-hertel merged commit bc7036a into symfony:main Oct 20, 2025
15 checks passed
@camilleislasse
Copy link
Author

Needed to rebase the branch for making it work - but it works 👍

Thanks @camilleislasse!

Cool thank's 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working MCP Bundle Issues & PRs about the MCP SDK integration bundle Status: Reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants