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

Introduce a reusable RequestBody and ensure Body::__toString() rewinds properly #1393

Merged
merged 3 commits into from Aug 2, 2015

Conversation

opengeek
Copy link
Contributor

This introduces a reusable RequestBody implementation that by default copies the php://input stream, which can only be read one time, to a php://temp stream that can be re-wound and re-read.

Also makes sure Body::__toString() does not throw any exceptions.

This should address issue #1386.

@JoeBengalen
Copy link
Contributor

Will stream_copy_to_stream be faster than appending each string being read to a cache property like phly does?
https://github.com/phly/http/blob/master/src/PhpInputStream.php

@opengeek
Copy link
Contributor Author

I tried both approaches and did some profiling with them but could not find any significant difference with small, basic payloads. The approach of keeping it an actual stream rather than caching the content should provide more flexibility within middleware usage however, with only slightly more memory usage. Copying it to the temp stream and discarding the original input stream should theoretically save memory usage however, with larger payloads since it uses temp files after 5Mb of memory is used.

I may try to find a way to test this with some larger payloads to better understand the impact. And now I wonder if I should call this implementation ServerRequestBody.

*
* @param resource|null $stream
*/
public function __construct($stream = null)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to accept a $stream parameter here. This is a specialist class solely for handling php://input

@akrabat
Copy link
Member

akrabat commented Jul 31, 2015

Can we simplify RequestBody to something like:

class RequestBody extends Body
{
    /**
     * Create a new RequestBody.
     */
    public function __construct()
    {
        $stream = fopen('php://temp', 'w+');
        stream_copy_to_stream(fopen('php://input', 'r'), $stream);
        rewind($stream);

        parent::__construct($stream);
    }
}

i.e. what do we gain with your current implementation over this?

This is according to PSR-7 specifications

Partially addresses slimphp#1386
This copies the php://input stream to a php://temp stream that can be rewound and read multiple times so that the raw request body is not cannabilised by the first middleware to read it. Also automatically rewinds the request body after reading it in Request::getMethod() on POST so the “first” call to getContents() within the App invocation is not empty.

Should resolve slimphp#1386
@akrabat akrabat merged commit 91c5678 into slimphp:3.x Aug 2, 2015
@akrabat akrabat added this to the 3.0.0 Beta 2 milestone Aug 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants