Streaming body parsers foundation #69
Conversation
|
|
||
| interface ParserInterface extends EventEmitterInterface | ||
| { | ||
| public function __construct(Request $request); |
There was a problem hiding this comment.
You have Factory method, so this antipattern __construct don't need to be in interface.
There was a problem hiding this comment.
Another way of doing this is having a public static factory method that takes a request. But that would just wrap passing the request into the constructor. Let me give it a try to see how it works out, I don't mind either way 😄 .
There was a problem hiding this comment.
Updated the PR, with use a factory method to create parsers
There was a problem hiding this comment.
Since the parser implementations are now marked as @internal, i propose to remove create from the interface since this is now just a additional redundant factory method.
|
I guess this could also use some documentation. What is this used for? When should I use this? Is this only used internally ( |
…o cancel parsing of a request body stream
… make sense in retrospec
|
@clue added documentation on the parser interface and marked certain methods internal |
| $this->promise = ContentLengthBufferedSink::createPromise( | ||
| $request, | ||
| $headers['content-length'] | ||
| )->then([$this, 'finish']); |
There was a problem hiding this comment.
Ideally, this should be done(). Since you're supporting react/promise < ^2.2, i'm not sure if you there should be a method_exists check here.
There was a problem hiding this comment.
Changing it to be ^2.2
|
|
||
| $this->promise = ContentLengthBufferedSink::createPromise( | ||
| $request, | ||
| $headers['content-length'] |
There was a problem hiding this comment.
What happens if there is no (or an invalid)content-length header? I know there is a check in the Factory, but since this class isn't marked as internal, i think there should be an additional check here or in create.
There was a problem hiding this comment.
Marked the class internal (just as NoBodyParser) and throwing an exception in case the header is missing. (In case someone ignores the internal tag.) The value of the header is cast to an integer in the sink forcing it to 0 length in case of an invalid value.
| * @param Request $request | ||
| * @return ParserInterface | ||
| */ | ||
| public static function create(Request $request); |
There was a problem hiding this comment.
Since the parser implementations are now @internal, i think this method can be removed from the interface and the implementations and simple constructor based instantiantion can be used in Factory.
…er is going to emit body events when data comes in and there for there will always be access to the raw body when needed
… public constructor now that parsers are marked @internal at reactphp#69 (comment)
|
Just updated this PR with documentation in the readme and various files |
|
I've talked with @WyriHaximus about this suggestion and I think it makes sense to get this in eventually – but not just yet 👍 I think we agree it makes sense to address PSR-7 support (#28) first. This is something I'm currently looking into with @legionth, so stay tuned.. 😎 |
|
@clue I'd put the priorities differently. If the request parser stuff were there, we could already plug phly/react2psr7 in right now. Might not be the most efficient thing but imho better than waiting? |
Foundation that way for the multipart, form urlencoded, and nobody parser PR's.