Skip to content

Option for ignoring the Content-Range header#23

Open
Stadly wants to merge 1 commit into
laminas:2.1.xfrom
Stadly:patch-1
Open

Option for ignoring the Content-Range header#23
Stadly wants to merge 1 commit into
laminas:2.1.xfrom
Stadly:patch-1

Conversation

@Stadly
Copy link
Copy Markdown

@Stadly Stadly commented Sep 15, 2021

Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC no
QA no

Description

When the response body has already been limited to the requested range, the emitter should ignore the Content-Range header and emit the full response body.

Fixes #22

Comment thread test/Emitter/SapiStreamEmitterTest.php Outdated
@lcobucci
Copy link
Copy Markdown
Member

Based on the discussion in #22, I'd say it makes more sense to provide an alternative implementation instead of modifying the existing one.

I'd be fine with reorganising SapiStreamEmitter to provide some decoration but having a different behaviour based on a boolean constructor parameter isn't a very maintainable/extensible design IMHO.

When the response body has already been limited to the requested range, the emitter should ignore the Content-Range header and emit the full response body.

Signed-off-by: Stadly <magnar@myrtveit.com>
@Stadly
Copy link
Copy Markdown
Author

Stadly commented Sep 16, 2021

Any other opinions? I'm fine with either:

  1. Modify SapiStreamEmitter (as done in this PR).
  2. Create a separate emitter.
  3. Fix the memory issue in SapiEmitter. That would also fix SapiStreamEmitter that does not consider Content-Range #22, as Content-Range is already ignored by SapiEmitter. I think this is my preferred option. Or is there a reason why SapiEmitter converts the whole response body to a string and emits it in a single go instead of emitting it as smaller chunks? With the current implementation of SapiEmitter, the entire response body must be written to memory, which does not work for large responses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SapiStreamEmitter that does not consider Content-Range

3 participants