[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
2 times, most recently
from
2eaf78e to
488a09e
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
488a09e to
8836c40
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