Skip to content

[Server] Fix to make Symfony Finder component optional#267

Open
bigdevlarry wants to merge 1 commit intomodelcontextprotocol:mainfrom
bigdevlarry:Make-Symfony-Finder-component-optional
Open

[Server] Fix to make Symfony Finder component optional#267
bigdevlarry wants to merge 1 commit intomodelcontextprotocol:mainfrom
bigdevlarry:Make-Symfony-Finder-component-optional

Conversation

@bigdevlarry
Copy link
Copy Markdown
Contributor

@bigdevlarry bigdevlarry commented Mar 15, 2026

Motivation and Context

The changes make the Symfony Finder component optional, as referenced in issue #263

How Has This Been Tested?

Yes

Breaking Changes

Not a breaking change

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@bigdevlarry bigdevlarry force-pushed the Make-Symfony-Finder-component-optional branch from 68bd923 to bc19684 Compare March 15, 2026 02:36
@bigdevlarry bigdevlarry force-pushed the Make-Symfony-Finder-component-optional branch 2 times, most recently from dd9bfef to 6e8e705 Compare March 23, 2026 13:14
@soyuka
Copy link
Copy Markdown
Contributor

soyuka commented Mar 24, 2026

Can you also add a change to https://github.com/modelcontextprotocol/php-sdk/blob/main/src/Server/Builder.php#L526 ? Avoid calling the createDiscoverer when Finder isn't accessible + log? This makes php-sdk not dependent from the Finder.

@bigdevlarry
Copy link
Copy Markdown
Contributor Author

@soyuka, this means the discoverer will never be called if there's no finder, and we won't be able to raise the exception to prompt the installation of the finder itself. By your suggestion, adding a null check silences the error in the logs, and the program continues as expected. Is this the ideal behaviour, or should we halt the program with the exception as we have it, so users go back to install? Let me know what you think as well @chr-hertel

@soyuka
Copy link
Copy Markdown
Contributor

soyuka commented Mar 25, 2026

Mhh from this PR I thought you wanted to move the Finder to a dev-requirement making this a soft dependency. From my point of view if that's the case the code should work after installing with composer and I'd have thought that it'd make sense to skip classes dependent from the Finder.

If we don't want to move this requirement, then I'm not sure I understand how this code path is going to happen as its required in our dependencies.

@bigdevlarry bigdevlarry force-pushed the Make-Symfony-Finder-component-optional branch from 5e7405e to 7438ab1 Compare March 27, 2026 18:37
@chr-hertel
Copy link
Copy Markdown
Member

Mhh from this PR I thought you wanted to move the Finder to a dev-requirement making this a soft dependency.
yes

and yes, the code should work unless you want to use the built-in discoverer.

if a user calls setDiscovery we can expect the intend is clear - calling setDiscovery, not calling setDiscoverer with an custom implementation, but not installing the symfony/finder should raise a warning or exception from my point of view.

We should add this as BC break to changelog tho, since users would need to install symfony/finder now themselves.

@chr-hertel chr-hertel added this to the 0.5.0 milestone Mar 30, 2026
@chr-hertel chr-hertel added the Server Issues & PRs related to the Server component label Mar 30, 2026
@chr-hertel chr-hertel changed the title Fix to make Symfony Finder component optional [Server]Fix to make Symfony Finder component optional Mar 30, 2026
@chr-hertel chr-hertel changed the title [Server]Fix to make Symfony Finder component optional [Server] Fix to make Symfony Finder component optional Mar 30, 2026
@chr-hertel chr-hertel added the needs more work Not ready to be merged yet, needs additional follow-up from the author(s). label Mar 30, 2026
@chr-hertel
Copy link
Copy Markdown
Member

@soyuka this is now what you've been thinking, right?

@bigdevlarry bigdevlarry force-pushed the Make-Symfony-Finder-component-optional branch from 903b991 to 98991a8 Compare March 31, 2026 19:30
@bigdevlarry bigdevlarry force-pushed the Make-Symfony-Finder-component-optional branch from 0c437fa to febba83 Compare March 31, 2026 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs more work Not ready to be merged yet, needs additional follow-up from the author(s). Server Issues & PRs related to the Server component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants