feature #34051 [HttpClient] allow option "buffer" to be a stream resource (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpClient] allow option "buffer" to be a stream resource

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Commits
-------

e87301603e [HttpClient] allow option "buffer" to be a stream resource
This commit is contained in:
Nicolas Grekas 2019-10-24 15:27:54 +02:00
commit c5024bdbd0
12 changed files with 126 additions and 73 deletions

View File

@ -14,6 +14,7 @@ CHANGELOG
* made `Psr18Client` implement relevant PSR-17 factories and have streaming responses
* added `TraceableHttpClient`, `HttpClientDataCollector` and `HttpClientPass` to integrate with the web profiler
* allow enabling buffering conditionally with a Closure
* allow option "buffer" to be a stream resource
4.3.0
-----

View File

@ -37,9 +37,7 @@ final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface
use HttpClientTrait;
use LoggerAwareTrait;
private $defaultOptions = [
'buffer' => null, // bool|\Closure - a boolean or a closure telling if the response should be buffered based on its headers
] + self::OPTIONS_DEFAULTS + [
private $defaultOptions = self::OPTIONS_DEFAULTS + [
'auth_ntlm' => null, // array|string - an array containing the username as first value, and optionally the
// password as the second one; or string like username:password - enabling NTLM auth
];
@ -64,7 +62,7 @@ final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface
throw new \LogicException('You cannot use the "Symfony\Component\HttpClient\CurlHttpClient" as the "curl" extension is not installed.');
}
$this->defaultOptions['buffer'] = \Closure::fromCallable([__CLASS__, 'shouldBuffer']);
$this->defaultOptions['buffer'] = $this->defaultOptions['buffer'] ?? \Closure::fromCallable([__CLASS__, 'shouldBuffer']);
if ($defaultOptions) {
[, $this->defaultOptions] = self::prepareRequest(null, null, $defaultOptions, $this->defaultOptions);

View File

@ -42,6 +42,32 @@ trait HttpClientTrait
$options = self::mergeDefaultOptions($options, $defaultOptions, $allowExtraOptions);
$buffer = $options['buffer'] ?? true;
if ($buffer instanceof \Closure) {
$options['buffer'] = static function (array $headers) use ($buffer) {
if (!\is_bool($buffer = $buffer($headers))) {
if (!\is_array($bufferInfo = @stream_get_meta_data($buffer))) {
throw new \LogicException(sprintf('The closure passed as option "buffer" must return bool or stream resource, got %s.', \is_resource($buffer) ? get_resource_type($buffer).' resource' : \gettype($buffer)));
}
if (false === strpbrk($bufferInfo['mode'], 'acew+')) {
throw new \LogicException(sprintf('The stream returned by the closure passed as option "buffer" must be writeable, got mode "%s".', $bufferInfo['mode']));
}
}
return $buffer;
};
} elseif (!\is_bool($buffer)) {
if (!\is_array($bufferInfo = @stream_get_meta_data($buffer))) {
throw new InvalidArgumentException(sprintf('Option "buffer" must be bool, stream resource or Closure, %s given.', \is_resource($buffer) ? get_resource_type($buffer).' resource' : \gettype($buffer)));
}
if (false === strpbrk($bufferInfo['mode'], 'acew+')) {
throw new InvalidArgumentException(sprintf('The stream in option "buffer" must be writeable, mode "%s" given.', $bufferInfo['mode']));
}
}
if (isset($options['json'])) {
if (isset($options['body']) && '' !== $options['body']) {
throw new InvalidArgumentException('Define either the "json" or the "body" option, setting both is not supported.');

View File

@ -35,9 +35,7 @@ final class NativeHttpClient implements HttpClientInterface, LoggerAwareInterfac
use HttpClientTrait;
use LoggerAwareTrait;
private $defaultOptions = [
'buffer' => null, // bool|\Closure - a boolean or a closure telling if the response should be buffered based on its headers
] + self::OPTIONS_DEFAULTS;
private $defaultOptions = self::OPTIONS_DEFAULTS;
/** @var NativeClientState */
private $multi;
@ -50,7 +48,7 @@ final class NativeHttpClient implements HttpClientInterface, LoggerAwareInterfac
*/
public function __construct(array $defaultOptions = [], int $maxHostConnections = 6)
{
$this->defaultOptions['buffer'] = \Closure::fromCallable([__CLASS__, 'shouldBuffer']);
$this->defaultOptions['buffer'] = $this->defaultOptions['buffer'] ?? \Closure::fromCallable([__CLASS__, 'shouldBuffer']);
if ($defaultOptions) {
[, $this->defaultOptions] = self::prepareRequest(null, null, $defaultOptions, $this->defaultOptions);

View File

@ -64,15 +64,21 @@ final class CurlResponse implements ResponseInterface
}
if (null === $content = &$this->content) {
$content = null === $options || true === $options['buffer'] ? fopen('php://temp', 'w+') : null;
$content = null === $options || true === $options['buffer'] ? fopen('php://temp', 'w+') : (\is_resource($options['buffer']) ? $options['buffer'] : null);
} else {
// Move the pushed response to the activity list
$buffer = $options['buffer'];
if ('headers' !== curl_getinfo($ch, CURLINFO_PRIVATE)) {
if ($options['buffer'] instanceof \Closure) {
[$content, $buffer] = [null, $content];
[$content, $buffer] = [$buffer, (bool) $options['buffer']($headers)];
try {
[$content, $buffer] = [null, $content];
[$content, $buffer] = [$buffer, $options['buffer']($headers)];
} catch (\Throwable $e) {
$multi->handlesActivity[$id][] = null;
$multi->handlesActivity[$id][] = $e;
[$content, $buffer] = [$buffer, false];
}
}
if (ftell($content)) {
@ -81,7 +87,9 @@ final class CurlResponse implements ResponseInterface
}
}
if (true !== $buffer) {
if (\is_resource($buffer)) {
$content = $buffer;
} elseif (true !== $buffer) {
$content = null;
}
}
@ -384,8 +392,8 @@ final class CurlResponse implements ResponseInterface
curl_setopt($ch, CURLOPT_PRIVATE, 'content');
try {
if (!$content && $options['buffer'] instanceof \Closure && $options['buffer']($headers)) {
$content = fopen('php://temp', 'w+');
if (!$content && $options['buffer'] instanceof \Closure && $content = $options['buffer']($headers) ?: null) {
$content = \is_resource($content) ? $content : fopen('php://temp', 'w+');
}
} catch (\Throwable $e) {
$multi->handlesActivity[$id][] = null;

View File

@ -107,7 +107,7 @@ class MockResponse implements ResponseInterface
$response->id = ++self::$idSequence;
if (!($options['buffer'] ?? null) instanceof \Closure) {
$response->content = true === ($options['buffer'] ?? true) ? fopen('php://temp', 'w+') : null;
$response->content = true === ($options['buffer'] ?? true) ? fopen('php://temp', 'w+') : (\is_resource($options['buffer']) ? $options['buffer'] : null);
}
$response->initializer = static function (self $response) {
if (null !== $response->info['error']) {
@ -115,8 +115,11 @@ class MockResponse implements ResponseInterface
}
if (\is_array($response->body[0] ?? null)) {
// Consume the first chunk if it's not yielded yet
self::stream([$response])->current();
foreach (self::stream([$response]) as $chunk) {
if ($chunk->isFirst()) {
break;
}
}
}
};
@ -183,9 +186,10 @@ class MockResponse implements ResponseInterface
$response->headers = $chunk[1]->getHeaders(false);
self::readResponse($response, $chunk[0], $chunk[1], $offset);
$multi->handlesActivity[$id][] = new FirstChunk();
$buffer = $response->requestOptions['buffer'] ?? null;
if (($response->requestOptions['buffer'] ?? null) instanceof \Closure) {
$response->content = $response->requestOptions['buffer']($response->headers) ? fopen('php://temp', 'w+') : null;
if ($buffer instanceof \Closure && $response->content = $buffer($response->headers) ?: null) {
$response->content = \is_resource($response->content) ? $response->content : fopen('php://temp', 'w+');
}
} catch (\Throwable $e) {
$multi->handlesActivity[$id][] = null;

View File

@ -51,7 +51,7 @@ final class NativeResponse implements ResponseInterface
$this->info = &$info;
$this->resolveRedirect = $resolveRedirect;
$this->onProgress = $onProgress;
$this->content = true === $options['buffer'] ? fopen('php://temp', 'w+') : null;
$this->content = true === $options['buffer'] ? fopen('php://temp', 'w+') : (\is_resource($options['buffer']) ? $options['buffer'] : null);
$this->shouldBuffer = $options['buffer'] instanceof \Closure ? $options['buffer'] : null;
// Temporary resources to dechunk/inflate the response stream
@ -179,8 +179,8 @@ final class NativeResponse implements ResponseInterface
}
try {
if (null !== $this->shouldBuffer && null === $this->content && ($this->shouldBuffer)($this->headers)) {
$this->content = fopen('php://temp', 'w+');
if (null !== $this->shouldBuffer && null === $this->content && $this->content = ($this->shouldBuffer)($this->headers) ?: null) {
$this->content = \is_resource($this->content) ? $this->content : fopen('php://temp', 'w+');
}
if (!$this->buffer) {

View File

@ -12,7 +12,6 @@
namespace Symfony\Component\HttpClient\Tests;
use Symfony\Component\HttpClient\Exception\ClientException;
use Symfony\Component\HttpClient\Exception\TransportException;
use Symfony\Contracts\HttpClient\Test\HttpClientTestCase as BaseHttpClientTestCase;
abstract class HttpClientTestCase extends BaseHttpClientTestCase
@ -81,51 +80,4 @@ abstract class HttpClientTestCase extends BaseHttpClientTestCase
$response = $client->request('GET', 'http://localhost:8057/404');
$stream = $response->toStream();
}
public function testConditionalBuffering()
{
$client = $this->getHttpClient(__FUNCTION__);
$response = $client->request('GET', 'http://localhost:8057');
$firstContent = $response->getContent();
$secondContent = $response->getContent();
$this->assertSame($firstContent, $secondContent);
$response = $client->request('GET', 'http://localhost:8057', ['buffer' => function () { return false; }]);
$response->getContent();
$this->expectException(TransportException::class);
$this->expectExceptionMessage('Cannot get the content of the response twice: buffering is disabled.');
$response->getContent();
}
public function testReentrantBufferCallback()
{
$client = $this->getHttpClient(__FUNCTION__);
$response = $client->request('GET', 'http://localhost:8057', ['buffer' => function () use (&$response) {
$response->cancel();
}]);
$this->assertSame(200, $response->getStatusCode());
$this->expectException(TransportException::class);
$this->expectExceptionMessage('Response has been canceled.');
$response->getContent();
}
public function testThrowingBufferCallback()
{
$client = $this->getHttpClient(__FUNCTION__);
$response = $client->request('GET', 'http://localhost:8057', ['buffer' => function () {
throw new \Exception('Boo');
}]);
$this->assertSame(200, $response->getStatusCode());
$this->expectException(TransportException::class);
$this->expectExceptionMessage('Boo');
$response->getContent();
}
}

View File

@ -48,7 +48,7 @@ class MockHttpClientTest extends HttpClientTestCase
return new MockHttpClient(function (string $method, string $url, array $options) use ($client) {
try {
// force the request to be completed so that we don't test side effects of the transport
$response = $client->request($method, $url, $options);
$response = $client->request($method, $url, ['buffer' => false] + $options);
$content = $response->getContent(false);
return new MockResponse($content, $response->getInfo());

View File

@ -22,7 +22,7 @@
"require": {
"php": "^7.1.3",
"psr/log": "^1.0",
"symfony/http-client-contracts": "^1.1.7|^2",
"symfony/http-client-contracts": "^1.1.8|^2",
"symfony/polyfill-php73": "^1.11"
},
"require-dev": {

View File

@ -45,7 +45,9 @@ interface HttpClientInterface
// NOT follow except for the initial host name
'http_version' => null, // string - defaults to the best supported version, typically 1.1 or 2.0
'base_uri' => null, // string - the URI to resolve relative URLs, following rules in RFC 3986, section 2
'buffer' => true, // bool - whether the content of the response should be buffered or not
'buffer' => true, // bool|resource|\Closure - whether the content of the response should be buffered or not,
// or a stream resource where the response body should be written,
// or a closure telling if/where the response should be buffered based on its headers
'on_progress' => null, // callable(int $dlNow, int $dlSize, array $info) - throwing any exceptions MUST abort
// the request; it MUST be called on DNS resolution, on arrival of headers and on
// completion; it SHOULD be called on upload/download of data and at least 1/s

View File

@ -87,6 +87,70 @@ abstract class HttpClientTestCase extends TestCase
$response->getContent();
}
public function testBufferSink()
{
$sink = fopen('php://temp', 'w+');
$client = $this->getHttpClient(__FUNCTION__);
$response = $client->request('GET', 'http://localhost:8057', [
'buffer' => $sink,
'headers' => ['Foo' => 'baR'],
]);
$body = $response->toArray();
$this->assertSame('baR', $body['HTTP_FOO']);
rewind($sink);
$sink = stream_get_contents($sink);
$this->assertSame($sink, $response->getContent());
}
public function testConditionalBuffering()
{
$client = $this->getHttpClient(__FUNCTION__);
$response = $client->request('GET', 'http://localhost:8057');
$firstContent = $response->getContent();
$secondContent = $response->getContent();
$this->assertSame($firstContent, $secondContent);
$response = $client->request('GET', 'http://localhost:8057', ['buffer' => function () { return false; }]);
$response->getContent();
$this->expectException(TransportExceptionInterface::class);
$response->getContent();
}
public function testReentrantBufferCallback()
{
$client = $this->getHttpClient(__FUNCTION__);
$response = $client->request('GET', 'http://localhost:8057', ['buffer' => function () use (&$response) {
$response->cancel();
return true;
}]);
$this->assertSame(200, $response->getStatusCode());
$this->expectException(TransportExceptionInterface::class);
$response->getContent();
}
public function testThrowingBufferCallback()
{
$client = $this->getHttpClient(__FUNCTION__);
$response = $client->request('GET', 'http://localhost:8057', ['buffer' => function () {
throw new \Exception('Boo');
}]);
$this->assertSame(200, $response->getStatusCode());
$this->expectException(TransportExceptionInterface::class);
$this->expectExceptionMessage('Boo');
$response->getContent();
}
public function testUnsupportedOption()
{
$client = $this->getHttpClient(__FUNCTION__);