Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Streaming parser bufferedsink #73

Conversation

WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Oct 6, 2016

Just like the bufferedsink from stream and #70 but this sink takes a streaming body parser and buffers any post field, body data, or uploaded file. Once the full request it in the promise will resolve.

Depends on #69 and #62

  • BufferedSink for streaming parsers
  • Class/Method Documentation
  • Readme Documentation
  • Split up sink in a file, post, and body sink
  • Merge master in when Streaming body parsers foundation #69 and File object #62 are in
  • Ensure tests are up to date and passing
  • Squash on merge

@WyriHaximus WyriHaximus changed the title [WIP] Feature streaming parser bufferedsink [WIP] Streaming parser bufferedsink Oct 8, 2016
@WyriHaximus WyriHaximus mentioned this pull request Oct 8, 2016
8 tasks
self::extractPost($postFields, $key, $value);
});
$parser->on('file', function ($name, File $file) use (&$files) {
StreamBufferedSink::createPromise($file->getStream())->then(function ($buffer) use ($name, $file, &$files) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done instead of then.

@jsor
Copy link
Member

jsor commented Oct 25, 2016

What about splitting this into 3 distinct sinks: a body, post and files sink. The sinks could be composed with parts you need. The resolution array from the current implementation could be achieved with

$promises = [
    'body'  => BodyBufferedSink::createPromise($parser),
    'post'  => PostBufferedSink::createPromise($parser),
    'files' => FilesBufferedSink::createPromise($parser),
];
React\Promise\all($promises)->done($result) {
   // $result contains 'body', 'post' and 'files `entries
});

@WyriHaximus
Copy link
Member Author

That is actually a great idea 👍 !

@WyriHaximus
Copy link
Member Author

@jsor updated the PR with the split up BufferedSink, left the BufferedSink in for reference for now.

use React\Http\Request;
use React\Tests\Http\TestCase;

class PostPostBufferedSinkTest extends TestCase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/PostPost/Post

@jsor
Copy link
Member

jsor commented Oct 26, 2016

Maybe the BufferedSink can stay as

class BufferedSink
{
    /**
     * @param ParserInterface $parser
     * @return PromiseInterface
     */
    public static function createPromise(ParserInterface $parser)
    {
        $promises = [
            'body'  => BodyBufferedSink::createPromise($parser),
            'post'  => PostBufferedSink::createPromise($parser),
            'files' => FilesBufferedSink::createPromise($parser),
        ];

        return Promise\all($promises);
    }
}

@WyriHaximus WyriHaximus changed the base branch from master to 0.5 November 30, 2016 20:34
@clue clue closed this Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants