Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug44168 #400

Closed
wants to merge 1 commit into from
Closed

Bug44168 #400

wants to merge 1 commit into from

Conversation

m6w6
Copy link
Contributor

@m6w6 m6w6 commented Aug 1, 2013

Leeaving that for cataphract

remove any previous Content-Length header and retain output
compression
@cataphract
Copy link
Contributor

I still maintain that this option is worse than the status quo, as it will break 206 responses. At the very least an exception should be carved out for that, but probably there are other situations where the Content-length field is mandatory (whereas compression is always optional).

@m6w6
Copy link
Contributor Author

m6w6 commented Aug 1, 2013

Where is a Content-Length required? A 206 response surely does not (according to the RFC)

@m6w6
Copy link
Contributor Author

m6w6 commented Aug 1, 2013

That may have sounded harsher than intended!

The RFC says to 206:

    Either a Content-Range header field (section 14.16) indicating
    the range included with this response, or a multipart/byteranges
    Content-Type including Content-Range fields for each part. If a
    Content-Length header field is present in the response, its
    value MUST match the actual number of OCTETs transmitted in the
    message-body.

@cataphract
Copy link
Contributor

You're right, it's my statement in the bug report that's incorrect. I guess I meant "Content-Range", which the example also uses, but then the implementation doesn't really match this description because it looks only for "Content-length". So it should look for that header instead (or for a 206 response). Or better, additionally, because:

I would still say that content-length should inhibit compression instead of compression inhibiting content-length. The first is a header that's set by the programmer, the other is a filter some people activate in php.ini. The header is very important for some applications which need to know ahead of time the size of the response (for display progress, to allow seeking, etc.). If the header is to be removed, this should definitely be limited to master.

@cataphract
Copy link
Contributor

By the way, the bug is this one: https://bugs.php.net/bug.php?id=44164 (44164, not 8).

@m6w6
Copy link
Contributor Author

m6w6 commented Aug 1, 2013

Alright, so why don't we just close the bug? It's still open, though the "agreed" fix is already in the code? I wrote the patch and just noticed afterwards, that your fix was live already for years :)

@cataphract
Copy link
Contributor

Well, the issue was a bit controversial, so it was just left marinating. I'll close it.

@m6w6
Copy link
Contributor Author

m6w6 commented Aug 1, 2013

Thank you! :)

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.

None yet

2 participants