Streaming request body parsing #41

Closed
wants to merge 66 commits into
from

Conversation

Projects
None yet
@WyriHaximus
Member

WyriHaximus commented Oct 1, 2015

This PR is the follow up for #13. It started out to make multipart streaming but ended up making all bodies streaming.

The parsers emit a post event with the key and value of a post variable and file on uploaded files found in the request. On the request object getFiles is gone due to the streaming nature of the parsers. getPost is still there but it won't have everything until the entire request has been parsed.

Todo:

  • Normal body streaming
  • Multipart body streaming
  • Form URL Encoded body streaming
  • File object for files in the request
  • Make sure all parsers behave the same
  • Make the form parsers optional
  • Add buffered sink like helpers for request post fields and files
  • Update readme with an example

@WyriHaximus WyriHaximus referenced this pull request Oct 1, 2015

Merged

Multipart handling #13

@nazar-pc

This comment has been minimized.

Show comment
Hide comment
@nazar-pc

nazar-pc Oct 1, 2015

I saw gPsr, are request/response objects PSR-7 compatible now?

nazar-pc commented Oct 1, 2015

I saw gPsr, are request/response objects PSR-7 compatible now?

@WyriHaximus

This comment has been minimized.

Show comment
Hide comment
@WyriHaximus

WyriHaximus Oct 1, 2015

Member

Not yet, those are also in the works though. But this is a step in that direction. The gPsr you're referring to is only used to parse request headers.

Member

WyriHaximus commented Oct 1, 2015

Not yet, those are also in the works though. But this is a step in that direction. The gPsr you're referring to is only used to parse request headers.

@clue

This comment has been minimized.

Show comment
Hide comment
@clue

clue Mar 19, 2016

Member

The parsers emit a post event with the key and value of a post variable and file on uploaded files found in the request.

The HTTP message body can potentially have any size (even single fields can be huge), do we really want to store this automatically?

As an alternative, nodejs only exposes the body stream and leaves it up to the consumer to pass this stream to the correct parser (for example http://stackoverflow.com/questions/4295782/how-do-you-extract-post-data-in-node-js).

Member

clue commented Mar 19, 2016

The parsers emit a post event with the key and value of a post variable and file on uploaded files found in the request.

The HTTP message body can potentially have any size (even single fields can be huge), do we really want to store this automatically?

As an alternative, nodejs only exposes the body stream and leaves it up to the consumer to pass this stream to the correct parser (for example http://stackoverflow.com/questions/4295782/how-do-you-extract-post-data-in-node-js).

@WyriHaximus

This comment has been minimized.

Show comment
Hide comment
@WyriHaximus

WyriHaximus Mar 19, 2016

Member

The HTTP message body can potentially have any size (even single fields can be huge), do we really want to store this automatically?

No I want to go nodes way where a string or stream is emitted? Those can just be gathered using the buffered sink. Not sure yet about the stream part. It would result in less overhead for small fields but more code on the implementing side.

Member

WyriHaximus commented Mar 19, 2016

The HTTP message body can potentially have any size (even single fields can be huge), do we really want to store this automatically?

No I want to go nodes way where a string or stream is emitted? Those can just be gathered using the buffered sink. Not sure yet about the stream part. It would result in less overhead for small fields but more code on the implementing side.

@clue

This comment has been minimized.

Show comment
Hide comment
@clue

clue Mar 19, 2016

Member

My personal vote here would be small, independent, composable parts instead of convenience built in.

Composable parts enable convenience on a higher level, such as this draft API:

$http->on('request', function (Request $request, Response $response) use ($formParser) {
    $formParser->parseDeferredStream($request)->then(function ($fields) use ($response) {
       $response->end('hello ' . $field['name']);
   }, function ($error) use ($response) {
       $response->writeHead(400);
   });
});

Once we look into PSR-7 support, we could probably build a convenient middleware around this concept in order to make this available to each request handler.

Member

clue commented Mar 19, 2016

My personal vote here would be small, independent, composable parts instead of convenience built in.

Composable parts enable convenience on a higher level, such as this draft API:

$http->on('request', function (Request $request, Response $response) use ($formParser) {
    $formParser->parseDeferredStream($request)->then(function ($fields) use ($response) {
       $response->end('hello ' . $field['name']);
   }, function ($error) use ($response) {
       $response->writeHead(400);
   });
});

Once we look into PSR-7 support, we could probably build a convenient middleware around this concept in order to make this available to each request handler.

@WyriHaximus

This comment has been minimized.

Show comment
Hide comment
@WyriHaximus

WyriHaximus Mar 19, 2016

Member

That looks good, I'm assuming that is just listening to form events emitted from $request. Or are you suggesting that $request just emits data and the form parser listens into that handling the parsing. Do like that even more 👍 . Should be fairly easy to do as well, I'll make sure both parsers behave the before refactoring into that

Member

WyriHaximus commented Mar 19, 2016

That looks good, I'm assuming that is just listening to form events emitted from $request. Or are you suggesting that $request just emits data and the form parser listens into that handling the parsing. Do like that even more 👍 . Should be fairly easy to do as well, I'll make sure both parsers behave the before refactoring into that

@WyriHaximus WyriHaximus referenced this pull request Mar 20, 2016

Closed

Tag new version #52

@clue

This comment has been minimized.

Show comment
Hide comment
@clue

clue Mar 20, 2016

Member

suggesting that $request just emits data and the form parser listens into that handling the parsing. Do like that even more 👍

This exactly 👍

Should be fairly easy to do as well […]

Yeah, I too suppose this should be easier than auto-wiring all parsers 👍

Member

clue commented Mar 20, 2016

suggesting that $request just emits data and the form parser listens into that handling the parsing. Do like that even more 👍

This exactly 👍

Should be fairly easy to do as well […]

Yeah, I too suppose this should be easier than auto-wiring all parsers 👍

@WyriHaximus

This comment has been minimized.

Show comment
Hide comment
@WyriHaximus

WyriHaximus Mar 20, 2016

Member

Thanks for clarifying that @clue, working on that refactor right now 👍

Member

WyriHaximus commented Mar 20, 2016

Thanks for clarifying that @clue, working on that refactor right now 👍

@WyriHaximus

This comment has been minimized.

Show comment
Hide comment
@WyriHaximus

WyriHaximus Mar 20, 2016

Member

@clue what I'm working out now would have this API approximately:

$http->on('request', function (Request $request, Response $response) {
    FormParserFactory::create($request)->deferredStream()->then(function ($fields) use ($response) {
       $response->end('hello ' . $field['name']);
   }, function ($error) use ($response) {
       $response->writeHead(400);
   });
});

Currently decoupling all that auto-wiring code

Member

WyriHaximus commented Mar 20, 2016

@clue what I'm working out now would have this API approximately:

$http->on('request', function (Request $request, Response $response) {
    FormParserFactory::create($request)->deferredStream()->then(function ($fields) use ($response) {
       $response->end('hello ' . $field['name']);
   }, function ($error) use ($response) {
       $response->writeHead(400);
   });
});

Currently decoupling all that auto-wiring code

@WyriHaximus WyriHaximus added this to the v0.4.2 milestone Mar 21, 2016

@WyriHaximus WyriHaximus self-assigned this Mar 21, 2016

@WyriHaximus

This comment has been minimized.

Show comment
Hide comment
@WyriHaximus

WyriHaximus Mar 22, 2016

Member

The last few commits remove the right wiring between the form parsers and request parser. They also add a form parser factory. Next step is adding methods like deferredStream to them.

Member

WyriHaximus commented Mar 22, 2016

The last few commits remove the right wiring between the form parsers and request parser. They also add a form parser factory. Next step is adding methods like deferredStream to them.

src/FormParserFactory.php
+ $headers = $request->getHeaders();
+
+ if (!array_key_exists('Content-Type', $headers)) {
+ }

This comment has been minimized.

@clue

clue Mar 22, 2016

Member

?

This comment has been minimized.

@WyriHaximus

WyriHaximus Mar 22, 2016

Member

Left over/Use to return early once I figured out what to return when we can't detect a parser to use.

@WyriHaximus

WyriHaximus Mar 22, 2016

Member

Left over/Use to return early once I figured out what to return when we can't detect a parser to use.

src/FormParserFactory.php
+{
+ public static function create(Request $request)
+ {
+ $headers = $request->getHeaders();

This comment has been minimized.

@clue

clue Mar 22, 2016

Member

http://php.net/array-change-key-case() here to match case insensitive?

@clue

clue Mar 22, 2016

Member

http://php.net/array-change-key-case() here to match case insensitive?

This comment has been minimized.

@WyriHaximus

WyriHaximus Mar 22, 2016

Member

Or #53, but array_change_key_case would work in there for now

@WyriHaximus

WyriHaximus Mar 22, 2016

Member

Or #53, but array_change_key_case would work in there for now

This comment has been minimized.

src/Parser/FormUrlencoded.php
+ ) {
+ $this->buffer = substr($this->buffer, 0, $this->request->getHeaders()['Content-Length']);
+ $this->finish();
+ }

This comment has been minimized.

@clue

clue Mar 22, 2016

Member

Is this needed here? If so, how about doing so once in the constructor?

@clue

clue Mar 22, 2016

Member

Is this needed here? If so, how about doing so once in the constructor?

This comment has been minimized.

@marcj

marcj Mar 22, 2016

How about a if (isset($this->request->getHeaders()['Content-Length']) && ...? I think its more common than using array_key_exists, should also be faster.

@marcj

marcj Mar 22, 2016

How about a if (isset($this->request->getHeaders()['Content-Length']) && ...? I think its more common than using array_key_exists, should also be faster.

This comment has been minimized.

@WyriHaximus

WyriHaximus Mar 23, 2016

Member

Fixed the array_key_exists usage and will move content size header detection to the constructor later.

@WyriHaximus

WyriHaximus Mar 23, 2016

Member

Fixed the array_key_exists usage and will move content size header detection to the constructor later.

src/Parser/FormUrlencoded.php
+ parse_str(trim($this->buffer), $result);
+ foreach ($result as $key => $value) {
+ $this->request->emit('post', [$key, $value]);
+ }

This comment has been minimized.

@clue

clue Mar 22, 2016

Member

If its only responsibility is buffering the stream and then calling the parser once, how about using a BufferedStreamSink instead?

@clue

clue Mar 22, 2016

Member

If its only responsibility is buffering the stream and then calling the parser once, how about using a BufferedStreamSink instead?

This comment has been minimized.

@WyriHaximus

WyriHaximus Mar 22, 2016

Member

Want to refactor this into streaming as well, haven't figured out the parsing for it yet

@WyriHaximus

WyriHaximus Mar 22, 2016

Member

Want to refactor this into streaming as well, haven't figured out the parsing for it yet

This comment has been minimized.

@clue

clue Mar 22, 2016

Member

I think there's some use for parsing a multi-megabyte string here. However, the 80% use case is probably accessing these parsed fields from an array anyway. In this case, my vote would be to merely limit the stream size, then buffer its contents and invoke a simple function here instead.

Afaict this also appears similar to how PHP's SAPI handles this (post_max_size etc.).

A streaming parser can probably be implemented at a later stage (if still deemed useful).

@clue

clue Mar 22, 2016

Member

I think there's some use for parsing a multi-megabyte string here. However, the 80% use case is probably accessing these parsed fields from an array anyway. In this case, my vote would be to merely limit the stream size, then buffer its contents and invoke a simple function here instead.

Afaict this also appears similar to how PHP's SAPI handles this (post_max_size etc.).

A streaming parser can probably be implemented at a later stage (if still deemed useful).

This comment has been minimized.

@WyriHaximus

WyriHaximus Mar 23, 2016

Member

Fair enough, lets do this in a follow up PR 👍

@WyriHaximus

WyriHaximus Mar 23, 2016

Member

Fair enough, lets do this in a follow up PR 👍

src/Request.php
+
+ $this->on('post', function ($key, $value) {
+ $this->addPost($key, $value);
+ });

This comment has been minimized.

@clue

clue Mar 22, 2016

Member

leftover?

@clue

clue Mar 22, 2016

Member

leftover?

This comment has been minimized.

@WyriHaximus

WyriHaximus Mar 22, 2016

Member

Correct

src/FormParserFactory.php
+ return new Multipart($request);
+ }
+
+ if (strtolower($headers['Content-Type']) == 'application/x-www-form-urlencoded') {

This comment has been minimized.

@marcj

marcj Mar 22, 2016

Here we should check with strpos, because 'application/x-www-form-urlencoded' can have ; charset=utf8 as suffix.

@marcj

marcj Mar 22, 2016

Here we should check with strpos, because 'application/x-www-form-urlencoded' can have ; charset=utf8 as suffix.

src/Parser/Multipart.php
+ *
+ * @author jason.gerfen@gmail.com
+ * @author stephane.goetz@onigoetz.ch
+ * @license http://www.gnu.org/licenses/gpl.html GPL License 3

This comment has been minimized.

@clue

clue Mar 22, 2016

Member

👎 The GPL is not compatible with our MIT license.

(FWIW: Looks like this has been introduced as part of #13 originally)

@clue

clue Mar 22, 2016

Member

👎 The GPL is not compatible with our MIT license.

(FWIW: Looks like this has been introduced as part of #13 originally)

This comment has been minimized.

@marcj

marcj Mar 22, 2016

I really don't see anything in common anymore with the linked gist. I think its legit to just throw that @license away as those code files are meanwhile completely different.

@marcj

marcj Mar 22, 2016

I really don't see anything in common anymore with the linked gist. I think its legit to just throw that @license away as those code files are meanwhile completely different.

This comment has been minimized.

@WyriHaximus

WyriHaximus Mar 23, 2016

Member

Does just removing @license gets us out of the woods or do I have to rewrite it to be sure? There are some things that also benefit from certain stream parts in that file.

@WyriHaximus

WyriHaximus Mar 23, 2016

Member

Does just removing @license gets us out of the woods or do I have to rewrite it to be sure? There are some things that also benefit from certain stream parts in that file.

This comment has been minimized.

@WyriHaximus

WyriHaximus Mar 24, 2016

Member

@cboden care to comment on this?

@WyriHaximus

WyriHaximus Mar 24, 2016

Member

@cboden care to comment on this?

This comment has been minimized.

@cboden

cboden Mar 24, 2016

Member

re-write. We can't mix licences and can't steal code that is licensed under GPL.

@cboden

cboden Mar 24, 2016

Member

re-write. We can't mix licences and can't steal code that is licensed under GPL.

This comment has been minimized.

@WyriHaximus

WyriHaximus Mar 24, 2016

Member

A re-write it is, it was introduce via #13. My bad 😞

@WyriHaximus

WyriHaximus Mar 24, 2016

Member

A re-write it is, it was introduce via #13. My bad 😞

@jsor

This comment has been minimized.

Show comment
Hide comment
@jsor

jsor Aug 3, 2016

Member

I know i'm nitpicking 😳

But we now have

  • StreamingBodyParser as the namespace
  • StreamingParserInterface as the interface
  • Parser as the implementation suffix
Member

jsor commented on 57bdc26 Aug 3, 2016

I know i'm nitpicking 😳

But we now have

  • StreamingBodyParser as the namespace
  • StreamingParserInterface as the interface
  • Parser as the implementation suffix

This comment has been minimized.

Show comment
Hide comment
@jsor

jsor Aug 3, 2016

Member

I'd vote for renaming StreamingParserInterface to ParserInterface.

Alternatively it could be StreamingBodyParserInterface and StreamingBodyParsersuffix for the implementations. But that'd result in very long class names.

Member

jsor replied Aug 3, 2016

I'd vote for renaming StreamingParserInterface to ParserInterface.

Alternatively it could be StreamingBodyParserInterface and StreamingBodyParsersuffix for the implementations. But that'd result in very long class names.

@WyriHaximus

This comment has been minimized.

Show comment
Hide comment
@WyriHaximus

WyriHaximus Aug 3, 2016

Member

Tbh I think ParserInterface is good, not to long but still clear on what it is combined with the namespace. StreamingBodyParserInterface and StreamingBodyParsersuffix would be to long IMHO.

Member

WyriHaximus commented Aug 3, 2016

Tbh I think ParserInterface is good, not to long but still clear on what it is combined with the namespace. StreamingBodyParserInterface and StreamingBodyParsersuffix would be to long IMHO.

@jsor

This comment has been minimized.

Show comment
Hide comment
@jsor

jsor Aug 3, 2016

Member

LGTM :shipit:

Btw, absolutely awesome work on this PR ❤️

Member

jsor commented Aug 3, 2016

LGTM :shipit:

Btw, absolutely awesome work on this PR ❤️

README.md
+
+## StreamingBodyParser\Factory and BufferedSink Usage
+
+The `FormParserFactory` parses a request and determines which body parser to use (multipart, formurlencoded, raw body, or no body). Those body parsers emit events on `post` fields, `file` on files, and raw body emits `body` when it received the whole body. `DeferredStream` listens for those events and returns them through a promise when done.

This comment has been minimized.

@jsor

jsor Aug 3, 2016

Member

FormParserFactory -> StreamingBodyParser\Factory
DeferredStream -> StreamingBodyParser\BufferedSink

@jsor

jsor Aug 3, 2016

Member

FormParserFactory -> StreamingBodyParser\Factory
DeferredStream -> StreamingBodyParser\BufferedSink

README.md
@@ -31,3 +31,25 @@ This is an HTTP server which responds with `Hello World` to every request.
$socket->listen(1337);
$loop->run();
```
+
+## StreamingBodyParser\Factory and BufferedSink Usage

This comment has been minimized.

@jsor

jsor Aug 3, 2016

Member

BufferedSink -> StreamingBodyParser\BufferedSink

@jsor

jsor Aug 3, 2016

Member

BufferedSink -> StreamingBodyParser\BufferedSink

README.md
+ $http = new React\Http\Server($socket);
+ $http->on('request', function ($request, $response) {
+ $parser = React\Http\StreamingBodyParser\Factory::create($request);
+ BufferedSink::createPromise($parser)->then(function ($result) use ($response) {

This comment has been minimized.

@jsor

jsor Aug 3, 2016

Member

BufferedSink -> React\Http\StreamingBodyParser\BufferedSink

@jsor

jsor Aug 3, 2016

Member

BufferedSink -> React\Http\StreamingBodyParser\BufferedSink

@clue

This comment has been minimized.

Show comment
Hide comment
@clue

clue Aug 20, 2016

Member

I've discussed this PR with @WyriHaximus and we both would love to get this in rather sooner than later :shipit:

Unfortunately, this currently builds on top of some existing PRs which will be reverted via #59. This means that this PR will result in a huge merge conflict after that :-)

We'll look into getting #59 in first and will then file new PRs that supersede / replace this one. While we can't promise a timeframe, be assured this is currently one of our top priorities 👍

Member

clue commented Aug 20, 2016

I've discussed this PR with @WyriHaximus and we both would love to get this in rather sooner than later :shipit:

Unfortunately, this currently builds on top of some existing PRs which will be reverted via #59. This means that this PR will result in a huge merge conflict after that :-)

We'll look into getting #59 in first and will then file new PRs that supersede / replace this one. While we can't promise a timeframe, be assured this is currently one of our top priorities 👍

@WyriHaximus

This comment has been minimized.

Show comment
Hide comment
@WyriHaximus

WyriHaximus Aug 20, 2016

Member

Yeah Github won't even let me merge it from the site 😝 . This PR is my top ReactPHP priority at the moment, would prefer to get reactphp/http-client#58 in A.S.A.P. so I can fully focus on this right here.

I'll discus with @clue how exactly we're going to cut it up, but since most of the discussion has already taken place here we can move relatively quick

Member

WyriHaximus commented Aug 20, 2016

Yeah Github won't even let me merge it from the site 😝 . This PR is my top ReactPHP priority at the moment, would prefer to get reactphp/http-client#58 in A.S.A.P. so I can fully focus on this right here.

I'll discus with @clue how exactly we're going to cut it up, but since most of the discussion has already taken place here we can move relatively quick

WyriHaximus added a commit that referenced this pull request Aug 30, 2016

@WyriHaximus WyriHaximus referenced this pull request Aug 30, 2016

Closed

File object #62

@clue clue modified the milestones: v0.5, v0.4.2 Sep 13, 2016

@moe123

This comment has been minimized.

Show comment
Hide comment
@moe123

moe123 Sep 24, 2016

Hello gents,

if I may, some remarks about the adopted design :

Input:
-- Any request may have query parameters HEAD, PUT, POST, DELETE and GET... (1)
-- It will make sense to me to have an onEvent taking the HTTP verb as filter not "request"
-- You are inlining the buffer of uploaded files (Apply to BODY and PUT) (2)
-- What happens if the Content-Length header is missing? or set to an offset out of range? bad things.

  • (1) Thus, you are forcing the user to juggle between one to another object representation to get everything.
  • (2) which bars you to handle large file ; also a security threat. In my opinion, it would be better to redirect the raw-input to a FIFO file session then carefully extract and parse per chunk from there ; everything in this safe space, even if the FS is involved ; the full in memory solution is an utopia.

Suggestions :
you should only pre-parse in memory the part-offsets from the FIFO file space ; then give the result totally parsed on demand ; files should be carefully extracted per chunk-unit-buffer (let's say 1024 or 2048) to their destination if the user request it, not inlined in memory.

Output:
Same concepts apply to the response object (streamable and not buffered) i.e handling file-downloads, ranged-streams or large body-response without starving the machine.

Regards.

moe123 commented Sep 24, 2016

Hello gents,

if I may, some remarks about the adopted design :

Input:
-- Any request may have query parameters HEAD, PUT, POST, DELETE and GET... (1)
-- It will make sense to me to have an onEvent taking the HTTP verb as filter not "request"
-- You are inlining the buffer of uploaded files (Apply to BODY and PUT) (2)
-- What happens if the Content-Length header is missing? or set to an offset out of range? bad things.

  • (1) Thus, you are forcing the user to juggle between one to another object representation to get everything.
  • (2) which bars you to handle large file ; also a security threat. In my opinion, it would be better to redirect the raw-input to a FIFO file session then carefully extract and parse per chunk from there ; everything in this safe space, even if the FS is involved ; the full in memory solution is an utopia.

Suggestions :
you should only pre-parse in memory the part-offsets from the FIFO file space ; then give the result totally parsed on demand ; files should be carefully extracted per chunk-unit-buffer (let's say 1024 or 2048) to their destination if the user request it, not inlined in memory.

Output:
Same concepts apply to the response object (streamable and not buffered) i.e handling file-downloads, ranged-streams or large body-response without starving the machine.

Regards.

@WyriHaximus

This comment has been minimized.

Show comment
Hide comment
@WyriHaximus

WyriHaximus Oct 8, 2016

Member

Been spending some time splitting this PR up into smaller ones, that resulted in the following pull requests:
#62: File object
#69: Streaming body parsers foundation
#70: Content length buffered sink
#71: Parser: urlencoded (depends on #69 and #70)
#72: Parser: multipart (depends on #69 and #62)
#73: Streaming parser bufferedsink (depends on #69 and #62)

-----------

#62: Uploaded File object that doesn't make sense on it's own but it provides something needed by #72 and #73.
#69: Provides the base factory and NoBody and RawBody parsers to get started.
#70: Mainly a buffered sink like the one from react/stream but cuts off at a set length or when the stream ends.
#71: Urlencoded parser that requires the base (#69) and the sink (#70).
#72: Multipart parser that requires the file object (#62) and parser factory (#69), will be updated once the factory is in and added to the factory.
#73: Buffered sink that takes a streaming parser and will resolve the promise when the parser is done parsing the incoming request.

-----------

Order of merging (all PR's will be squashed on merge keeping the history clear): #69, #62, #70, #73, #71, #72

-----------

Will go over @moe123's comment carefully and see where adjustment is necessary. One of the things I've already done due to @moe123's is make all the parsers cancelable.

Member

WyriHaximus commented Oct 8, 2016

Been spending some time splitting this PR up into smaller ones, that resulted in the following pull requests:
#62: File object
#69: Streaming body parsers foundation
#70: Content length buffered sink
#71: Parser: urlencoded (depends on #69 and #70)
#72: Parser: multipart (depends on #69 and #62)
#73: Streaming parser bufferedsink (depends on #69 and #62)

-----------

#62: Uploaded File object that doesn't make sense on it's own but it provides something needed by #72 and #73.
#69: Provides the base factory and NoBody and RawBody parsers to get started.
#70: Mainly a buffered sink like the one from react/stream but cuts off at a set length or when the stream ends.
#71: Urlencoded parser that requires the base (#69) and the sink (#70).
#72: Multipart parser that requires the file object (#62) and parser factory (#69), will be updated once the factory is in and added to the factory.
#73: Buffered sink that takes a streaming parser and will resolve the promise when the parser is done parsing the incoming request.

-----------

Order of merging (all PR's will be squashed on merge keeping the history clear): #69, #62, #70, #73, #71, #72

-----------

Will go over @moe123's comment carefully and see where adjustment is necessary. One of the things I've already done due to @moe123's is make all the parsers cancelable.

@WyriHaximus WyriHaximus referenced this pull request in reactphp/react Oct 15, 2016

Closed

Reading post data example #137

@andig

This comment has been minimized.

Show comment
Hide comment
@andig

andig Nov 30, 2016

Contributor

Checking back with how close we are to merging 😮. From what I understand PRs should be fine up to #62 and #69, open comment on #70 and open tasks on #71, #72, #73.

Anything userland could do to get closer to merging apart from friendly nagging?

Contributor

andig commented Nov 30, 2016

Checking back with how close we are to merging 😮. From what I understand PRs should be fine up to #62 and #69, open comment on #70 and open tasks on #71, #72, #73.

Anything userland could do to get closer to merging apart from friendly nagging?

@WyriHaximus

This comment has been minimized.

Show comment
Hide comment
@WyriHaximus

WyriHaximus Nov 30, 2016

Member

@andig yes #62 and #69 are done as far as I'm concerned, unless @jsor or @clue thinks otherwise. And I like to get them in soon, I'll ping them on IRC tonight and see how they look at it.

Once that is in, start working on completing the other PR's. One of the issues I came across is that the urlencoded parser (#71) is going to be interesting as I can't use build in PHP functions to do the parsing without buffering.

Member

WyriHaximus commented Nov 30, 2016

@andig yes #62 and #69 are done as far as I'm concerned, unless @jsor or @clue thinks otherwise. And I like to get them in soon, I'll ping them on IRC tonight and see how they look at it.

Once that is in, start working on completing the other PR's. One of the issues I came across is that the urlencoded parser (#71) is going to be interesting as I can't use build in PHP functions to do the parsing without buffering.

@andig

This comment has been minimized.

Show comment
Hide comment
@andig

andig Dec 2, 2016

Contributor

One of the issues I came across is that the urlencoded parser (#71) is going to be interesting as I can't use build in PHP functions to do the parsing without buffering.

Can't we assume- for the time being- that buffering for this case is ok, i.e. you either have a POST blob which doesn't need decoding or you have urlencoded data that will most likely not exceed a certain size?

Contributor

andig commented Dec 2, 2016

One of the issues I came across is that the urlencoded parser (#71) is going to be interesting as I can't use build in PHP functions to do the parsing without buffering.

Can't we assume- for the time being- that buffering for this case is ok, i.e. you either have a POST blob which doesn't need decoding or you have urlencoded data that will most likely not exceed a certain size?

@WyriHaximus

This comment has been minimized.

Show comment
Hide comment
@WyriHaximus

WyriHaximus Dec 2, 2016

Member

One of the issues I came across is that the urlencoded parser (#71) is going to be interesting as I can't use build in PHP functions to do the parsing without buffering.

Can't we assume- for the time being- that buffering for this case is ok, i.e. you either have a POST blob which doesn't need decoding or you have urlencoded data that will most likely not exceed a certain size?

We could do that, I've set up several milestones that allows us to release this in parts. For example first getting the foundation out in 0.5.0, then multipart in 0.5.1, and then urlencoded parser in 0.5.2.

Member

WyriHaximus commented Dec 2, 2016

One of the issues I came across is that the urlencoded parser (#71) is going to be interesting as I can't use build in PHP functions to do the parsing without buffering.

Can't we assume- for the time being- that buffering for this case is ok, i.e. you either have a POST blob which doesn't need decoding or you have urlencoded data that will most likely not exceed a certain size?

We could do that, I've set up several milestones that allows us to release this in parts. For example first getting the foundation out in 0.5.0, then multipart in 0.5.1, and then urlencoded parser in 0.5.2.

@andig andig referenced this pull request in php-pm/php-pm Dec 21, 2016

Closed

React\Http\RequestParser not found #190

@clue clue referenced this pull request Feb 10, 2017

Closed

Parse request body #105

@clue clue added the new feature label Feb 10, 2017

@clue clue modified the milestones: v0.5.0, v0.8.0 Feb 14, 2017

@bweston92

This comment has been minimized.

Show comment
Hide comment
@bweston92

bweston92 Mar 13, 2017

Any update?

Any update?

@WyriHaximus

This comment has been minimized.

Show comment
Hide comment
@WyriHaximus

WyriHaximus Mar 13, 2017

Member

@bweston92 See this issue for our roadmap: #120

Member

WyriHaximus commented Mar 13, 2017

@bweston92 See this issue for our roadmap: #120

@@ -28,27 +37,23 @@ public function feed($data)
// Extract the header from the buffer
// in case the content isn't complete
- list($headers, $this->buffer) = explode("\r\n\r\n", $this->buffer, 2);
+ list($headers, $buffer) = explode("\r\n\r\n", $this->buffer, 2);

This comment has been minimized.

@kelunik

kelunik May 28, 2017

This might result in a large string operation. Better use the previous strpos and check that against the $this->maxSize before. You might also want to use substr as you already have the position then.

@kelunik

kelunik May 28, 2017

This might result in a large string operation. Better use the previous strpos and check that against the $this->maxSize before. You might also want to use substr as you already have the position then.

@jsor jsor closed this in #226 Oct 2, 2017

@clue clue removed this from the v0.8.0 milestone Nov 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment