Fix headers to be handled case insensitive and ignore 100-continue for HTTP/1.0 #107
Conversation
| $http->on('request', function (Request $request, Response $response) { | ||
| if ($request->expectsContinue()) { | ||
| $response->writeContinue(); | ||
| } |
There was a problem hiding this comment.
How can I access the actual request body then? I think this should be handled transparently.
There was a problem hiding this comment.
This only adds documentation for the existing behavior, accessing the request body is not part of this 👍 (see also #104 and #105)
That being said and given the current architecture, I think it's very reasonable to handle this explicitly. The documentation explicitly states why the consumer of this lib is in control of this behavior. (see also #47)
Note that I'm not saying this has to stay that way, but I'd rather discuss this in a separate feature ticket instead (BC break ahead). Do you feel like filing one? 👍
There was a problem hiding this comment.
If I write code like documented now, how does reading the request body work then? Do I have to find it out manually and discard the first expect 100 request?
There was a problem hiding this comment.
See the existing and the updated documentation: The Request implements ReadableStreamInterface, so you can read the request body by awaiting the data events and wait for a final end event.
Note that this is somewhat limited until #104 is resolved, which is something I'm currently looking into with @legionth 👍
Note that this is actually only a single request, so nothing is being "discarded" here.
There was a problem hiding this comment.
@clue Remembered it wrong, thought expect-100 would send the request headers again then.
| array('X-Powered-By' => 'React/alpha'), | ||
| $headers | ||
| ); | ||
| } |
There was a problem hiding this comment.
This should probably update $lower, too?
There was a problem hiding this comment.
The $lower is only used within this method, so it's not needed anymore after this assignment.
src/Request.php
Outdated
| public function expectsContinue() | ||
| { | ||
| return isset($this->headers['Expect']) && '100-continue' === $this->headers['Expect']; | ||
| return '100-continue' === $this->getHeaderLine('Expect'); |
There was a problem hiding this comment.
This needs a strtolower call for $this->getHeaderLine("expect"), see https://tools.ietf.org/html/rfc7231#section-5.1.1
The Expect field-value is case-insensitive.
There was a problem hiding this comment.
This should also check the protocol version, as it must return false for HTTP/1.0.
A server that receives a 100-continue expectation in an HTTP/1.0 request MUST ignore that expectation.
|
Ping, afaict everything's been resolved |
clue
changed the title
writeHead() and writeContinue()
4 participants
This simple PR makes sure we always match header names case insensitive.
Also adds some documentation for the existing methods.
Builds on top of #103
Fixes / closes #51
Fixes / closes #36
Supersedes / closes #54
Supersedes #53
Supersedes #47