[MCP Bundle] Make MonologBundle optional #864
Conversation
Fixes symfony#830 - Wrap monolog.logger.mcp service creation in class_exists() check - Add ->nullOnInvalid() to setLogger() call - Uniformize logger references in McpCommand and McpController - Update test to skip when MonologBundle is not installed Builds on symfony#849 by @WebMamba
camilleislasse
requested review from
Nyholm,
OskarStark and
chr-hertel
as code owners
carsonbot
changed the title
OskarStark
added
the
MCP Bundle
carsonbot
changed the title
OskarStark
changed the title
WebMamba
left a comment
WebMamba
left a comment
There was a problem hiding this comment.
Thanks @camilleislasse to give me a hand on this.
| if (!class_exists(MonologBundle::class)) { | ||
| $this->markTestSkipped('MonologBundle is not installed'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Since monolog is never installed on the ci, this test will be always skipped, so the rest of the test is useless. What do you think about requiring monolog on dev then?
src/mcp-bundle/config/services.php
Outdated
| ->call('setPaginationLimit', [param('mcp.pagination_limit')]) | ||
| ->call('setInstructions', [param('mcp.instructions')]) | ||
| ->call('setLogger', [service('monolog.logger.mcp')]) | ||
| ->call('setLogger', [service('monolog.logger.mcp')->nullOnInvalid()]) |
There was a problem hiding this comment.
The function setLogger can't have a nullable as argument see:
https://github.com/modelcontextprotocol/php-sdk/blob/7a3b473c405411aca8be14c0c6e12d2493752d61/src/Server/Builder.php#L254.
So I don't think this can work... Maybe to have an if condition before the call can be better ?
There was a problem hiding this comment.
Yes and another error probably here :
There was a problem hiding this comment.
I'm not entirely happy with this solution ecea6d6 - conditionally instantiating transports with/without the logger parameter feels verbose.
The issue: MCP SDK has LoggerInterface $logger = new NullLogger() (non-nullable), so we can't pass null even though there's a default, if you have a better idea @WebMamba
Can change if modelcontextprotocol/php-sdk#152 is correct
…ts, and handle nullable logger in McpCommand/McpController by conditionally passing it to Transports. When MonologBundle is absent, Server and Transports use their default NullLogger.
|
I tend more and more to require the logger |
Me too, I feel like we’re making things unnecessarily complicated 😂 |
4 participants
This PR makes MonologBundle optional dependency for McpBundle. Without it installed, the container compilation was failing with "Parent definition' monolog.logger_prototype' does not exist."
Changes
monolog.logger.mcpservice creation inclass_exists()check->nullOnInvalid()tosetLogger()call in services.phpmonolog.logger.mcpwithNULL_ON_INVALID_REFERENCECredits
Builds on the initial work in #849 by @WebMamba. That PR protected service creation but the service was still referenced without protection. This PR completes the fix by also protecting the service usage.