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

Store response data as property of Response object #29

Closed
wants to merge 2 commits into from
Closed

Store response data as property of Response object #29

wants to merge 2 commits into from

Conversation

Renegade334
Copy link

Currently, after the creation of the response object, the only way to access data received from the stream is to construct a buffer and feed into it data received from the response object's 'data' events. This is cumbersome, and the vast vast majority of uses for the client will only necessitate receiving the contents of the response in full once the request has been fully served.

To that end, this changeset stores the data buffer as a property of the Response object, accessible with a getter function as per other response attributes. With this functionality, accessing the HTTP response content is transformed from the complicated process of setting callback handlers to process the creation of the response object and receiving chunked data, to something akin to the following:

$request = $client->request('GET', 'https://github.com/reactphp/http-client/');
$request->on('end', function ($error, $response) {
    // error handling...

    $data = $response->getContent();
    // process the data
});
$request->end();

I will leave this here for more experienced eyes to consider. Feel free to cherry-pick or modify.

@WyriHaximus
Copy link
Member

Love the feature and this will work fine with small files. But when dealing with a ton of files or dealing with a bunch of very big files all their contents get loaded into the memory and there is a good change you run of memory within the limit or actual RAM. (Mileage may vary depending on hardware and configuration.)

@Renegade334
Copy link
Author

Aye. Could be made toggleable, either with an additional Client::request() parameter, or with an option setter along the lines of Response::setSaveResponseContent() which could then be called by a 'response' event listener.

@hiend
Copy link

hiend commented May 18, 2015

$request->once('response', function (Response $response) {
    \React\Stream\BufferedSink::createPromise($response)->then(function ($content) {
        echo $content;
    });
});

@clue
Copy link
Member

clue commented Jan 18, 2018

I'm closing this for now as it hasn't received any input in a while and it's unlikely this will get traction any time soon. The stream semantics have been fixed in recent updates, so buffering the request body is now significantly easier.

The feature request is still open in #48 and we'll look into this in the not too far future 👍 This will also be relevant once we implement PSR-7 interfaces via #41. If you feel this was closed prematurely or want to pick this up again, please let us know and we can reopen this.

Thank you for your effort!

@clue clue closed this Jan 18, 2018
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.

None yet

4 participants