From 2c9cbe9126dc6ee90dea85eae9d9b53ca55e1663 Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Sat, 8 Jun 2019 19:41:48 +0200 Subject: [PATCH 01/12] Create a CSV Response class This class is almost identical to TextResponse, except that it sets the Content-Type header to text/csv. --- src/Response/CsvResponse.php | 79 +++++++++++++++++++++++++ test/Response/CsvResponseTest.php | 97 +++++++++++++++++++++++++++++++ 2 files changed, 176 insertions(+) create mode 100644 src/Response/CsvResponse.php create mode 100644 test/Response/CsvResponseTest.php diff --git a/src/Response/CsvResponse.php b/src/Response/CsvResponse.php new file mode 100644 index 00000000..39190ca9 --- /dev/null +++ b/src/Response/CsvResponse.php @@ -0,0 +1,79 @@ +createBody($text), + $status, + $this->injectContentType('text/csv; charset=utf-8', $headers) + ); + } + + /** + * Create the CSV message body. + * + * @param string|StreamInterface $text + * @throws Exception\InvalidArgumentException if $text is neither a string or stream. + */ + private function createBody($text) : StreamInterface + { + if ($text instanceof StreamInterface) { + return $text; + } + + if (! is_string($text)) { + throw new Exception\InvalidArgumentException(sprintf( + 'Invalid CSV content (%s) provided to %s', + (is_object($text) ? get_class($text) : gettype($text)), + __CLASS__ + )); + } + + $body = new Stream('php://temp', 'wb+'); + $body->write($text); + $body->rewind(); + return $body; + } +} diff --git a/test/Response/CsvResponseTest.php b/test/Response/CsvResponseTest.php new file mode 100644 index 00000000..639c11de --- /dev/null +++ b/test/Response/CsvResponseTest.php @@ -0,0 +1,97 @@ +assertSame(self::VALID_CSV_BODY, (string) $response->getBody()); + $this->assertSame(200, $response->getStatusCode()); + } + + public function testConstructorAllowsPassingStatus() + { + $status = 404; + + $response = new CsvResponse(self::VALID_CSV_BODY, $status); + $this->assertSame(404, $response->getStatusCode()); + $this->assertSame(self::VALID_CSV_BODY, (string) $response->getBody()); + } + + public function testConstructorAllowsPassingHeaders() + { + $status = 404; + $headers = [ + 'x-custom' => [ 'foo-bar' ], + ]; + + $response = new CsvResponse(self::VALID_CSV_BODY, $status, $headers); + $this->assertSame(['foo-bar'], $response->getHeader('x-custom')); + $this->assertSame('text/csv; charset=utf-8', $response->getHeaderLine('content-type')); + $this->assertSame(404, $response->getStatusCode()); + $this->assertSame(self::VALID_CSV_BODY, (string) $response->getBody()); + } + + public function testAllowsStreamsForResponseBody() + { + $stream = $this->prophesize(StreamInterface::class); + $body = $stream->reveal(); + $response = new CsvResponse($body); + $this->assertSame($body, $response->getBody()); + } + + public function invalidContent() + { + return [ + 'null' => [null], + 'true' => [true], + 'false' => [false], + 'zero' => [0], + 'int' => [1], + 'zero-float' => [0.0], + 'float' => [1.1], + 'array' => [['php://temp']], + 'object' => [(object) ['php://temp']], + ]; + } + + /** + * @dataProvider invalidContent + */ + public function testRaisesExceptionforNonStringNonStreamBodyContent($body) + { + $this->expectException(InvalidArgumentException::class); + + new CsvResponse($body); + } + + /** + * @group 115 + */ + public function testConstructorRewindsBodyStream() + { + $response = new CsvResponse(self::VALID_CSV_BODY); + + $actual = $response->getBody()->getContents(); + $this->assertSame(self::VALID_CSV_BODY, $actual); + } +} From b05ec0c84d48368ee0bdb0f381e2da374462fc46 Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Mon, 10 Jun 2019 11:46:08 +0200 Subject: [PATCH 02/12] Add a trait for managing download responses After doing a bit of searching, I didn't find anything that would help with sending a downloaded response, instead of the default, which is to send a text response. So I created this one, which sends six headers that ensure that the response sent will be interpreted by the client as a downloadable file. --- src/Response/DownloadResponseTrait.php | 83 ++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 src/Response/DownloadResponseTrait.php diff --git a/src/Response/DownloadResponseTrait.php b/src/Response/DownloadResponseTrait.php new file mode 100644 index 00000000..9bd9e0f7 --- /dev/null +++ b/src/Response/DownloadResponseTrait.php @@ -0,0 +1,83 @@ +overridesDownloadHeaders($this->downloadResponseHeaders, $headers)) { + throw new InvalidArgumentException( + sprintf( + 'Cannot override download headers (%s) when download response is being sent', + implode(', ', $this->downloadResponseHeaders) + ) + ); + } + + return array_merge($headers, $this->getDownloadHeaders($filename)); + } +} From 62b26053366a1955448f3b758ef3c9a3768353ba Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Mon, 10 Jun 2019 11:47:49 +0200 Subject: [PATCH 03/12] Refactor the CsvResponse to support sending a downloadable response This change update the CsvResponse to be able to either send a plain CSV text response, or to send the response as a downloadable file instead. --- src/Response/CsvResponse.php | 184 ++++++++++++---------- test/Response/CsvResponseTest.php | 245 ++++++++++++++++++------------ 2 files changed, 253 insertions(+), 176 deletions(-) diff --git a/src/Response/CsvResponse.php b/src/Response/CsvResponse.php index 39190ca9..cc0eaf02 100644 --- a/src/Response/CsvResponse.php +++ b/src/Response/CsvResponse.php @@ -1,79 +1,105 @@ -createBody($text), - $status, - $this->injectContentType('text/csv; charset=utf-8', $headers) - ); - } - - /** - * Create the CSV message body. - * - * @param string|StreamInterface $text - * @throws Exception\InvalidArgumentException if $text is neither a string or stream. - */ - private function createBody($text) : StreamInterface - { - if ($text instanceof StreamInterface) { - return $text; - } - - if (! is_string($text)) { - throw new Exception\InvalidArgumentException(sprintf( - 'Invalid CSV content (%s) provided to %s', - (is_object($text) ? get_class($text) : gettype($text)), - __CLASS__ - )); - } - - $body = new Stream('php://temp', 'wb+'); - $body->write($text); - $body->rewind(); - return $body; - } -} +prepareDownloadHeaders($filename, $headers); + } + + parent::__construct( + $this->createBody($text), + $status, + $this->injectContentType('text/csv; charset=utf-8', $headers) + ); + } + + /** + * Create the CSV message body. + * + * @param string|StreamInterface $text + * @return StreamInterface + * @throws Exception\InvalidArgumentException if $text is neither a string or stream. + */ + private function createBody($text) : StreamInterface + { + if ($text instanceof StreamInterface) { + return $text; + } + + if (! is_string($text)) { + throw new Exception\InvalidArgumentException(sprintf( + 'Invalid CSV content (%s) provided to %s', + (is_object($text) ? get_class($text) : gettype($text)), + __CLASS__ + )); + } + + $body = new Stream('php://temp', 'wb+'); + $body->write($text); + $body->rewind(); + return $body; + } + + /** + * Get download headers + * + * @param string $filename + * @return array + */ + private function getDownloadHeaders(string $filename): array + { + $headers = []; + $headers['cache-control'] = ['must-revalidate']; + $headers['content-description'] = ['File Transfer']; + $headers['content-disposition'] = [sprintf('attachment; filename=%s', $filename)]; + $headers['content-transfer-encoding'] = ['Binary']; + $headers['content-type'] = ['text/csv; charset=utf-8']; + $headers['expires'] = ['0']; + $headers['pragma'] = ['Public']; + + return $headers; + } +} diff --git a/test/Response/CsvResponseTest.php b/test/Response/CsvResponseTest.php index 639c11de..5be85ce6 100644 --- a/test/Response/CsvResponseTest.php +++ b/test/Response/CsvResponseTest.php @@ -1,97 +1,148 @@ -assertSame(self::VALID_CSV_BODY, (string) $response->getBody()); - $this->assertSame(200, $response->getStatusCode()); - } - - public function testConstructorAllowsPassingStatus() - { - $status = 404; - - $response = new CsvResponse(self::VALID_CSV_BODY, $status); - $this->assertSame(404, $response->getStatusCode()); - $this->assertSame(self::VALID_CSV_BODY, (string) $response->getBody()); - } - - public function testConstructorAllowsPassingHeaders() - { - $status = 404; - $headers = [ - 'x-custom' => [ 'foo-bar' ], - ]; - - $response = new CsvResponse(self::VALID_CSV_BODY, $status, $headers); - $this->assertSame(['foo-bar'], $response->getHeader('x-custom')); - $this->assertSame('text/csv; charset=utf-8', $response->getHeaderLine('content-type')); - $this->assertSame(404, $response->getStatusCode()); - $this->assertSame(self::VALID_CSV_BODY, (string) $response->getBody()); - } - - public function testAllowsStreamsForResponseBody() - { - $stream = $this->prophesize(StreamInterface::class); - $body = $stream->reveal(); - $response = new CsvResponse($body); - $this->assertSame($body, $response->getBody()); - } - - public function invalidContent() - { - return [ - 'null' => [null], - 'true' => [true], - 'false' => [false], - 'zero' => [0], - 'int' => [1], - 'zero-float' => [0.0], - 'float' => [1.1], - 'array' => [['php://temp']], - 'object' => [(object) ['php://temp']], - ]; - } - - /** - * @dataProvider invalidContent - */ - public function testRaisesExceptionforNonStringNonStreamBodyContent($body) - { - $this->expectException(InvalidArgumentException::class); - - new CsvResponse($body); - } - - /** - * @group 115 - */ - public function testConstructorRewindsBodyStream() - { - $response = new CsvResponse(self::VALID_CSV_BODY); - - $actual = $response->getBody()->getContents(); - $this->assertSame(self::VALID_CSV_BODY, $actual); - } -} +assertSame(self::VALID_CSV_BODY, (string) $response->getBody()); + $this->assertSame(200, $response->getStatusCode()); + } + + public function testConstructorAllowsPassingStatus() + { + $status = 404; + + $response = new CsvResponse(self::VALID_CSV_BODY, $status); + $this->assertSame(404, $response->getStatusCode()); + $this->assertSame(self::VALID_CSV_BODY, (string) $response->getBody()); + } + + public function testConstructorAllowsSendingDownloadResponse() + { + $status = 404; + $filename = 'download.csv'; + + $response = new CsvResponse(self::VALID_CSV_BODY, $status, $filename); + $this->assertSame( + [ + 'cache-control' => ['must-revalidate'], + 'content-description' => ['File Transfer'], + 'content-disposition' => [sprintf('attachment; filename=%s', basename($filename))], + 'content-transfer-encoding' => ['Binary'], + 'content-type' => ['text/csv; charset=utf-8'], + 'expires' => ['0'], + 'pragma' => ['Public'], + ], + $response->getHeaders() + ); + $this->assertSame(404, $response->getStatusCode()); + $this->assertSame(self::VALID_CSV_BODY, (string) $response->getBody()); + } + + /** + * @dataProvider invalidHeadersWhenDownloading + */ + public function testConstructorDoesNotAllowsOverridingDownloadHeadersWhenSendingDownloadResponse($header, $value) + { + $status = 404; + $filename = 'download.csv'; + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage( + 'Cannot override download headers (cache-control, content-description, content-disposition, content-transfer-encoding, expires, pragma) when download response is being sent' + ); + + new CsvResponse(self::VALID_CSV_BODY, $status, $filename, [$header => [$value]]); + + } + + public function invalidHeadersWhenDownloading() + { + return [ + ['cache-control', 'must-revalidate',], + ['content-description', 'File Transfer',], + ['content-disposition', 'upload.csv',], + ['content-transfer-encoding', 'Binary',], + ['expires', '0',], + ['pragma', 'Public',] + ]; + } + + public function testConstructorAllowsPassingHeaders() + { + $status = 404; + $headers = [ + 'x-custom' => [ 'foo-bar' ], + ]; + $filename = ''; + + $response = new CsvResponse(self::VALID_CSV_BODY, $status, $filename, $headers); + $this->assertSame(['foo-bar'], $response->getHeader('x-custom')); + $this->assertSame('text/csv; charset=utf-8', $response->getHeaderLine('content-type')); + $this->assertSame(404, $response->getStatusCode()); + $this->assertSame(self::VALID_CSV_BODY, (string) $response->getBody()); + } + + public function testAllowsStreamsForResponseBody() + { + $stream = $this->prophesize(StreamInterface::class); + $body = $stream->reveal(); + $response = new CsvResponse($body); + $this->assertSame($body, $response->getBody()); + } + + public function invalidContent() + { + return [ + 'null' => [null], + 'true' => [true], + 'false' => [false], + 'zero' => [0], + 'int' => [1], + 'zero-float' => [0.0], + 'float' => [1.1], + 'array' => [['php://temp']], + 'object' => [(object) ['php://temp']], + ]; + } + + /** + * @dataProvider invalidContent + */ + public function testRaisesExceptionforNonStringNonStreamBodyContent($body) + { + $this->expectException(InvalidArgumentException::class); + + new CsvResponse($body); + } + + /** + * @group 115 + */ + public function testConstructorRewindsBodyStream() + { + $response = new CsvResponse(self::VALID_CSV_BODY); + + $actual = $response->getBody()->getContents(); + $this->assertSame(self::VALID_CSV_BODY, $actual); + } +} From 20b01d9b6f3828c85c3bb1c526ee6016d4bb3229 Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Mon, 10 Jun 2019 12:19:37 +0200 Subject: [PATCH 04/12] Update documentation to add document new CsvResponse class In the style of the existing Response class documentation, this change adds a section covering the new CsvResponse class. --- docs/book/v2/custom-responses.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/docs/book/v2/custom-responses.md b/docs/book/v2/custom-responses.md index 38bf4ef3..e1f93574 100644 --- a/docs/book/v2/custom-responses.md +++ b/docs/book/v2/custom-responses.md @@ -132,6 +132,34 @@ $response = new Zend\Diactoros\Response\JsonResponse( ); ``` +## CSV Responses + +`Zend\Diactoros\Response\CsvResponse` creates a plain text response. It sets the +`Content-Type` header to `text/csv` by default: + +```php +$csvContent = << ['zend-diactoros']] +); + ## Empty Responses Many API actions allow returning empty responses: From 411aed27cda9eaa62caa2151139ea6156bf98b82 Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Wed, 18 Dec 2019 05:17:29 +0100 Subject: [PATCH 05/12] Rename the DownloadResponse trait to a class The intent here is to kick of the refactor of the trait to a class and to follow it up progressively with further commits that complete the refactor. --- src/Response/CsvResponse.php | 1 - .../{DownloadResponseTrait.php => DownloadResponse.php} | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) rename src/Response/{DownloadResponseTrait.php => DownloadResponse.php} (97%) diff --git a/src/Response/CsvResponse.php b/src/Response/CsvResponse.php index cc0eaf02..36e46a1b 100644 --- a/src/Response/CsvResponse.php +++ b/src/Response/CsvResponse.php @@ -29,7 +29,6 @@ */ class CsvResponse extends Response { - use DownloadResponseTrait; use InjectContentTypeTrait; /** diff --git a/src/Response/DownloadResponseTrait.php b/src/Response/DownloadResponse.php similarity index 97% rename from src/Response/DownloadResponseTrait.php rename to src/Response/DownloadResponse.php index 9bd9e0f7..dd3a46cd 100644 --- a/src/Response/DownloadResponseTrait.php +++ b/src/Response/DownloadResponse.php @@ -18,10 +18,10 @@ use function sprintf; /** - * Trait DownloadResponseTrait + * Class DownloadResponse * @package Zend\Diactoros\Response */ -trait DownloadResponseTrait +class DownloadResponse extends Response { /** * A list of header keys required to be sent with a download response From 2088793c69d0123e04b8c9ec05b26f2c6b214471 Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Wed, 18 Dec 2019 05:18:42 +0100 Subject: [PATCH 06/12] Add vfsstream as a development dependency Over the course of several projects I've found this package to be excellent for mocking the filesystem, which is required for the DownloadResponse class. --- composer.json | 1 + composer.lock | 48 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 738d3ecf..0cbffd6d 100644 --- a/composer.json +++ b/composer.json @@ -35,6 +35,7 @@ "ext-dom": "*", "ext-libxml": "*", "http-interop/http-factory-tests": "^0.5.0", + "mikey179/vfsstream": "^1.6", "php-http/psr7-integration-tests": "dev-master", "phpunit/phpunit": "^7.5.18", "zendframework/zend-coding-standard": "~1.0.0" diff --git a/composer.lock b/composer.lock index 1e073ae8..a826a5de 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "1bac61830d3bc9c8234592861c735f58", + "content-hash": "5b844a9db9c1c0aa3478cae9cd0ec7e9", "packages": [ { "name": "psr/http-factory", @@ -210,6 +210,52 @@ ], "time": "2018-07-31T18:30:29+00:00" }, + { + "name": "mikey179/vfsstream", + "version": "v1.6.8", + "source": { + "type": "git", + "url": "https://github.com/bovigo/vfsStream.git", + "reference": "231c73783ebb7dd9ec77916c10037eff5a2b6efe" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/bovigo/vfsStream/zipball/231c73783ebb7dd9ec77916c10037eff5a2b6efe", + "reference": "231c73783ebb7dd9ec77916c10037eff5a2b6efe", + "shasum": "" + }, + "require": { + "php": ">=5.3.0" + }, + "require-dev": { + "phpunit/phpunit": "^4.5|^5.0" + }, + "type": "library", + "extra": { + "branch-alias": { + "dev-master": "1.6.x-dev" + } + }, + "autoload": { + "psr-0": { + "org\\bovigo\\vfs\\": "src/main/php" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "BSD-3-Clause" + ], + "authors": [ + { + "name": "Frank Kleine", + "homepage": "http://frankkleine.de/", + "role": "Developer" + } + ], + "description": "Virtual file system to mock the real file system in unit tests.", + "homepage": "http://vfs.bovigo.org/", + "time": "2019-10-30T15:31:00+00:00" + }, { "name": "myclabs/deep-copy", "version": "1.9.3", From aecca69763063815e180e9a14cbf88862427a0f0 Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Wed, 18 Dec 2019 05:22:02 +0100 Subject: [PATCH 07/12] Test that a DownloadResponse can be created from a valid CSV string --- src/Response/DownloadResponse.php | 40 ++++++++++++++++++ test/Response/DownloadResponseTest.php | 56 ++++++++++++++++++++++++++ 2 files changed, 96 insertions(+) create mode 100644 test/Response/DownloadResponseTest.php diff --git a/src/Response/DownloadResponse.php b/src/Response/DownloadResponse.php index dd3a46cd..917749b1 100644 --- a/src/Response/DownloadResponse.php +++ b/src/Response/DownloadResponse.php @@ -10,7 +10,9 @@ namespace Zend\Diactoros\Response; use Zend\Diactoros\Exception\InvalidArgumentException; +use Zend\Diactoros\Response; +use Zend\Diactoros\Stream; use function array_keys; use function array_merge; use function implode; @@ -37,6 +39,44 @@ class DownloadResponse extends Response 'pragma' ]; + /** + * DownloadResponse constructor. + * @param $body + * @param int $status + * @param string $filename + * @param array $headers + */ + public function __construct($body, int $status = 200, string $filename = '', array $headers = []) + { + $content = new Stream('php://temp', 'wb+'); + $content->write($body); + $content->rewind(); + + $headers = $this->prepareDownloadHeaders($filename, $headers); + + parent::__construct($content, $status, $headers); + } + + /** + * Get download headers + * + * @param string $filename + * @return array + */ + private function getDownloadHeaders(string $filename): array + { + $headers = []; + $headers['cache-control'] = ['must-revalidate']; + $headers['content-description'] = ['File Transfer']; + $headers['content-disposition'] = [sprintf('attachment; filename=%s', $filename)]; + $headers['content-transfer-encoding'] = ['Binary']; + $headers['content-type'] = ['text/csv; charset=utf-8']; + $headers['expires'] = ['0']; + $headers['pragma'] = ['Public']; + + return $headers; + } + /** * Check if the extra headers contain any of the download headers * diff --git a/test/Response/DownloadResponseTest.php b/test/Response/DownloadResponseTest.php new file mode 100644 index 00000000..bf4c0c62 --- /dev/null +++ b/test/Response/DownloadResponseTest.php @@ -0,0 +1,56 @@ + [ + 'empty.csv' => "", + 'valid.csv' => $validCSVString, + 'non-readable-file.csv' => vfsStream::newFile('non-readable-file.csv', 0000) + ] + ]; + $this->root = vfsStream::setup('root', null, $directoryStructure); + } + + public function testCanCreateResponseFromString() + { + $csvString = file_get_contents($this->root->url() . '/files/valid.csv'); + $response = new DownloadResponse($csvString); + $this->assertInstanceOf(Response::class, $response); + $this->assertEquals($csvString, (string) $response->getBody()->getContents()); + $this->assertArrayHasKey('cache-control', $response->getHeaders()); + } +} From d55873a9ab26b00accf76811a48a506c3192f946 Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Thu, 26 Dec 2019 21:57:23 +0100 Subject: [PATCH 08/12] Refactor to be able to use stream or filename for response body This is following on from what mwop suggested in that the body of the response can be set from either a string or a file. It's making use of a Stream object and the body is instantiated similarly to XmlResponse as how that works makes a lot of sense in this case. --- src/Response/DownloadResponse.php | 62 ++++++++++++++++++-------- test/Response/DownloadResponseTest.php | 58 ++++++++++++++++++++++-- 2 files changed, 98 insertions(+), 22 deletions(-) diff --git a/src/Response/DownloadResponse.php b/src/Response/DownloadResponse.php index 917749b1..694c3a25 100644 --- a/src/Response/DownloadResponse.php +++ b/src/Response/DownloadResponse.php @@ -9,6 +9,7 @@ namespace Zend\Diactoros\Response; +use Psr\Http\Message\StreamInterface; use Zend\Diactoros\Exception\InvalidArgumentException; use Zend\Diactoros\Response; @@ -41,20 +42,20 @@ class DownloadResponse extends Response /** * DownloadResponse constructor. - * @param $body - * @param int $status - * @param string $filename + * @param string|StreamInterface $body String or stream for the message body. + * @param int $status Integer status code for the response; 200 by default. + * @param string $filename The name of the file to be downloaded + * @param array $headers Array of headers to use at initialization. + * @throws InvalidArgumentException if $text is neither a string or stream. * @param array $headers */ - public function __construct($body, int $status = 200, string $filename = '', array $headers = []) + public function __construct($body, int $status = 200, string $filename = 'download', array $headers = []) { - $content = new Stream('php://temp', 'wb+'); - $content->write($body); - $content->rewind(); - - $headers = $this->prepareDownloadHeaders($filename, $headers); - - parent::__construct($content, $status, $headers); + parent::__construct( + $this->createBody($body), + $status, + $this->prepareDownloadHeaders($filename, $headers) + ); } /** @@ -66,13 +67,13 @@ public function __construct($body, int $status = 200, string $filename = '', arr private function getDownloadHeaders(string $filename): array { $headers = []; - $headers['cache-control'] = ['must-revalidate']; - $headers['content-description'] = ['File Transfer']; - $headers['content-disposition'] = [sprintf('attachment; filename=%s', $filename)]; - $headers['content-transfer-encoding'] = ['Binary']; - $headers['content-type'] = ['text/csv; charset=utf-8']; - $headers['expires'] = ['0']; - $headers['pragma'] = ['Public']; + $headers['cache-control'] = 'must-revalidate'; + $headers['content-description'] = 'File Transfer'; + $headers['content-disposition'] = sprintf('attachment; filename=%s', $filename); + $headers['content-transfer-encoding'] = 'Binary'; + $headers['content-type'] = 'application/octet-stream'; + $headers['expires'] = '0'; + $headers['pragma'] = 'Public'; return $headers; } @@ -120,4 +121,29 @@ private function prepareDownloadHeaders(string $filename, array $headers = []) : return array_merge($headers, $this->getDownloadHeaders($filename)); } + + /** + * @param string|StreamInterface $content + * @return StreamInterface + * @throws InvalidArgumentException if $body is neither a string nor a stream + */ + private function createBody($content): StreamInterface + { + if ($content instanceof StreamInterface) { + return $content; + } + + if (!is_string($content)) { + throw new InvalidArgumentException(sprintf( + 'Invalid content (%s) provided to %s', + (is_object($content) ? get_class($content) : gettype($content)), + __CLASS__ + )); + } + + $body = new Stream('php://temp', 'wb+'); + $body->write($content); + $body->rewind(); + return $body; + } } diff --git a/test/Response/DownloadResponseTest.php b/test/Response/DownloadResponseTest.php index bf4c0c62..c24c2ae1 100644 --- a/test/Response/DownloadResponseTest.php +++ b/test/Response/DownloadResponseTest.php @@ -13,6 +13,7 @@ use PHPUnit\Framework\TestCase; use Zend\Diactoros\Response; use Zend\Diactoros\Response\DownloadResponse; +use Zend\Diactoros\Stream; class DownloadResponseTest extends TestCase { @@ -47,10 +48,59 @@ public function setUp() public function testCanCreateResponseFromString() { - $csvString = file_get_contents($this->root->url() . '/files/valid.csv'); - $response = new DownloadResponse($csvString); + $body = file_get_contents($this->root->url() . '/files/valid.csv'); + $response = new DownloadResponse($body); $this->assertInstanceOf(Response::class, $response); - $this->assertEquals($csvString, (string) $response->getBody()->getContents()); - $this->assertArrayHasKey('cache-control', $response->getHeaders()); + $this->assertEquals($body, (string) $response->getBody()); + $this->assertEquals(619, $response->getBody()->getSize()); + $this->assertHasValidResponseHeaders($response); + $this->assertSame('attachment; filename=download', $response->getHeaderLine('content-disposition')); + } + + public function testCanCreateResponseFromFilename() + { + $body = new Stream($this->root->url() . '/files/valid.csv'); + $response = new DownloadResponse($body); + $this->assertInstanceOf(Response::class, $response); + $this->assertEquals( + file_get_contents($this->root->url() . '/files/valid.csv'), + (string) $response->getBody() + ); + $this->assertHasValidResponseHeaders($response); + $this->assertSame('attachment; filename=download', $response->getHeaderLine('content-disposition')); + } + + public function testCanSendResponseWithCustomFilename() + { + $body = new Stream($this->root->url() . '/files/valid.csv'); + $response = new DownloadResponse($body, 200, 'valid.csv'); + $this->assertInstanceOf(Response::class, $response); + $this->assertEquals( + file_get_contents($this->root->url() . '/files/valid.csv'), + (string) $response->getBody() + ); + $this->assertHasValidResponseHeaders($response, 'valid.csv'); + $this->assertSame('attachment; filename=valid.csv', $response->getHeaderLine('content-disposition')); + } + + /** + * @param DownloadResponse $response + * @param string $filename + */ + private function assertHasValidResponseHeaders(DownloadResponse $response, $filename = 'download'): void + { + $requiredHeaders = [ + 'cache-control' => 'must-revalidate', + 'content-description' => 'File Transfer', + 'content-disposition' => sprintf('attachment; filename=%s', $filename), + 'content-transfer-encoding' => 'Binary', + 'content-type' => 'application/octet-stream', + 'expires' => '0', + 'pragma' => 'Public' + ]; + foreach ($requiredHeaders as $header => $value) { + $this->assertTrue($response->hasHeader($header)); + $this->assertSame($value, $response->getHeaderLine($header)); + } } } From 585446250c088e7cfd97b1c0135b57fbca2e398f Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Thu, 26 Dec 2019 22:42:10 +0100 Subject: [PATCH 09/12] Set the content-type and -disposition headers from constructor arguments After thinking about this at some length, it makes sense to me (at least at this stage) to set the content-type and content-disposition headers from values supplied ($contentType and $filename values respectively) to the constructor. This, I think, will be cleaner and more maintainable. --- src/Response/DownloadResponse.php | 59 ++++++++++++++++++++------ test/Response/DownloadResponseTest.php | 23 ++++++++-- 2 files changed, 65 insertions(+), 17 deletions(-) diff --git a/src/Response/DownloadResponse.php b/src/Response/DownloadResponse.php index 694c3a25..4ff86eb2 100644 --- a/src/Response/DownloadResponse.php +++ b/src/Response/DownloadResponse.php @@ -26,6 +26,9 @@ */ class DownloadResponse extends Response { + const DEFAULT_CONTENT_TYPE = 'application/octet-stream'; + const DEFAULT_DOWNLOAD_FILENAME = 'download'; + /** * A list of header keys required to be sent with a download response * @@ -40,36 +43,52 @@ class DownloadResponse extends Response 'pragma' ]; + /** + * @var string The filename to be sent with the response + */ + private $filename; + + /** + * @var string The content type to be sent with the response + */ + private $contentType; + /** * DownloadResponse constructor. + * * @param string|StreamInterface $body String or stream for the message body. * @param int $status Integer status code for the response; 200 by default. - * @param string $filename The name of the file to be downloaded - * @param array $headers Array of headers to use at initialization. - * @throws InvalidArgumentException if $text is neither a string or stream. - * @param array $headers - */ - public function __construct($body, int $status = 200, string $filename = 'download', array $headers = []) - { + * @param string $filename The file name to be sent with the response + * @param string $contentType The content type to be sent with the response + * @param array $headers An array of optional headers. These cannot override those set in getDownloadHeaders */ + public function __construct( + $body, + int $status = 200, + string $filename = self::DEFAULT_DOWNLOAD_FILENAME, + string $contentType = self::DEFAULT_CONTENT_TYPE, + array $headers = [] + ) { + $this->filename = $filename; + $this->contentType = $contentType; + parent::__construct( $this->createBody($body), $status, - $this->prepareDownloadHeaders($filename, $headers) + $this->prepareDownloadHeaders($headers) ); } /** * Get download headers * - * @param string $filename * @return array */ - private function getDownloadHeaders(string $filename): array + private function getDownloadHeaders(): array { $headers = []; $headers['cache-control'] = 'must-revalidate'; $headers['content-description'] = 'File Transfer'; - $headers['content-disposition'] = sprintf('attachment; filename=%s', $filename); + $headers['content-disposition'] = sprintf('attachment; filename=%s', self::DEFAULT_DOWNLOAD_FILENAME); $headers['content-transfer-encoding'] = 'Binary'; $headers['content-type'] = 'application/octet-stream'; $headers['expires'] = '0'; @@ -104,11 +123,16 @@ public function overridesDownloadHeaders(array $downloadHeaders, array $headers /** * Prepare download response headers * - * @param string $filename + * This function prepares the download response headers. It does so by: + * - Merging the optional with over the default ones (the default ones cannot be overridden) + * - Set the content-type and content-disposition headers from $filename and $contentType passed + * to the constructor. + * * @param array $headers * @return array + * @throws InvalidArgumentException if an attempt is made to override a default header */ - private function prepareDownloadHeaders(string $filename, array $headers = []) : array + private function prepareDownloadHeaders(array $headers = []) : array { if ($this->overridesDownloadHeaders($this->downloadResponseHeaders, $headers)) { throw new InvalidArgumentException( @@ -119,7 +143,14 @@ private function prepareDownloadHeaders(string $filename, array $headers = []) : ); } - return array_merge($headers, $this->getDownloadHeaders($filename)); + return array_merge( + $headers, + $this->getDownloadHeaders(), + [ + 'content-disposition' => sprintf('attachment; filename=%s', $this->filename), + 'content-type' => $this->contentType, + ] + ); } /** diff --git a/test/Response/DownloadResponseTest.php b/test/Response/DownloadResponseTest.php index c24c2ae1..f8ab3fe0 100644 --- a/test/Response/DownloadResponseTest.php +++ b/test/Response/DownloadResponseTest.php @@ -83,18 +83,35 @@ public function testCanSendResponseWithCustomFilename() $this->assertSame('attachment; filename=valid.csv', $response->getHeaderLine('content-disposition')); } + public function testCanSendResponseWithCustomContentType() + { + $body = new Stream($this->root->url() . '/files/valid.csv'); + $response = new DownloadResponse($body, 200, 'valid.csv', 'text/csv'); + $this->assertInstanceOf(Response::class, $response); + $this->assertEquals( + file_get_contents($this->root->url() . '/files/valid.csv'), + (string) $response->getBody() + ); + $this->assertHasValidResponseHeaders($response, 'valid.csv', 'text/csv'); + $this->assertSame('attachment; filename=valid.csv', $response->getHeaderLine('content-disposition')); + } + /** * @param DownloadResponse $response * @param string $filename + * @param string $contentType */ - private function assertHasValidResponseHeaders(DownloadResponse $response, $filename = 'download'): void - { + private function assertHasValidResponseHeaders( + DownloadResponse $response, + $filename = 'download', + $contentType = 'application/octet-stream' + ) : void { $requiredHeaders = [ 'cache-control' => 'must-revalidate', 'content-description' => 'File Transfer', 'content-disposition' => sprintf('attachment; filename=%s', $filename), 'content-transfer-encoding' => 'Binary', - 'content-type' => 'application/octet-stream', + 'content-type' => $contentType, 'expires' => '0', 'pragma' => 'Public' ]; From 56ddc755a104d3acb547aa58054248517c156833 Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Thu, 26 Dec 2019 22:44:42 +0100 Subject: [PATCH 10/12] Minor docblock cleanup --- src/Response/DownloadResponse.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Response/DownloadResponse.php b/src/Response/DownloadResponse.php index 4ff86eb2..437846ae 100644 --- a/src/Response/DownloadResponse.php +++ b/src/Response/DownloadResponse.php @@ -156,7 +156,7 @@ private function prepareDownloadHeaders(array $headers = []) : array /** * @param string|StreamInterface $content * @return StreamInterface - * @throws InvalidArgumentException if $body is neither a string nor a stream + * @throws InvalidArgumentException if $body is neither a string nor a Stream */ private function createBody($content): StreamInterface { From 071b3bd21650487ff5d260aadf0da58730d0a475 Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Tue, 31 Dec 2019 10:38:01 +0100 Subject: [PATCH 11/12] Split core of DownloadResponse back into a DownloadResponseTrait I think that I misunderstood what was being asked originally regarding working with a DownloadResponseTrait and a CsvResponse. Given that, this change is being made to create a DownloadResponse but keep the core of the functions in the trait. Not sure if this is the correct approach, but it makes sense to me at the moment. --- src/Response/DownloadResponse.php | 109 +----------------------- src/Response/DownloadResponseTrait.php | 113 +++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 105 deletions(-) create mode 100644 src/Response/DownloadResponseTrait.php diff --git a/src/Response/DownloadResponse.php b/src/Response/DownloadResponse.php index 437846ae..b1f69800 100644 --- a/src/Response/DownloadResponse.php +++ b/src/Response/DownloadResponse.php @@ -1,7 +1,7 @@ overridesDownloadHeaders($this->downloadResponseHeaders, $headers)) { - throw new InvalidArgumentException( - sprintf( - 'Cannot override download headers (%s) when download response is being sent', - implode(', ', $this->downloadResponseHeaders) - ) - ); - } - - return array_merge( - $headers, - $this->getDownloadHeaders(), - [ - 'content-disposition' => sprintf('attachment; filename=%s', $this->filename), - 'content-type' => $this->contentType, - ] - ); - } - /** * @param string|StreamInterface $content * @return StreamInterface diff --git a/src/Response/DownloadResponseTrait.php b/src/Response/DownloadResponseTrait.php new file mode 100644 index 00000000..da738d55 --- /dev/null +++ b/src/Response/DownloadResponseTrait.php @@ -0,0 +1,113 @@ +overridesDownloadHeaders($this->downloadResponseHeaders, $headers)) { + throw new InvalidArgumentException( + sprintf( + 'Cannot override download headers (%s) when download response is being sent', + implode(', ', $this->downloadResponseHeaders) + ) + ); + } + + return array_merge( + $headers, + $this->getDownloadHeaders(), + [ + 'content-disposition' => sprintf('attachment; filename=%s', $this->filename), + 'content-type' => $this->contentType, + ] + ); + } +} From 3ef6022394ed2214f362203b6a5c5887b8a6f0e7 Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Tue, 31 Dec 2019 10:46:42 +0100 Subject: [PATCH 12/12] Remove download functionality from CsvResponse --- src/Response/CsvResponse.php | 22 +------------ test/Response/CsvResponseTest.php | 55 +++---------------------------- 2 files changed, 6 insertions(+), 71 deletions(-) diff --git a/src/Response/CsvResponse.php b/src/Response/CsvResponse.php index 36e46a1b..7e89350c 100644 --- a/src/Response/CsvResponse.php +++ b/src/Response/CsvResponse.php @@ -1,7 +1,7 @@ rewind(); return $body; } - - /** - * Get download headers - * - * @param string $filename - * @return array - */ - private function getDownloadHeaders(string $filename): array - { - $headers = []; - $headers['cache-control'] = ['must-revalidate']; - $headers['content-description'] = ['File Transfer']; - $headers['content-disposition'] = [sprintf('attachment; filename=%s', $filename)]; - $headers['content-transfer-encoding'] = ['Binary']; - $headers['content-type'] = ['text/csv; charset=utf-8']; - $headers['expires'] = ['0']; - $headers['pragma'] = ['Public']; - - return $headers; - } } diff --git a/test/Response/CsvResponseTest.php b/test/Response/CsvResponseTest.php index 5be85ce6..276468a3 100644 --- a/test/Response/CsvResponseTest.php +++ b/test/Response/CsvResponseTest.php @@ -14,6 +14,11 @@ use Psr\Http\Message\StreamInterface; use Zend\Diactoros\Response\CsvResponse; +/** + * Class CsvResponseTest + * @package ZendTest\Diactoros\Response + * @coversDefaultClass \Zend\Diactoros\Response\CsvResponse + */ class CsvResponseTest extends TestCase { const VALID_CSV_BODY = <<assertSame(self::VALID_CSV_BODY, (string) $response->getBody()); } - public function testConstructorAllowsSendingDownloadResponse() - { - $status = 404; - $filename = 'download.csv'; - - $response = new CsvResponse(self::VALID_CSV_BODY, $status, $filename); - $this->assertSame( - [ - 'cache-control' => ['must-revalidate'], - 'content-description' => ['File Transfer'], - 'content-disposition' => [sprintf('attachment; filename=%s', basename($filename))], - 'content-transfer-encoding' => ['Binary'], - 'content-type' => ['text/csv; charset=utf-8'], - 'expires' => ['0'], - 'pragma' => ['Public'], - ], - $response->getHeaders() - ); - $this->assertSame(404, $response->getStatusCode()); - $this->assertSame(self::VALID_CSV_BODY, (string) $response->getBody()); - } - - /** - * @dataProvider invalidHeadersWhenDownloading - */ - public function testConstructorDoesNotAllowsOverridingDownloadHeadersWhenSendingDownloadResponse($header, $value) - { - $status = 404; - $filename = 'download.csv'; - $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage( - 'Cannot override download headers (cache-control, content-description, content-disposition, content-transfer-encoding, expires, pragma) when download response is being sent' - ); - - new CsvResponse(self::VALID_CSV_BODY, $status, $filename, [$header => [$value]]); - - } - - public function invalidHeadersWhenDownloading() - { - return [ - ['cache-control', 'must-revalidate',], - ['content-description', 'File Transfer',], - ['content-disposition', 'upload.csv',], - ['content-transfer-encoding', 'Binary',], - ['expires', '0',], - ['pragma', 'Public',] - ]; - } - public function testConstructorAllowsPassingHeaders() { $status = 404;