Remove all listeners after emitting error in RequestHeaderParser #68
Conversation
|
Ping @clue |
src/RequestHeaderParser.php
Outdated
| $this->parseAndEmitRequest(); | ||
| } catch (Exception $exception) { | ||
| $this->emit('error', [$exception]); | ||
| $this->emit('error', [$exception, $this]); |
There was a problem hiding this comment.
Not a fan of this tbh. This should probably be discussed in the Stream component instead, but this messes with forwarding events.
There was a problem hiding this comment.
What would you suggest? Remove it here and from line 21, which would create a minor BC break. Or something else?
There was a problem hiding this comment.
I'm undecided tbh.
This change in itself looks okay to me, but I assume we may wan to remove these in an upcoming stream release anyway. This would obviously create a (minor) BC break, so this is not something that's going to happen soon anyway.
Let's put it this way: Has this line caused any issue for you? Do you mind reverting this line (and tests accordingly) and possibly PR title?
There was a problem hiding this comment.
Tbh I have no problem reverting this line and associated tests. This line hasn't caused any issues. I'll revert it 👍
|
Not sure why this PR isn't seeing my latest changes. Contacting github |
|
Turned out I committed it to my fork instead of the original repo here 😊 |
clue
left a comment
clue
left a comment
There was a problem hiding this comment.
PR title should be updated to reflect what this now does, otherwise LGTM 👍
WyriHaximus
changed the title
…ctphp#68) * Ensure removeAllListeners on all error event in RequestHeaderParser * Ensure all error events from RequestHeaderParser emit $this as second item in event * Reverted emitting $this with error
Follow up on @clue's comment here: #65 (comment)