feat: add StreamResponse and SSE response factories#10395
Conversation
michalsn
added
new feature
memleakd
left a comment
memleakd
left a comment
There was a problem hiding this comment.
Thanks for working on this, looks useful! I left a few comments:
| // Intentionally skip CSP finalize: the body is streamed, not buffered HTML. | ||
| $this->sendHeaders(); | ||
| $this->sendCookies(); |
There was a problem hiding this comment.
Small question about this: for SSE, skipping CSP makes sense to me, but StreamResponse is generic and could stream HTML.
Would it make sense to still finalize CSP headers when CSP is enabled/touched, even if nonce placeholder replacement cannot really work for streamed bodies?
There was a problem hiding this comment.
Good point. I updated StreamResponse to finalize CSP headers when enabled/touched, while keeping SSE explicitly opted out.
| } | ||
|
|
||
| if (! $this->hasHeader('Content-Encoding')) { | ||
| $this->setHeader('Content-Encoding', 'identity'); |
There was a problem hiding this comment.
I might be reading this too strictly, but would it make sense to avoid setting Content-Encoding: identity here?
RFC 9110 section 8.4-5 says identity is reserved for Accept-Encoding and SHOULD NOT be included in Content-Encoding: https://www.rfc-editor.org/rfc/rfc9110.html#section-8.4-5
Maybe it would be better not to set this by default and leave compression/buffering guidance to the docs/server config?
There was a problem hiding this comment.
Fair point. I removed the default Content-Encoding: identity header and updated tests/docs.
| // Forward the upstream file chunk by chunk, without buffering it in memory | ||
| $source = fopen('https://storage.internal/reports/annual.pdf', 'rb'); |
There was a problem hiding this comment.
Tiny concern with the example: this opens the upstream stream inside the callback, so if fopen() fails the response headers have probably already been sent.
Would it be clearer to open/validate $source before creating the StreamResponse, then only read and close it inside the callback?
There was a problem hiding this comment.
Agreed. I unnecessarily focused only on the streaming mechanism - the example is now updated.
| foreach (model('UserModel')->findAll() as $user) { | ||
| // write() returns false when the client disconnects | ||
| if (! $stream->write("{$user->id},{$user->email}\n")) { |
There was a problem hiding this comment.
Maybe avoiding findAll() here would make the example clearer? Since this section is about streaming, loading all rows first feels a bit misleading.
Also, this looks like CSV, so users may copy it as-is, but direct interpolation would break for commas, quotes, or newlines. Maybe proper CSV escaping or a simpler plain text/NDJSON example would fit better?
There was a problem hiding this comment.
I changed the example to batch records and stream NDJSON, which keeps the focus on streaming and avoids CSV escaping/buffering details.
memleakd
left a comment
memleakd
left a comment
There was a problem hiding this comment.
Thanks for the updates. I left two small follow-up comments.
| */ | ||
| public function eventStream(callable $callback): SSEResponse | ||
| { | ||
| return new SSEResponse($callback); |
There was a problem hiding this comment.
Should this copy the current protocol version to the new response?
This might matter because SSEResponse skips Connection: keep-alive for HTTP/2+, but this creates a fresh response, so it looks like it falls back to 1.1. In an HTTP/2/3 request, this may still emit the HTTP/1.x-only header.
|
|
||
| // Any iterable of string chunks works, including generators. | ||
| $chunks = static function (): \Generator { | ||
| foreach (model('LogModel')->findAll() as $log) { |
There was a problem hiding this comment.
Sorry, I forgot to mention this earlier. This example also uses findAll(), so it may load all rows before yielding them.
Maybe a small fixed iterable, or a batched example, would fit the streaming/low-memory point a bit better?
2 participants
Description
This PR adds first-class support for streaming HTTP responses. It introduces
StreamResponsefor responses generated progressively, such as large exports, proxied streams, or token-style output, and buildsSSEResponseon top of it for Server-Sent Events. Streaming responses can be created from the response service withstream()andeventStream().Checklist: