-
Notifications
You must be signed in to change notification settings - Fork 62
[Server] Registry Architecture Refactoring - Enhanced Separation of Concerns #46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Server] Registry Architecture Refactoring - Enhanced Separation of Concerns #46
Conversation
Hey @butschster, first of all thanks for kicking off that discussion - I think there are a couple of instance in our current code base where we're mixing too many concerns in one class, as well state vs service classes. so, yes, let's work on this. there is #39 going on at the same time, and i think it would be easier to pull off after this PR here - @xentixar what do you think? (need to give your proposed slicing a deeper look tho later) |
Hello, @chr-hertel ! 👋 As you noticed, I’ve taken the time to study the project thoroughly. In this pull request, I tried to demonstrate how I see the structure evolving. The example I used comes from my production MCP servers, which are based on @CodeWithKyrian’s MCP package. This gave me the opportunity to validate the approach in a real-world environment and think through how the structure could be reorganized more effectively. If you like the direction, I’m ready to finish the task. |
@chr-hertel Thanks for bringing this up! I agree that it would make sense to complete the Registry architecture refactoring in PR 46 first, as it will likely simplify the implementation in PR 39 and reduce potential conflicts. The separation of concerns approach outlined in this PR aligns well with what I'm working on, and having the cleaner interfaces in place will definitely make the integration smoother. I'm happy to wait for this refactoring to be merged before proceeding with my work. This will also give me a chance to review the new architecture and ensure my implementation follows the improved patterns. Thanks for coordinating this - it's much better to get the foundation right first! |
@chr-hertel could you please run actions to check tests and cs? |
I think I’m done with this iteration, but a couple of notes:
However, Should I get rid of arguments or add some todo? If a README or usage notes for the new features would help, let me know and I can add one. 🚀 |
@chr-hertel @xentixar Ready to review I added unit tests for the code affected by my changes and fixed the PHPStan issues, except for those that need to be discussed and addressed collaboratively and that are unrelated to my changes. After writing the tests, I found a small issue and created an issue for it #50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great step in the right direction - left a few comments, but will revisit after the diff is cleaned up, and pipeline is green.
Thanks already! 👍
@chr-hertel done |
@chr-hertel I want to merge changes from #53 to my PR after merging. Ready to wait |
Fine by me 👍 One thing i wonder, what do you think about repository pattern here - per type of capability? Yes, we're not dealing with a database engine here, true, but with #39 we'd need to think about state here as well. And going forward with a per-capability-design, would potentially ease to bring in per-capability-infrastructure in list/get request handlers as well. don't get me wrong, this PR is def an improvement - just a thought :) |
- Extract ReferenceProvider and ReferenceRegistryInterface interfaces - Create DefaultToolExecutor with ReferenceHandlerInterface - Remove execution responsibility from Registry class - Enable custom handler and executor implementations
* Create DefaultResourceReader, DefaultPromptGetter * Refactor JsonRpc Handler and RequestHandlers to use dedicated executors * Update ServerBuilder to support custom executors via dependency injection
Hi @chr-hertel, thanks for raising this point! From my experience building a more complex tool CTX that already embeds an MCP server inside, I tend to agree with the idea of splitting things up per capability. If I understood your question correctly, here’s my perspective: an MCP server usually exposes several types of elements — tools, resources, prompts, and prompt templates. Each of these can come from different sources:
Because of this variety, a single “global” repository abstraction quickly becomes too rigid. Sooner or later we’ll need the flexibility to aggregate from multiple sources per type. That’s where having dedicated repositories per capability — or even composite repositories — really pays off. They allow us to cleanly plug in new sources without overloading one central abstraction. In practice, this makes the design easier to extend and reason about, especially when different types of resources evolve at different speeds and have very different lifecycles. So personally, I see “more granularity” here as a net positive, and a repository-per-capability approach would give us the room to support heterogeneous sources without bending the model too much. There are repositories for each resource type - ResourceRegistry, ToolRegistry, PromptRegistry? |
@chr-hertel @CodeWithKyrian I fixed code style. phpstan will be fixed here #55 |
Hi @butschster, the PHPStan issues are related to your pull request. Please take a look. It would be easier if you resolved them now, so I can pull into my branch if your PR gets merged first. If my work gets merged first, you can update your branch afterward, as I've fixed the PHPStan issues in #55. |
Hi @bigdevlarry, thanks for pointing this out. Just to clarify: the PHPStan issues you’re seeing were already present before my changes. For example, pagination logic wasn’t working initially and still needs additional work, so I didn’t touch it here since my focus was on other parts of the code. Another reported issue is related to the examples — that code was also non-functional from the start. I left it as is to avoid bloating this pull request with unrelated fixes. So I’d suggest handling those issues separately, outside of this PR. |
Hi @chr-hertel! |
I’ve completed all the changes for this pull request, and all tests and qa are passing. Thanks a lot for your time and feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my point of view this is good to get merged 👍
I think we could go on with a per-capability approach on top of that, and I'm not sure about the design of every interface, but it's a great next step.
@CodeWithKyrian do you agree?
Totally agree. Looks solid to me. Thanks for the great work on this, @butschster 👍🏽 |
Thanks @butschster 🙌 |
…nsport (#49) * feat(server): Rework transport architecture and add StreamableHttpTransport - `Server::connect()` no longer contains a processing loop. - `TransportInterface` is updated with `setMessageHandler()` and `listen()`. - `StdioTransport` is updated to implement the new interface. - A new, minimal `StreamableHttpTransport` is added for stateless HTTP. * refactor(server): use event-driven message handling * feat(server): Introduce a formal session management system * fix: ensure session elements are preserved when building server (regression from #46) * refactor: consolidate HTTP example to use shared dependencies * feat(server): Enhance message handling with session support - Updated `TransportInterface` to use `onMessage` for handling incoming messages with session IDs. - Refactored `Server`, `Handler`, and transport classes to accommodate session management using `Uuid`. - Introduced methods for creating sessions with auto-generated and specific UUIDs in `SessionFactory` and `SessionFactoryInterface`. * feat(server): Integrate session management in message handling - Added session support to the `Server` and `Handler` classes, allowing for session data to be managed during message processing. - Updated `TransportInterface` to include session context in the `send` method. - Refactored various request handlers to utilize session information, ensuring proper session handling for incoming requests. - Introduced a file-based session store for persistent session data management * feat: Update session builder method to use set* instead of with* * feat: Fix test compatibility with new session management architecture * chore: apply code style fixes * fix: remove deprecated SSE transport and resolve PHPStan issues
I would like to propose a refactoring of the Registry architecture to better align with SOLID principles and improve the overall design. This is currently a work-in-progress draft, and I welcome feedback and discussion on the approach.
Real-World Integration Issues
This particularly affects integration with validation libraries like Valinor:
The Interface Solution
ReferenceHandlerInterface
enables:This interface transforms the rigid, filtering-based approach into a flexible, composable system that works with the PHP ecosystem rather than against it.
Usage Example
Before (Current Approach)
After (Proposed Approach)
Custom Handler Example
Benefits
Dependency Injection and Container Integration
The interfaces are crucial for proper dependency injection and container bindings in modern PHP applications:
Registry Provider Binding
Handler Interface Binding
Tool Executor Binding
Current Issue
The existing
Registry
class currently handles both registration and execution of MCP elements, which violates the Single Responsibility Principle. This creates several challenges:Proposed Solution
This refactoring introduces a cleaner separation of concerns by splitting the Registry responsibilities:
Interface Segregation
ReferenceProvider Interface - Focused on accessing registered elements:
ReferenceRegistry Interface - Focused on managing registrations:
Handler Interface
ReferenceHandlerInterface - Enables custom execution strategies:
This allows users to implement their own handling logic while maintaining compatibility with the existing system.
Default Tool Executor
DefaultToolExecutor - Demonstrates the intended usage pattern:
Discussion Points
I would appreciate feedback on several aspects:
Why ReferenceHandlerInterface is Critical
The current
ReferenceHandler
class presents significant extensibility challenges that this refactoring addresses:Current Limitations
The existing
ReferenceHandler
uses a rigid argument filtering mechanism that breaks custom callable patterns:The Problem with Custom Wrappers
This filtering breaks patterns where developers want to implement custom argument processing:
Context Note
The
ToolExecutorInterface
was already present in the codebase but wasn't being utilized as the primary execution mechanism. This refactoring makes it the central component for tool execution as originally intended, rather than just an extension point that wasn't actively used.Next Steps
If this approach looks reasonable, I would like to:
Thank you for taking the time to review this proposal. I look forward to your thoughts and suggestions.