From bb1e8f2d6f36bda9a138af96226beab69d9d4d60 Mon Sep 17 00:00:00 2001 From: Rob Allen Date: Fri, 16 Aug 2019 09:15:49 +0100 Subject: [PATCH 1/4] Add support for caching the underlying stream While php://input is seekable, you can't re-read it a second time. This is problematic when you want to parse the body for `getParsedBody()`, but also allow subsequent uses of `getBody()` to work. To address this, we now allow `Stream` to optionally cache the data it reads (via `read()` or `getContents()`. Then, once the eof is detected, we return the cached data on subsequent `getContents()` calls so that the stream is not read a second time. Also, update `ServerRequestFactory` so that when it creates a stream for php://input, it enables the caching mechanism. --- src/Factory/ServerRequestFactory.php | 2 +- src/Factory/StreamFactory.php | 13 +++++---- src/Stream.php | 41 ++++++++++++++++++++++++++-- tests/StreamTest.php | 19 +++++++++++++ 4 files changed, 67 insertions(+), 8 deletions(-) diff --git a/src/Factory/ServerRequestFactory.php b/src/Factory/ServerRequestFactory.php index 44c2973..13cd268 100644 --- a/src/Factory/ServerRequestFactory.php +++ b/src/Factory/ServerRequestFactory.php @@ -90,7 +90,7 @@ public static function createFromGlobals(): Request $headers = Headers::createFromGlobals(); $cookies = Cookies::parseHeader($headers->getHeader('Cookie', [])); - $body = (new StreamFactory())->createStreamFromFile('php://input'); + $body = (new StreamFactory())->createStreamFromFile('php://input', 'r', true); $uploadedFiles = UploadedFile::createFromGlobals($_SERVER); $request = new Request($method, $uri, $headers, $cookies, $_SERVER, $body, $uploadedFiles); diff --git a/src/Factory/StreamFactory.php b/src/Factory/StreamFactory.php index 8cdbd5d..b76049b 100644 --- a/src/Factory/StreamFactory.php +++ b/src/Factory/StreamFactory.php @@ -39,8 +39,11 @@ public function createStream(string $content = ''): StreamInterface /** * {@inheritdoc} */ - public function createStreamFromFile(string $filename, string $mode = 'r'): StreamInterface - { + public function createStreamFromFile( + string $filename, + string $mode = 'r', + bool $cacheStream = false + ): StreamInterface { $resource = fopen($filename, $mode); if (!is_resource($resource)) { @@ -49,13 +52,13 @@ public function createStreamFromFile(string $filename, string $mode = 'r'): Stre ); } - return new Stream($resource); + return new Stream($resource, $cacheStream); } /** * {@inheritdoc} */ - public function createStreamFromResource($resource): StreamInterface + public function createStreamFromResource($resource, bool $cacheStream = false): StreamInterface { if (!is_resource($resource)) { throw new InvalidArgumentException( @@ -63,6 +66,6 @@ public function createStreamFromResource($resource): StreamInterface ); } - return new Stream($resource); + return new Stream($resource, $cacheStream); } } diff --git a/src/Stream.php b/src/Stream.php index 3dbac64..39d08dc 100644 --- a/src/Stream.php +++ b/src/Stream.php @@ -60,13 +60,30 @@ class Stream implements StreamInterface protected $isPipe; /** - * @param resource $stream A PHP resource handle. + * @var bool + */ + protected $finished; + + /** + * @var bool + */ + protected $cacheStream; + + /** + * @var string + */ + protected $cache = ''; + + /** + * @param resource $stream A PHP resource handle. + * @param bool $cacheStream Indicate if the stream should be cached * * @throws InvalidArgumentException If argument is not a resource. */ - public function __construct($stream) + public function __construct($stream, bool $cacheStream = false) { $this->attach($stream); + $this->cacheStream = $cacheStream; } /** @@ -137,6 +154,10 @@ public function __toString(): string return ''; } + if ($this->cacheStream && $this->finished) { + return $this->cache; + } + try { $this->rewind(); return $this->getContents(); @@ -292,6 +313,12 @@ public function read($length): string } if (is_string($data)) { + if ($this->cacheStream) { + $this->cache .= $data; + } + if ($this->eof()) { + $this->finished = true; + } return $data; } @@ -322,6 +349,10 @@ public function write($string) */ public function getContents(): string { + if ($this->cacheStream && $this->finished) { + return $this->cache; + } + $contents = false; if ($this->stream) { @@ -329,6 +360,12 @@ public function getContents(): string } if (is_string($contents)) { + if ($this->cacheStream) { + $this->cache .= $contents; + } + if ($this->eof()) { + $this->finished = true; + } return $contents; } diff --git a/tests/StreamTest.php b/tests/StreamTest.php index f27aead..13e74dc 100644 --- a/tests/StreamTest.php +++ b/tests/StreamTest.php @@ -201,4 +201,23 @@ private function openPipeStream() $this->pipeFh = popen('echo 12', 'r'); $this->pipeStream = new Stream($this->pipeFh); } + + + public function testCachedStreamsGetsContentFromTheCache() + { + $resource = popen('echo HelloWorld', 'r'); + $stream = new Stream($resource, true); + + $this->assertEquals("HelloWorld\n", $stream->getContents()); + $this->assertEquals("HelloWorld\n", $stream->getContents()); + } + + public function testCachedStreamsFillsCacheOnRead() + { + $resource = fopen('data://,0', 'r'); + $stream = new Stream($resource, true); + + $this->assertEquals("0", $stream->read(100)); + $this->assertEquals("0", $stream->__toString()); + } } From 4285a7362774416d104695fcf81a725f70aabad5 Mon Sep 17 00:00:00 2001 From: Rob Allen Date: Fri, 16 Aug 2019 09:42:22 +0100 Subject: [PATCH 2/4] Test that the ServerRequestFactory enables a cached Stream for php://input --- tests/Factory/ServerRequestFactoryTest.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/Factory/ServerRequestFactoryTest.php b/tests/Factory/ServerRequestFactoryTest.php index 6fad89e..5dbe590 100644 --- a/tests/Factory/ServerRequestFactoryTest.php +++ b/tests/Factory/ServerRequestFactoryTest.php @@ -12,6 +12,7 @@ use Interop\Http\Factory\ServerRequestFactoryTestCase; use InvalidArgumentException; use Psr\Http\Message\UriInterface; +use ReflectionClass; use Slim\Psr7\Environment; use Slim\Psr7\Factory\ServerRequestFactory; use Slim\Psr7\Factory\UriFactory; @@ -115,6 +116,14 @@ public function testCreateFromGlobalsBodyPointsToPhpInput() $request = ServerRequestFactory::createFromGlobals(); $this->assertEquals('php://input', $request->getBody()->getMetadata('uri')); + + // ensure that the Stream's $cacheStream property has been set to true for this php://input stream + $stream = $request->getBody(); + $class = new ReflectionClass($stream); + $property = $class->getProperty('cacheStream'); + $property->setAccessible(true); + $cacheStreamValue = $property->getValue($stream); + $this->assertTrue($cacheStreamValue); } public function testCreateFromGlobalsWithUploadedFiles() From 12ff0228737c85cf6f52bc97b6274c7794515cc0 Mon Sep 17 00:00:00 2001 From: Rob Allen Date: Sun, 18 Aug 2019 12:30:19 +0100 Subject: [PATCH 3/4] Cache to a StreamInterface in Slim\Psr7\Stream When caching a non-seekable stream, allow the user to provide the StreamInterface object that the wish to use. When creating a Stream from globals, cache the php://input based Stream with a php://temp steam. --- src/Factory/ServerRequestFactory.php | 7 +++- src/Factory/StreamFactory.php | 8 ++--- src/Stream.php | 37 +++++++++++----------- tests/Factory/ServerRequestFactoryTest.php | 11 +++++-- tests/StreamTest.php | 24 ++++++++++++-- 5 files changed, 59 insertions(+), 28 deletions(-) diff --git a/src/Factory/ServerRequestFactory.php b/src/Factory/ServerRequestFactory.php index 13cd268..a8115b6 100644 --- a/src/Factory/ServerRequestFactory.php +++ b/src/Factory/ServerRequestFactory.php @@ -18,6 +18,7 @@ use Slim\Psr7\Cookies; use Slim\Psr7\Headers; use Slim\Psr7\Request; +use Slim\Psr7\Stream; use Slim\Psr7\UploadedFile; class ServerRequestFactory implements ServerRequestFactoryInterface @@ -90,7 +91,11 @@ public static function createFromGlobals(): Request $headers = Headers::createFromGlobals(); $cookies = Cookies::parseHeader($headers->getHeader('Cookie', [])); - $body = (new StreamFactory())->createStreamFromFile('php://input', 'r', true); + // Cache the php://input stream as it cannot be re-read + $cacheResource = fopen('php://temp', 'wb+'); + $cache = $cacheResource ? new Stream($cacheResource) : null; + + $body = (new StreamFactory())->createStreamFromFile('php://input', 'r', $cache); $uploadedFiles = UploadedFile::createFromGlobals($_SERVER); $request = new Request($method, $uri, $headers, $cookies, $_SERVER, $body, $uploadedFiles); diff --git a/src/Factory/StreamFactory.php b/src/Factory/StreamFactory.php index b76049b..26341a2 100644 --- a/src/Factory/StreamFactory.php +++ b/src/Factory/StreamFactory.php @@ -42,7 +42,7 @@ public function createStream(string $content = ''): StreamInterface public function createStreamFromFile( string $filename, string $mode = 'r', - bool $cacheStream = false + StreamInterface $cache = null ): StreamInterface { $resource = fopen($filename, $mode); @@ -52,13 +52,13 @@ public function createStreamFromFile( ); } - return new Stream($resource, $cacheStream); + return new Stream($resource, $cache); } /** * {@inheritdoc} */ - public function createStreamFromResource($resource, bool $cacheStream = false): StreamInterface + public function createStreamFromResource($resource, StreamInterface $cache = null): StreamInterface { if (!is_resource($resource)) { throw new InvalidArgumentException( @@ -66,6 +66,6 @@ public function createStreamFromResource($resource, bool $cacheStream = false): ); } - return new Stream($resource, $cacheStream); + return new Stream($resource, $cache); } } diff --git a/src/Stream.php b/src/Stream.php index 39d08dc..88b9494 100644 --- a/src/Stream.php +++ b/src/Stream.php @@ -65,25 +65,24 @@ class Stream implements StreamInterface protected $finished; /** - * @var bool - */ - protected $cacheStream; - - /** - * @var string + * @var StreamInterface | null */ - protected $cache = ''; + protected $cache; /** * @param resource $stream A PHP resource handle. - * @param bool $cacheStream Indicate if the stream should be cached + * @param StreamInterface $cache A stream to cache $stream (useful for non-seekable streams) * * @throws InvalidArgumentException If argument is not a resource. */ - public function __construct($stream, bool $cacheStream = false) + public function __construct($stream, StreamInterface $cache = null) { $this->attach($stream); - $this->cacheStream = $cacheStream; + + if ($cache && (!$cache->isSeekable() || !$cache->isWritable())) { + throw new RuntimeException('Cache stream must be seekable and writable'); + } + $this->cache = $cache; } /** @@ -154,8 +153,9 @@ public function __toString(): string return ''; } - if ($this->cacheStream && $this->finished) { - return $this->cache; + if ($this->cache && $this->finished) { + $this->cache->rewind(); + return $this->cache->getContents(); } try { @@ -313,8 +313,8 @@ public function read($length): string } if (is_string($data)) { - if ($this->cacheStream) { - $this->cache .= $data; + if ($this->cache) { + $this->cache->write($data); } if ($this->eof()) { $this->finished = true; @@ -349,8 +349,9 @@ public function write($string) */ public function getContents(): string { - if ($this->cacheStream && $this->finished) { - return $this->cache; + if ($this->cache && $this->finished) { + $this->cache->rewind(); + return $this->cache->getContents(); } $contents = false; @@ -360,8 +361,8 @@ public function getContents(): string } if (is_string($contents)) { - if ($this->cacheStream) { - $this->cache .= $contents; + if ($this->cache) { + $this->cache->write($contents); } if ($this->eof()) { $this->finished = true; diff --git a/tests/Factory/ServerRequestFactoryTest.php b/tests/Factory/ServerRequestFactoryTest.php index 5dbe590..d0aba12 100644 --- a/tests/Factory/ServerRequestFactoryTest.php +++ b/tests/Factory/ServerRequestFactoryTest.php @@ -116,14 +116,19 @@ public function testCreateFromGlobalsBodyPointsToPhpInput() $request = ServerRequestFactory::createFromGlobals(); $this->assertEquals('php://input', $request->getBody()->getMetadata('uri')); + } + + public function testCreateFromGlobalsSetsACache() + { + $request = ServerRequestFactory::createFromGlobals(); - // ensure that the Stream's $cacheStream property has been set to true for this php://input stream + // ensure that the Stream's $cache property has been set for this php://input stream $stream = $request->getBody(); $class = new ReflectionClass($stream); - $property = $class->getProperty('cacheStream'); + $property = $class->getProperty('cache'); $property->setAccessible(true); $cacheStreamValue = $property->getValue($stream); - $this->assertTrue($cacheStreamValue); + $this->assertNotNull($cacheStreamValue); } public function testCreateFromGlobalsWithUploadedFiles() diff --git a/tests/StreamTest.php b/tests/StreamTest.php index 13e74dc..862ecc9 100644 --- a/tests/StreamTest.php +++ b/tests/StreamTest.php @@ -202,11 +202,31 @@ private function openPipeStream() $this->pipeStream = new Stream($this->pipeFh); } + public function testReadOnlyCachedStreamsAreDisallowed() + { + $resource = fopen('php://temp', 'w+'); + $cache = new Stream(fopen('php://temp', 'r')); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('Cache stream must be seekable and writable'); + new Stream($resource, $cache); + } + + public function testNonSeekableCachedStreamsAreDisallowed() + { + $resource = fopen('php://temp', 'w+'); + $cache = new Stream(fopen('php://output', 'w')); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('Cache stream must be seekable and writable'); + + new Stream($resource, $cache); + } public function testCachedStreamsGetsContentFromTheCache() { $resource = popen('echo HelloWorld', 'r'); - $stream = new Stream($resource, true); + $stream = new Stream($resource, new Stream(fopen('php://temp', 'w+'))); $this->assertEquals("HelloWorld\n", $stream->getContents()); $this->assertEquals("HelloWorld\n", $stream->getContents()); @@ -215,7 +235,7 @@ public function testCachedStreamsGetsContentFromTheCache() public function testCachedStreamsFillsCacheOnRead() { $resource = fopen('data://,0', 'r'); - $stream = new Stream($resource, true); + $stream = new Stream($resource, new Stream(fopen('php://temp', 'w+'))); $this->assertEquals("0", $stream->read(100)); $this->assertEquals("0", $stream->__toString()); From 754b85b6b7a962720a259c3f99dfb160dc3871d2 Mon Sep 17 00:00:00 2001 From: Rob Allen Date: Sun, 18 Aug 2019 12:53:08 +0100 Subject: [PATCH 4/4] Use PHP 7.3 for analysis on Travis --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 5820908..b52d2be 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,9 +5,9 @@ dist: trusty matrix: include: - php: 7.1 - env: ANALYSIS='true' - php: 7.2 - php: 7.3 + env: ANALYSIS='true' - php: nightly allow_failures: - php: nightly