[Server] Add missing handler for resource subscribe/unsubscribe handlers #220
Conversation
bigdevlarry
requested review from
CodeWithKyrian,
Nyholm and
chr-hertel
as code owners
bigdevlarry
force-pushed
the
Add-missing-handler-for-Resource-Subscribe-Unsubscribe
branch
2 times, most recently
from
627f00b to
95be722
Compare
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| ini_set('display_errors', '0'); |
There was a problem hiding this comment.
This prevents PHP from printing errors to the response body fixes the JSON-RPC error of outputting html
bigdevlarry
force-pushed
the
Add-missing-handler-for-Resource-Subscribe-Unsubscribe
branch
2 times, most recently
from
c0e0c22 to
f40d08e
Compare
chr-hertel
added
Server
chr-hertel
changed the title
chr-hertel
left a comment
chr-hertel
left a comment
There was a problem hiding this comment.
Thanks @bigdevlarry for working on this - my main concern is about bloating the Registry, other than that i think the comments are good to tackle 👍 Thanks again!
chr-hertel
left a comment
chr-hertel
left a comment
There was a problem hiding this comment.
Hey @bigdevlarry, I'm afraid we need to rollback a bit here - you really needed to go an extra mile to bring in the cross-session support, but i have some doubts about the getAllSessionIds. I think while integrating the SDK some will just replace the built-in session with something they have in their application infrastucture, and that might get tricky with that method - you already had to do some extra work here.
I'd like to ask you for this:
- let's rename the
Mcp\Server\Resource\ResourceSubscriptionInterfacetoMcp\Server\Resource\SubscriptionManagerInterface - let's rename the
Mcp\Server\Resource\ResourceSubscriptiontoMcp\Server\Resource\SessionSubscriptionManageras one concrete implementation we ship out of the box, which only supports notifications per session
we would add a disclaimer as code comment and in documentation, but also the openness to inject custom built - potentially cross client - SubscriptionManagerInterface implementations - you basically enable that already with the method on the Builder that you added -> rename to Builder::setResourceSubscriptionManager
On top, please rebase to get rid of pipeline failure, also see #230.
Thanks again for working on this! :)
bigdevlarry
force-pushed
the
Add-missing-handler-for-Resource-Subscribe-Unsubscribe
branch
3 times, most recently
from
488a09e to
8836c40
Compare
|
Hey @chr-hertel I've implemented the following feedback
This aligns with your suggestion to keep session-specific mechanics out of the subscription contract while still supporting the built-in session-based notifier which is brilliant :) |
| * | ||
| * @throws InvalidArgumentException | ||
| */ | ||
| public function notifyResourceChanged(Protocol $protocol, string $uri): void; |
There was a problem hiding this comment.
regarding that notify method, i was curious about injecting the Protocol here and while tackling #234 I understood why you went with it - it's a bit hard to access that.
Happy to hear your thoughts about that other PR, we could adopt that event handling here as well.
There was a problem hiding this comment.
I agree with the proposed approach; we could adopt an event listener approach here rather than the current approach, which requires injecting Session alongside the Protocol. I'll add my thoughts to #234
bigdevlarry
force-pushed
the
Add-missing-handler-for-Resource-Subscribe-Unsubscribe
branch
4 times, most recently
from
49191f3 to
bd38a1f
Compare
Add missing handler for resource subscribe and unsubscribe Add missing handler for resource subscribe and unsubscribe Add missing handler for resource subscribe and unsubscribe Add missing handler for resource subscribe and unsubscribe Add missing handler for resource subscribe and unsubscribe Add missing handler for resource subscribe and unsubscribe Add missing handler for resource subscribe and unsubscribe Add missing handler for resource subscribe and unsubscribe Add missing handler for resource subscribe and unsubscribe Add missing handler for resource subscribe and unsubscribe
bigdevlarry
force-pushed
the
Add-missing-handler-for-Resource-Subscribe-Unsubscribe
branch
from
bd38a1f to
20f43fb
Compare
3 participants
Add missing handler for resource subscribe/unsubscribe handlers
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context