Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
0e88cc5
Streaming body parsers foundation
WyriHaximus Sep 27, 2016
758ad27
Added documentation to the parser interface and added cancel method t…
WyriHaximus Sep 29, 2016
304611f
Updated the raw body parser to comply wth the new parser interface
WyriHaximus Sep 29, 2016
dee32e3
Updated raw body parser test
WyriHaximus Sep 29, 2016
1ae613f
No body parser
WyriHaximus Sep 29, 2016
2dcee0f
Updated the factory, without the no body parser this PR didn't really…
WyriHaximus Sep 29, 2016
68340bb
No body parser tests
WyriHaximus Sep 29, 2016
d089c56
Changing then to done: https://github.com/reactphp/http/pull/69#discu…
WyriHaximus Oct 25, 2016
75aac07
Mark raw body parser as internal
WyriHaximus Oct 25, 2016
7a1414a
Mark no body parser as internal
WyriHaximus Oct 25, 2016
2c40605
Throw exception when content-length is missing
WyriHaximus Oct 25, 2016
a2083d2
NoBodyParser is gone as it will serve no purpose now that RawBodyPars…
WyriHaximus Oct 27, 2016
0afcef1
Clarified that the body is emitted in chunks
WyriHaximus Oct 27, 2016
8810190
Removed NoBodyParser from factory
WyriHaximus Oct 27, 2016
fe58c44
Transformed RawBodyParser to a parser that emits data chunks as they …
WyriHaximus Oct 27, 2016
5ae429e
Followed @jsor's suggestion to drop the factory method in favour of a…
WyriHaximus Nov 15, 2016
b3436bb
Factory create documentation
WyriHaximus Dec 16, 2016
f18d114
RawBodyParser::cancel docblock
WyriHaximus Dec 16, 2016
e6d5a16
Marked RawBodyParser final, it shouldn't be extended but wrapped/deco…
WyriHaximus Dec 16, 2016
2dacc3d
Added a expected arguments to streaming parser interface
WyriHaximus Dec 16, 2016
76fd4cb
Updated readme with request body parser as example
WyriHaximus Dec 16, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/StreamingBodyParser/Factory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?php

namespace React\Http\StreamingBodyParser;

use React\Http\Request;

class Factory
{
/**
* @param Request $request
* @return ParserInterface
*/
public static function create(Request $request)
{
return new RawBodyParser($request);
}
}
11 changes: 11 additions & 0 deletions src/StreamingBodyParser/ParserInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

namespace React\Http\StreamingBodyParser;

use Evenement\EventEmitterInterface;
use React\Http\Request;

interface ParserInterface extends EventEmitterInterface
{
public function __construct(Request $request);

Choose a reason for hiding this comment

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

You have Factory method, so this antipattern __construct don't need to be in interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 😄 .

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the PR, with use a factory method to create parsers

Choose a reason for hiding this comment

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

Looks much better for me!

Copy link
Member

Choose a reason for hiding this comment

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

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.

}
34 changes: 34 additions & 0 deletions src/StreamingBodyParser/RawBodyParser.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

namespace React\Http\StreamingBodyParser;

use Evenement\EventEmitterTrait;
use React\Http\Request;

class RawBodyParser implements ParserInterface
{
use EventEmitterTrait;

/**
* @param Request $request
*/
public function __construct(Request $request)
{
$headers = $request->getHeaders();
$headers = array_change_key_case($headers, CASE_LOWER);

ContentLengthBufferedSink::createPromise(
$request,
$headers['content-length']
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

)->then([$this, 'finish']);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changing it to be ^2.2

}

/**
* @param string $buffer
*/
public function finish($buffer)
{
$this->emit('body', [$buffer]);
$this->emit('end');
}
}
19 changes: 19 additions & 0 deletions tests/StreamingBodyParser/FactoryTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php

namespace React\Tests\Http\StreamingBodyParser;

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

class FactoryTest extends TestCase
{
public function testNoContentType()
{
$request = new Request('POST', 'http://example.com/', [], 1.1, [
'content-length' => 123,
]);
$parser = Factory::create($request);
$this->assertInstanceOf('React\Http\StreamingBodyParser\RawBodyParser', $parser);
}
}
24 changes: 24 additions & 0 deletions tests/StreamingBodyParser/RawBodyParserTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

namespace React\Tests\Http\StreamingBodyParser;

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

class RawBodyParserTest extends TestCase
{
public function testNoContentLength()
{
$body = '';
$request = new Request('POST', 'http://example.com/', [], 1.1, [
'content-length' => 3,
]);
$parser = new RawBodyParser($request);
$parser->on('body', function ($rawBody) use (&$body) {
$body = $rawBody;
});
$request->emit('data', ['abc']);
$this->assertSame('abc', $body);
}
}