Catch Guzzle parse request errors #65
Conversation
WyriHaximus
changed the title
|
@clue ping 😎 |
| list($request, $bodyBuffer) = $this->parseRequest($this->buffer); | ||
| $this->emit('headers', array($request, $bodyBuffer)); | ||
| } catch (Exception $exception) { | ||
| $this->emit('error', [$exception]); |
There was a problem hiding this comment.
Not sure this should be moved moved here, removing the listeners is not covered by any tests.
I agree that it makes sense to remove all listeners after an error, but we should probably look at line 20 then. Should this be postponed?
This whole block looks a bit unclear to me: catching exceptions from both parsing the request and emitting the headers event and continuing after an error only to remove all listeners.
There was a problem hiding this comment.
Not sure this should be moved moved here, removing the listeners is not covered by any tests.
I'll update the tests tomorrow testing the removal of listeners.
I agree that it makes sense to remove all listeners after an error, but we should probably look at line 20 then. Should this be postponed?
Makes more sense in a separate PR tbh.
This whole block looks a bit unclear to me: catching exceptions from both parsing the request and emitting the
headersevent and continuing after an error only to remove all listeners.
It is controlling of the flow, if parsing the headers fails emitting will not be executed. To clear this up I'll move that to a new function instead.
| $this->assertInstanceOf('InvalidArgumentException', $error); | ||
| $this->assertSame('Invalid message', $error->getMessage()); | ||
| $this->assertSame(0, count($parser->listeners('headers'))); | ||
| $this->assertSame(0, count($parser->listeners('error'))); |
There was a problem hiding this comment.
I think my previous comment still holds: If we check the listeners for this error, we should probably also check the listeners for the other error event, so this provides a consistent interface for any kind of error.
There was a problem hiding this comment.
Still agreeing with your previous comment but I rather make a new PR for that one as it feels related but out of scope for this PR
|
Otherwise this LGTM, perhaps squash this into two concise commits? 👍 |
|
Nah will quash it on merge into one commit, easier 😄 |
As discovered during @clue's talk at Typo3 days https://www.youtube.com/watch?v=q2pVVDZtuJs Guzzle's request parser can throw an exception but we don't catch it. This PR adds catching that exception and emitting it as an error.