From e87301603e9a0bfd4b44814ab5eeb968fbe24a57 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 21 Oct 2019 17:37:25 +0200 Subject: [PATCH] [HttpClient] allow option "buffer" to be a stream resource --- src/Symfony/Component/HttpClient/CHANGELOG.md | 1 + .../Component/HttpClient/CurlHttpClient.php | 6 +- .../Component/HttpClient/HttpClientTrait.php | 26 ++++++++ .../Component/HttpClient/NativeHttpClient.php | 6 +- .../HttpClient/Response/CurlResponse.php | 20 ++++-- .../HttpClient/Response/MockResponse.php | 14 ++-- .../HttpClient/Response/NativeResponse.php | 6 +- .../HttpClient/Tests/HttpClientTestCase.php | 48 -------------- .../HttpClient/Tests/MockHttpClientTest.php | 2 +- .../Component/HttpClient/composer.json | 2 +- .../HttpClient/HttpClientInterface.php | 4 +- .../HttpClient/Test/HttpClientTestCase.php | 64 +++++++++++++++++++ 12 files changed, 126 insertions(+), 73 deletions(-) diff --git a/src/Symfony/Component/HttpClient/CHANGELOG.md b/src/Symfony/Component/HttpClient/CHANGELOG.md index a116ff72ec..7bd03aae38 100644 --- a/src/Symfony/Component/HttpClient/CHANGELOG.md +++ b/src/Symfony/Component/HttpClient/CHANGELOG.md @@ -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 ----- diff --git a/src/Symfony/Component/HttpClient/CurlHttpClient.php b/src/Symfony/Component/HttpClient/CurlHttpClient.php index b02d32bb91..45c4a6ae8b 100644 --- a/src/Symfony/Component/HttpClient/CurlHttpClient.php +++ b/src/Symfony/Component/HttpClient/CurlHttpClient.php @@ -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); diff --git a/src/Symfony/Component/HttpClient/HttpClientTrait.php b/src/Symfony/Component/HttpClient/HttpClientTrait.php index 8df309ebc3..d691638b9f 100644 --- a/src/Symfony/Component/HttpClient/HttpClientTrait.php +++ b/src/Symfony/Component/HttpClient/HttpClientTrait.php @@ -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.'); diff --git a/src/Symfony/Component/HttpClient/NativeHttpClient.php b/src/Symfony/Component/HttpClient/NativeHttpClient.php index b86b7c84eb..a500200aba 100644 --- a/src/Symfony/Component/HttpClient/NativeHttpClient.php +++ b/src/Symfony/Component/HttpClient/NativeHttpClient.php @@ -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); diff --git a/src/Symfony/Component/HttpClient/Response/CurlResponse.php b/src/Symfony/Component/HttpClient/Response/CurlResponse.php index f92731308f..c5f99f99c8 100644 --- a/src/Symfony/Component/HttpClient/Response/CurlResponse.php +++ b/src/Symfony/Component/HttpClient/Response/CurlResponse.php @@ -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; diff --git a/src/Symfony/Component/HttpClient/Response/MockResponse.php b/src/Symfony/Component/HttpClient/Response/MockResponse.php index 59db21cc3f..794ddc314b 100644 --- a/src/Symfony/Component/HttpClient/Response/MockResponse.php +++ b/src/Symfony/Component/HttpClient/Response/MockResponse.php @@ -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; diff --git a/src/Symfony/Component/HttpClient/Response/NativeResponse.php b/src/Symfony/Component/HttpClient/Response/NativeResponse.php index b1585597ec..3e1812db4e 100644 --- a/src/Symfony/Component/HttpClient/Response/NativeResponse.php +++ b/src/Symfony/Component/HttpClient/Response/NativeResponse.php @@ -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) { diff --git a/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php b/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php index 1934d403d7..450544c5cc 100644 --- a/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php +++ b/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php @@ -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(); - } } diff --git a/src/Symfony/Component/HttpClient/Tests/MockHttpClientTest.php b/src/Symfony/Component/HttpClient/Tests/MockHttpClientTest.php index f0721aa8a2..7bc5283548 100644 --- a/src/Symfony/Component/HttpClient/Tests/MockHttpClientTest.php +++ b/src/Symfony/Component/HttpClient/Tests/MockHttpClientTest.php @@ -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()); diff --git a/src/Symfony/Component/HttpClient/composer.json b/src/Symfony/Component/HttpClient/composer.json index 5a5512a3e7..85fd09368c 100644 --- a/src/Symfony/Component/HttpClient/composer.json +++ b/src/Symfony/Component/HttpClient/composer.json @@ -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": { diff --git a/src/Symfony/Contracts/HttpClient/HttpClientInterface.php b/src/Symfony/Contracts/HttpClient/HttpClientInterface.php index b9ae4c658b..2a89486d1b 100644 --- a/src/Symfony/Contracts/HttpClient/HttpClientInterface.php +++ b/src/Symfony/Contracts/HttpClient/HttpClientInterface.php @@ -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 diff --git a/src/Symfony/Contracts/HttpClient/Test/HttpClientTestCase.php b/src/Symfony/Contracts/HttpClient/Test/HttpClientTestCase.php index 11ba1ae6d2..b279fe76c4 100644 --- a/src/Symfony/Contracts/HttpClient/Test/HttpClientTestCase.php +++ b/src/Symfony/Contracts/HttpClient/Test/HttpClientTestCase.php @@ -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__);