Repository navigation
[Server] Answer server/discover with a request handler - #622
Open
BrocksiNet wants to merge 3 commits into
Open
BrocksiNet wants to merge 3 commits into
BrocksiNet wants to merge 3 commits into
Conversation
`StatelessProtocol` answered `server/discover` itself, before dispatch, so a server could not change the answer and the request skipped the request, response and error events every other method goes through. Answer it with a default `DiscoverHandler`, like `initialize` is answered with `InitializeHandler`. `Builder::buildStateless()` registers it after the custom handlers, so a handler added with `addRequestHandler()` that supports `DiscoverRequest` answers instead, and adds `DiscoverRequest` to the modern dispatcher's message factory only, so the handshake era still does not parse `server/discover`. The wire answer is unchanged and is never streamed, also when the client accepts `text/event-stream`. The private `StatelessProtocol::discover()` is removed.
BrocksiNet
requested review from
CodeWithKyrian,
Nyholm,
chr-hertel and
soyuka
as code owners
October 26, 2026 14:42
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block the change enabling customizable modern-era discovery handlers.
No open issues 🎉
What changed in this PR
Moves server/discover into the SDK’s request-handler pipeline, addressing #621 by allowing servers to customize modern-era discovery independently of handshake-era initialization.
Changes:
- Adds a default
DiscoverHandlerwith modern-only registration and support for custom handlers and request events. - Preserves the default response format and plain-JSON behavior, including when clients accept streams.
- Adds handler, protocol, and stdio tests, plus customization documentation.
| File | Description |
|---|---|
tests/Unit/Server/Transport/StdioDualEraTest.php |
Tests custom discovery handling and handshake-era rejection. |
tests/Unit/Server/Stateless/StatelessProtocolTest.php |
Tests default output, overrides, events, and non-streaming responses. |
tests/Unit/Server/Handler/Request/DiscoverHandlerTest.php |
Tests supported requests and configuration-derived results. |
src/Server/Stateless/StatelessProtocol.php |
Routes discovery through dispatch while preventing streaming. |
src/Server/Handler/Request/DiscoverHandler.php |
Implements the default discovery response. |
src/Server/Builder.php |
Registers discovery requests and the default handler for the modern era. |
docs/run/server-builder.md |
Links to discovery customization guidance. |
docs/run/protocol-eras.md |
Documents handler overrides and response-event customization. |
CHANGELOG.md |
Records the discovery-handler change for 0.9.1. |
Review effort: Balanced
…-handler # Conflicts: # CHANGELOG.md
…lers The custom handler and events guides still said the SDK answers `server/discover` itself and dispatches no events for it, which `DiscoverHandler` made untrue.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
server/discoveris answered by a new defaultDiscoverHandler, likeinitializeis answered byInitializeHandler, instead of being hard-coded inStatelessProtocol. The privateStatelessProtocol::discover()is removed.addRequestHandler()run before the defaults, so a handler supportingDiscoverRequestanswers instead. AResponseEventlistener can also adjust the default answer, sinceserver/discovernow dispatchesRequestEvent,ResponseEventandErrorEventlike every other request.server/discoverexists in the modern era only, soBuilder::buildStateless()adds both its message class (DiscoverRequest) and its default handler there, not to the parts both eras share. The handshake era therefore still never parsesserver/discover: it answers it as before, and a custom discover handler is not reachable from it.resultType, caching hints andserverInfo), status codes and header validation run as before, and the answer is never streamed, also when the client acceptstext/event-stream(a replacement handler that emits notifications gets them dropped, as for a client that does not read streams).server/discover" indocs/run/protocol-eras.mdwith a short example, and a pointer indocs/run/server-builder.md.I considered a per-era config field (
setModernInstructions()), but it only covers instructions and would copy every public attribute per era. A handler covers any member with the extension point the SDK already has.Motivation: Shopware serves both eras and hides its session-bound toolset tools on 2026-07-28, so its
initializeinstructions must not reach stateless clients. Today it rewrites theserver/discoverJSON response as a workaround: shopware/shopware#22011Fixes #621
Test plan
DiscoverHandlerTest: supports onlyDiscoverRequest, answers with versions, capabilities and instructions, leaves instructions out when none are setStatelessProtocolTest: the default answer equals the previous wire output exactly; a handler added through the builder replaces it; request and response events are dispatched and aResponseEventlistener can change the answer; a client acceptingtext/event-streamgets plain JSON even from a handler that emits a notification (fails without the change)StdioDualEraTest: a custom discover handler answers a modern stdio client, and a handshake-era stdio connection still gets-32600forserver/discovermake tests(2960 tests)make phpstanmake cscomposer validate --strict