Skip to content

Conversation

@legionth
Copy link
Contributor

@legionth legionth commented Mar 3, 2017

Currently the complete header will be emitted to the request event. The request event will always receive decoded data. So at this point there is no need to keep the header lines.

Builds on top of #116

src/Server.php Outdated
$stream = new LengthLimitedStream($stream, $contentLength);
}

$request = $request->withoutHeader('Content-Length');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the point in always removing this? Isn't this something (for example) a middleware might be interested in?

Also, I don't see this documented anywhere, in particular not the WHY?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some lines to the README and changed the code. The 'Content-Length' will now only be removed if also 'Transfer-Encoding' is given.

@legionth legionth changed the title Remove 'Transfer-Encoding' and 'Content-Length' header before emitting the request event Remove 'Transfer-Encoding' before emitting to the request event Mar 3, 2017
@clue clue added this to the v0.6.0 milestone Mar 3, 2017
@clue
Copy link
Member

clue commented Mar 3, 2017

Makes perfect sense now 👍

README.md Outdated

The `Request` class is responsible for streaming the incoming request body
and contains meta data which was parsed from the request headers.
If the request body is chunked-encoded, the data will be decoded an emitted on the data event.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: and emitted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eyes 👍 , fixed it.

@legionth legionth force-pushed the remove-header-for-request branch from 3f1c927 to bcfbce1 Compare March 4, 2017 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants