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

Streaming requests #1143

Merged
merged 11 commits into from
Jun 28, 2021
Merged

Streaming requests #1143

merged 11 commits into from
Jun 28, 2021

Conversation

richardm-stripe
Copy link
Contributor

@richardm-stripe richardm-stripe commented Jun 18, 2021

Reviews

r? @dcr-stripe @ob-stripe
cc @stripe/api-libraries

Summary

This PR adds "streaming" support to our HttpClient.

Similar to PRs in stripe-node, stripe-java, stripe-ruby, stripe-python, and stripe-go.

Up until now, all Stripe methods return JSON; stripe-php buffers the JSON into memory, parses it and constructs a StripeObject of the appropriate class before returning it to the user.

We plan soon to release a /pdf method that will violate this pattern. It will return bytes that the user will likely want to write directly to a file or transmit to another server, without buffering into memory.

Towards that end this pr introduces a requestStreaming method.

requestStreaming's implementation passes \CURLOPT_RETURNTRANSFER = false and a callback function
in \CURLOPT_WRITEFUNCTION, that curl will then call with bytes from the body.

requestStreaming exposes a similar interface to the caller. The user passes in a callback $readBodyChunk, which requestStreaming will call with "chunks" from a successful request body as they arrive, before returning.

Example of what the end-user experience will look like:

$foo = FooResource::retrieve('foo');
$fileHandle = \fopen("/tmp/foo.pdf", 'wb');
$readBodyChunk = function ($chunk) use (&$fileHandle) {
    \fwrite($fileHandle, $chunk);
};
$foo->pdf($readBodyChunk);
\fclose($fileHandle);

or see the test

I think in an ideal world, we'd return a PSR-7-compatible "stream" interface instead -- users have requested our responses be more PSR-7 compatible in the past. This, though, is the approach I think is most compatible with our current CurlClient implementation.

@richardm-stripe richardm-stripe force-pushed the richardm-binary-streaming branch 6 times, most recently from 300df71 to 10f05be Compare June 21, 2021 19:15
Copy link
Contributor

@dcr-stripe dcr-stripe left a comment

Choose a reason for hiding this comment

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

Overall I think this looks good and makes sense. Left a few comments. Thanks for putting together the test infrastructure! Curious what @ob-stripe thinks as well.

tests/TestServer.php Outdated Show resolved Hide resolved
lib/HttpClient/CurlClient.php Outdated Show resolved Hide resolved
lib/HttpClient/CurlClient.php Show resolved Hide resolved
tests/Stripe/HttpClient/CurlClientTest.php Outdated Show resolved Hide resolved
tests/TestServer.php Show resolved Hide resolved
tests/TestServer.php Outdated Show resolved Hide resolved
lib/HttpClient/CurlClient.php Outdated Show resolved Hide resolved
tests/Stripe/HttpClient/CurlClientTest.php Outdated Show resolved Hide resolved
@richardm-stripe richardm-stripe force-pushed the richardm-binary-streaming branch 3 times, most recently from 3c911af to ee83267 Compare June 26, 2021 00:17
@richardm-stripe richardm-stripe changed the title [WIP] binary streaming support Streaming requests Jun 26, 2021
@richardm-stripe
Copy link
Contributor Author

richardm-stripe commented Jun 26, 2021

r? @dcr-stripe this is ready for another review - I removed a test case that was unavoidably flaky and addressed feedback from the previous review

Copy link
Contributor

@dcr-stripe dcr-stripe left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks Richard! 🚢

Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

LGTM as well.

Not a huge fan of having to spin up a new process in tests for the test server, but it works and I can't think of a better way.

@richardm-stripe richardm-stripe merged commit 990ff44 into master Jun 28, 2021
@richardm-stripe richardm-stripe deleted the richardm-binary-streaming branch June 28, 2021 16:18
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.

3 participants