From f7e1bf02695f6327ea76404e31b854137f57f4f7 Mon Sep 17 00:00:00 2001 From: Markus Podar Date: Thu, 9 Jul 2020 06:17:35 +0200 Subject: [PATCH] guzzle: use __toString instead of getContent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The problem with getContent is that it _consumes_ the data and multiple invocations will not return the same data, i.e. ```php $response->getBody()->getContents(); // "some data" $response->getBody()->getContents(); // "" ``` Or see this example using Laravels tinker: ``` $ ./artisan tinker Psy Shell v0.10.4 (PHP 7.4.7 — cli) by Justin Hileman >>> $client = new \GuzzleHttp\Client(); => GuzzleHttp\Client {#4150} >>> $response = $client->get('https://github.com'); => GuzzleHttp\Psr7\Response {#4169} >>> strlen($response->getBody()->getContents()); => 134095 >>> strlen($response->getBody()->getContents()); => 0 >>> strlen($response->getBody()->__toString()); => 134095 >>> strlen($response->getBody()->__toString()); => 134095 ``` This _usually_ is not an issue but if you use additional middleware in Guzzle which might already inspect the data stream, it may have advanced already. This is also documented on the `getContents` method (albeit the impact might not be clear), please see https://github.com/php-fig/http-message/blob/efd67d1dc14a7ef4fc4e518e7dee91c271d524e4/src/StreamInterface.php#L137 ```php /** * Returns the remaining contents in a string * * @return string * @throws \RuntimeException if unable to read or an error occurs while * reading. */ public function getContents(); ``` Solution: use `__toString()`, also documented as https://github.com/php-fig/http-message/blob/efd67d1dc14a7ef4fc4e518e7dee91c271d524e4/src/StreamInterface.php#L15 ```php /** * Reads all data from the stream into a string, from the beginning to end. * * This method MUST attempt to seek to the beginning of the stream before * reading data and read the stream until the end is reached. * * Warning: This could attempt to load a large amount of data into memory. * * This method MUST NOT raise an exception in order to conform with PHP's * string casting operations. * * @see http://php.net/manual/en/language.oop5.magic.php#object.tostring * @return string */ public function __toString(); ``` See: - https://www.google.com/search?hl=en&q=guzzle%20getcontents%20empty%20body - https://slides.com/simonpodlipsky/php-http#/21/3/0 Very recommandable to read the slide to understand this --- src/Postmark/Transport.php | 2 +- .../TransportGuzzleStreamConsumptionTest.php | 123 ++++++++++++++++++ 2 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 tests/TransportGuzzleStreamConsumptionTest.php diff --git a/src/Postmark/Transport.php b/src/Postmark/Transport.php index 34be046..1c912bd 100644 --- a/src/Postmark/Transport.php +++ b/src/Postmark/Transport.php @@ -96,7 +96,7 @@ public function send(Swift_Mime_SimpleMessage $message, &$failedRecipients = nul $success = $response->getStatusCode() === 200; - if ($responseEvent = $this->_eventDispatcher->createResponseEvent($this, $response->getBody()->getContents(), $success)) { + if ($responseEvent = $this->_eventDispatcher->createResponseEvent($this, $response->getBody()->__toString(), $success)) { $this->_eventDispatcher->dispatchEvent($responseEvent, 'responseReceived'); } diff --git a/tests/TransportGuzzleStreamConsumptionTest.php b/tests/TransportGuzzleStreamConsumptionTest.php new file mode 100644 index 0000000..beaf513 --- /dev/null +++ b/tests/TransportGuzzleStreamConsumptionTest.php @@ -0,0 +1,123 @@ +registerPlugin(new ThrowExceptionOnFailurePlugin()); + + $exception = null; + try { + $transport->send($message); + } catch (Swift_TransportException $exception) { + // Deliberately empty + } + + $this->assertNotNull($exception); + $this->assertInstanceOf(Swift_TransportException::class, $exception); + $this->assertSame('Some error from server', $exception->getMessage()); + } + + public function testSendWithMiddleware() + { + $message = new Swift_Message(); + + $transport = new TransportGuzzleStreamConsumptionTestPostmarkTransportStubWithConsumingMiddleware([ + new Response(422, [], 'Some error from server'), + ]); + $transport->registerPlugin(new ThrowExceptionOnFailurePlugin()); + + $exception = null; + try { + $transport->send($message); + } catch (Swift_TransportException $exception) { + // Deliberately empty + } + + $this->assertNotNull($exception); + $this->assertInstanceOf(Swift_TransportException::class, $exception); + + // This would fail if \Postmark\Transport::send would use + // getBody->getContents() instead of getBody->__toString() + $this->assertSame('Some error from server', $exception->getMessage()); + } +} + +class TransportGuzzleStreamConsumptionTestPostmarkTransportStubNoMiddleware extends Transport +{ + protected $client; + + public function __construct(array $responses = []) + { + parent::__construct('TESTING_SERVER'); + + $this->client = $this->mockGuzzle($responses); + } + + protected function getHttpClient() + { + return $this->client; + } + + private function mockGuzzle(array $responses) + { + $stack = HandlerStack::create(new MockHandler($responses)); + + return new Client(['handler' => $stack]); + } +} + +class TransportGuzzleStreamConsumptionTestPostmarkTransportStubWithConsumingMiddleware extends Transport +{ + protected $client; + + public function __construct(array $responses = []) + { + parent::__construct('TESTING_SERVER'); + + $this->client = $this->mockGuzzle($responses); + } + + protected function getHttpClient() + { + return $this->client; + } + + private function mockGuzzle(array $responses) + { + $stack = HandlerStack::create(new MockHandler($responses)); + $stack->push( + function (callable $handler) { + return function ($request, array $options) use ($handler) { + return $handler($request, $options)->then( + function (Response $response) { + // Pretend to do something with $response, like logging + $response->getBody()->__toString(); + + return $response; + } + ); + }; + } + ); + + return new Client(['handler' => $stack]); + } +} +