Support buffering request body (RequestBodyBufferMiddleware) #216
Conversation
My main concern with this PR is lack of documentation, looks like some of this somehow ended up in
tests/Middleware/BufferTest.php
Outdated
| public function testToLargeBody() | ||
| { | ||
| $size = $this->iniMaxPostSize() + 1; | ||
| $stream = new BufferStream($size); |
There was a problem hiding this comment.
Not sure what's supposed to be tested here? Afaict this should probably use a fixed limit instead?
src/Middleware/Buffer.php
Outdated
|
|
||
| if ($size === null) { | ||
| return new Response(411, array('Content-Type' => 'text/plain'), 'No Content-Length header given'); | ||
| } |
There was a problem hiding this comment.
Makes perfect sense to me, but we should probably add some documentation and tests for this 👍
For the reference: I've discussed this with @WyriHaximus and I think it's save to say we all agree that it may make sense to also support incoming requests with Transfer-Encoding: chunked in a future version (follow-up PR) 👍 This feature is rarely used in the real world and not something that will be needed for most people (unlike outgoing responses with Transfer-Encoding: chunked which are supported).
src/Middleware/Buffer.php
Outdated
| { | ||
| private $sizeLimit; | ||
|
|
||
| public function __construct($sizeLimit = null) |
There was a problem hiding this comment.
Could use some documentation about the expected parameter type 👍
src/Middleware/Buffer.php
Outdated
| use React\Stream\ReadableStreamInterface; | ||
| use RingCentral\Psr7\BufferStream; | ||
|
|
||
| final class Buffer |
There was a problem hiding this comment.
Not sure about the name, as new Buffer() doesn't really convey its responsibility? How about something like RequestBufferer (which doesn't sound perfect either).
There was a problem hiding this comment.
RequestBodyBuffer could work.
There was a problem hiding this comment.
Sounds much better to me, so I won't block this 👍
To be this name still implies this class represents the buffer itself, while it is actually (only) responsible for the buffering process and storing the complete buffer back into the request instance that will be passed upstream.
I guess this class could use some documentation? :-)
src/Middleware/RequestBodyBuffer.php
Outdated
| { | ||
| private $sizeLimit; | ||
|
|
||
| public function __construct($sizeLimit = null) |
There was a problem hiding this comment.
Could use some documentation about the expected parameter type 👍
| public function testToLargeBody() | ||
| { | ||
| $size = $this->iniMaxPostSize() + 1; | ||
| $stream = new BufferStream($size); |
There was a problem hiding this comment.
Not sure what's supposed to be tested here? Afaict this should probably use a fixed limit instead?
There was a problem hiding this comment.
Replaced that tests with one specifically for the 413 error
src/Middleware/RequestBodyBuffer.php
Outdated
|
|
||
| if ($size === null) { | ||
| return new Response(411, array('Content-Type' => 'text/plain'), 'No Content-Length header given'); | ||
| } |
There was a problem hiding this comment.
Makes perfect sense to me, but we should probably add some documentation and tests for this 👍
There was a problem hiding this comment.
And added docs for it
src/Middleware/RequestBodyBuffer.php
Outdated
| @@ -61,6 +77,6 @@ private function iniMaxPostSize() | |||
| return substr($size, 0, -1) * 1024 * 1024 * 1024; | |||
| } | |||
|
|
|||
| return $size; | |||
| throw new \UnexpectedValueException('post_max_size value is out of range.'); | |||
There was a problem hiding this comment.
This will now throw for a size specified in bytes (without a suffix)
There was a problem hiding this comment.
Good point, I've reverted it
|
Thanks! Ping @clue :P |
clue
force-pushed
the
middleware-buffer
branch
from
949b061 to
0bbc802
Compare
clue
force-pushed
the
middleware-buffer
branch
from
0bbc802 to
4c4e950
Compare
clue
force-pushed
the
middleware-buffer
branch
from
4c4e950 to
c8d66b8
Compare
clue
changed the title
Buffer Middleware split off from #213
Assumes to be used with #215