From 12204ccda890fb769993f380bdcd3f1afb637cf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jer=C3=B4me=20Bakker?= Date: Tue, 1 Nov 2022 10:44:20 +0100 Subject: [PATCH] feat(core): added download response ref #11178 --- actions/diagnostics/download.php | 11 +- engine/classes/Elgg/Http/DownloadResponse.php | 69 +++++++++++ engine/lib/pagehandler.php | 19 ++++ .../Http/DownloadResponseIntegrationTest.php | 54 +++++++++ .../Elgg/Http/DownloadResponseUnitTest.php | 107 ++++++++++++++++++ ...onseTest.php => ErrorResponseUnitTest.php} | 16 +-- .../unit/Elgg/Http/OkResponseUnitTest.php | 13 ++- .../Elgg/Http/RedirectResponseUnitTest.php | 11 +- ...eUnitTest.php => ResponseUnitTestCase.php} | 36 +++--- views/default/page/elements/html.php | 2 +- views/default/resources/robots.txt.php | 7 +- views/json/page/default.php | 2 +- views/rss/page/default.php | 15 ++- 13 files changed, 301 insertions(+), 61 deletions(-) create mode 100644 engine/classes/Elgg/Http/DownloadResponse.php create mode 100644 engine/tests/phpunit/integration/Elgg/Http/DownloadResponseIntegrationTest.php create mode 100644 engine/tests/phpunit/unit/Elgg/Http/DownloadResponseUnitTest.php rename engine/tests/phpunit/unit/Elgg/Http/{ErrorResponseTest.php => ErrorResponseUnitTest.php} (79%) rename engine/tests/phpunit/unit/Elgg/Http/{ResponseUnitTest.php => ResponseUnitTestCase.php} (87%) diff --git a/actions/diagnostics/download.php b/actions/diagnostics/download.php index b45c3a1255f..a68b0741f63 100644 --- a/actions/diagnostics/download.php +++ b/actions/diagnostics/download.php @@ -9,11 +9,6 @@ $output = elgg_echo('diagnostics:header', [date('r'), elgg_get_logged_in_user_entity()->getDisplayName()]); $output = elgg_trigger_event_results('diagnostics:report', 'system', [], $output); -header("Cache-Control: public"); -header("Content-Description: File Transfer"); -header('Content-disposition: attachment; filename=elggdiagnostic.txt'); -header("Content-Type: text/plain"); -header('Content-Length: ' . strlen($output)); - -echo $output; -exit(); +return elgg_download_response($output, 'elggdiagnostic.txt', false, [ + 'Content-Type' => 'text/plain; charset=utf-8', +]); diff --git a/engine/classes/Elgg/Http/DownloadResponse.php b/engine/classes/Elgg/Http/DownloadResponse.php new file mode 100644 index 00000000000..d0df1d8ae50 --- /dev/null +++ b/engine/classes/Elgg/Http/DownloadResponse.php @@ -0,0 +1,69 @@ +content) && !isset($headers['Content-Length'])) { + $headers['Content-Length'] = strlen((string) $this->content); + } + + return $headers; + } + + /** + * {@inheritDoc} + */ + public function setForwardURL(string $forward_url = REFERRER) { + return $this; + } + + /** + * Set the filename for the download + * + * This will only be applied if the 'Content-Disposition' header isn't already set + * + * @param string $filename The filename when downloaded + * @param bool $inline Is this an inline download (default: false, determines the 'Content-Disposition' header) + * + * @return self + */ + public function setFilename(string $filename = '', bool $inline = false): static { + if (isset($this->headers['Content-Disposition'])) { + return $this; + } + + $disposition = $inline ? 'inline' : 'attachment'; + + if (!elgg_is_empty($filename)) { + $disposition .= "; filename=\"{$filename}\""; + } + + $this->headers['Content-Disposition'] = $disposition; + + return $this; + } +} diff --git a/engine/lib/pagehandler.php b/engine/lib/pagehandler.php index d2e80beaf1f..39544dd3c52 100644 --- a/engine/lib/pagehandler.php +++ b/engine/lib/pagehandler.php @@ -269,3 +269,22 @@ function elgg_error_response(string|array $message = '', string $forward_url = R function elgg_redirect_response(string $forward_url = REFERRER, int $status_code = ELGG_HTTP_FOUND): \Elgg\Http\RedirectResponse { return new Elgg\Http\RedirectResponse($forward_url, $status_code); } + +/** + * Prepare a download response + * + * @param string $content The content of the download + * @param string $filename The filename when downloaded + * @param bool $inline Is this an inline download (default: false, determines the 'Content-Disposition' header) + * @param array $headers (optional) additional headers for the response + * + * @return \Elgg\Http\DownloadResponse + * @since 5.0 + */ +function elgg_download_response(string $content, string $filename = '', bool $inline = false, array $headers = []): \Elgg\Http\DownloadResponse { + $response = new \Elgg\Http\DownloadResponse($content); + $response->setHeaders($headers); + $response->setFilename($filename, $inline); + + return $response; +} diff --git a/engine/tests/phpunit/integration/Elgg/Http/DownloadResponseIntegrationTest.php b/engine/tests/phpunit/integration/Elgg/Http/DownloadResponseIntegrationTest.php new file mode 100644 index 00000000000..d706e1066dc --- /dev/null +++ b/engine/tests/phpunit/integration/Elgg/Http/DownloadResponseIntegrationTest.php @@ -0,0 +1,54 @@ +assertInstanceOf(DownloadResponse::class, $response); + $this->assertEquals($content, $response->getContent()); + + $response_headers = $response->getHeaders(); + $this->assertIsArray($response_headers); + $this->assertArrayHasKey('Content-Disposition', $response_headers); + + if (!isset($headers['Content-Disposition'])) { + if (!empty($filename)) { + $this->assertStringContainsString("filename=\"{$filename}\"", $response_headers['Content-Disposition']); + } else { + $this->assertStringNotContainsString('filename', $response_headers['Content-Disposition']); + } + + if ($inline) { + $this->assertStringContainsString('inline', $response_headers['Content-Disposition']); + } else { + $this->assertStringContainsString('attachment', $response_headers['Content-Disposition']); + } + } + + foreach ($headers as $key => $value) { + $this->assertArrayHasKey($key, $response_headers); + $this->assertEquals($value, $response_headers[$key]); + } + } + + public function getDownloadResponseProvider() { + return [ + ['foo'], + ['foo', 'bar.txt'], + ['foo', 'bar.txt', true], + ['foo', 'bar.txt', false, ['Content-Type' => 'text/plain; charset=utf-8']], + ['foo', 'bar.txt', false, [ + 'Content-Type' => 'text/json; charset=utf-8', + 'Content-Disposition' => 'inline; filename="bar.json"', + ]], + ]; + } +} diff --git a/engine/tests/phpunit/unit/Elgg/Http/DownloadResponseUnitTest.php b/engine/tests/phpunit/unit/Elgg/Http/DownloadResponseUnitTest.php new file mode 100644 index 00000000000..3bbefb1832f --- /dev/null +++ b/engine/tests/phpunit/unit/Elgg/Http/DownloadResponseUnitTest.php @@ -0,0 +1,107 @@ +getReponseClassName(); + $response = new $test_class(); + + $this->assertEquals('', $response->getContent()); + $this->assertEquals(ELGG_HTTP_OK, $response->getStatusCode()); + $this->assertNull($response->getForwardURL()); + $this->assertEquals([ + 'Content-Type' => 'application/octet-stream; charset=utf-8', + 'Cache-Control' => 'no-store', + 'Content-Disposition' => 'attachment', + ], $response->getHeaders()); + } + + public function testCanConstructWithArguments() { + $content = 'foo'; + $status_code = ELGG_HTTP_PARTIAL_CONTENT; + + $test_class = $this->getReponseClassName(); + $response = new $test_class($content, $status_code, REFERRER); + + $this->assertEquals($content, $response->getContent()); + $this->assertEquals($status_code, $response->getStatusCode()); + $this->assertNull($response->getForwardURL()); + $this->assertEquals([ + 'Content-Type' => 'application/octet-stream; charset=utf-8', + 'Cache-Control' => 'no-store', + 'Content-Disposition' => 'attachment', + 'Content-Length' => strlen($content), + ], $response->getHeaders()); + } + + public function testSetFilename() { + $test_class = $this->getReponseClassName(); + $response = new $test_class(); + + // test attachment + $response->setFilename('foo'); + + $headers = $response->getHeaders(); + $this->assertIsArray($headers); + $this->assertArrayHasKey('Content-Disposition', $headers); + $this->assertStringContainsString('filename="foo"', $headers['Content-Disposition']); + $this->assertStringContainsString('attachment', $headers['Content-Disposition']); + + // test inline + $response->setHeaders([]); // need to clean headers + $response->setFilename('bar', true); + + $headers = $response->getHeaders(); + $this->assertIsArray($headers); + $this->assertArrayHasKey('Content-Disposition', $headers); + $this->assertStringContainsString('filename="bar"', $headers['Content-Disposition']); + $this->assertStringContainsString('inline', $headers['Content-Disposition']); + + // test trying to overrule filename fails + $response->setFilename('notset'); + + $headers = $response->getHeaders(); + $this->assertIsArray($headers); + $this->assertArrayHasKey('Content-Disposition', $headers); + $this->assertStringNotContainsString('filename="notset"', $headers['Content-Disposition']); + $this->assertStringNotContainsString('attachment', $headers['Content-Disposition']); + } + + /** + * Overruled tests from parent because of changes to the DownloadResponse + */ + + /** + * @dataProvider validForwardURLsProvider + */ + public function testCanSetForwardURL($value) { + $test_class = $this->getReponseClassName(); + $response = new $test_class(); + + $response->setForwardURL($value); + $this->assertNull($response->getForwardURL()); + } + + public function testCanSetHeaders() { + $test_class = $this->getReponseClassName(); + $response = new $test_class(); + $this->assertEquals([ + 'Content-Type' => 'application/octet-stream; charset=utf-8', + 'Cache-Control' => 'no-store', + 'Content-Disposition' => 'attachment', + ], $response->getHeaders()); + + $response->setHeaders(['Content-Type' => 'application/json']); + $this->assertEquals([ + 'Content-Type' => 'application/json', + 'Cache-Control' => 'no-store', + 'Content-Disposition' => 'attachment', + ], $response->getHeaders()); + } +} diff --git a/engine/tests/phpunit/unit/Elgg/Http/ErrorResponseTest.php b/engine/tests/phpunit/unit/Elgg/Http/ErrorResponseUnitTest.php similarity index 79% rename from engine/tests/phpunit/unit/Elgg/Http/ErrorResponseTest.php rename to engine/tests/phpunit/unit/Elgg/Http/ErrorResponseUnitTest.php index ff19222b157..e06c216ac48 100644 --- a/engine/tests/phpunit/unit/Elgg/Http/ErrorResponseTest.php +++ b/engine/tests/phpunit/unit/Elgg/Http/ErrorResponseUnitTest.php @@ -6,15 +6,16 @@ * @group HttpService * @group UnitTests */ -class ErrorResponseUnitTest extends ResponseUnitTest { +class ErrorResponseUnitTest extends ResponseUnitTestCase { - public function up() { - $this->class = ErrorResponse::class; + public function getReponseClassName(): string { + return ErrorResponse::class; } public function testCanConstructWihtoutArguments() { - $test_class = $this->class; + $test_class = $this->getReponseClassName(); $response = new $test_class(); + $this->assertEquals('', $response->getContent()); $this->assertEquals(ELGG_HTTP_BAD_REQUEST, $response->getStatusCode()); $this->assertEquals(REFERRER, $response->getForwardURL()); @@ -25,8 +26,8 @@ public function testCanConstructWithArguments() { $error = 'foo'; $status_code = ELGG_HTTP_NOT_FOUND; $forward_url = REFERRER; - - $test_class = $this->class; + + $test_class = $this->getReponseClassName(); $response = new $test_class($error, $status_code, $forward_url); $this->assertEquals($error, $response->getContent()); @@ -36,8 +37,9 @@ public function testCanConstructWithArguments() { } public function testConstructWithInvalidStatusCode() { - $test_class = $this->class; + $test_class = $this->getReponseClassName(); $response = new $test_class('foo', 9999); + $this->assertEquals(ELGG_HTTP_INTERNAL_SERVER_ERROR, $response->getStatusCode()); } diff --git a/engine/tests/phpunit/unit/Elgg/Http/OkResponseUnitTest.php b/engine/tests/phpunit/unit/Elgg/Http/OkResponseUnitTest.php index da1e1933dbf..db15c2931de 100644 --- a/engine/tests/phpunit/unit/Elgg/Http/OkResponseUnitTest.php +++ b/engine/tests/phpunit/unit/Elgg/Http/OkResponseUnitTest.php @@ -6,18 +6,19 @@ * @group HttpService * @group UnitTests */ -class OkResponseUnitTest extends ResponseUnitTest { +class OkResponseUnitTest extends ResponseUnitTestCase { - public function up() { - $this->class = OkResponse::class; + public function getReponseClassName(): string { + return OkResponse::class; } public function testCanConstructWihtoutArguments() { - $test_class = $this->class; + $test_class = $this->getReponseClassName(); $response = new $test_class(); + $this->assertEquals('', $response->getContent()); $this->assertEquals(ELGG_HTTP_OK, $response->getStatusCode()); - $this->assertEquals(null, $response->getForwardURL()); + $this->assertNull($response->getForwardURL()); $this->assertEquals([], $response->getHeaders()); } @@ -26,7 +27,7 @@ public function testCanConstructWithArguments() { $status_code = ELGG_HTTP_PARTIAL_CONTENT; $forward_url = REFERRER; - $test_class = $this->class; + $test_class = $this->getReponseClassName(); $response = new $test_class($content, $status_code, $forward_url); $this->assertEquals($content, $response->getContent()); diff --git a/engine/tests/phpunit/unit/Elgg/Http/RedirectResponseUnitTest.php b/engine/tests/phpunit/unit/Elgg/Http/RedirectResponseUnitTest.php index f381309cf74..e163de486f0 100644 --- a/engine/tests/phpunit/unit/Elgg/Http/RedirectResponseUnitTest.php +++ b/engine/tests/phpunit/unit/Elgg/Http/RedirectResponseUnitTest.php @@ -6,15 +6,16 @@ * @group HttpService * @group UnitTests */ -class RedirectResponseUnitTest extends ResponseUnitTest { +class RedirectResponseUnitTest extends ResponseUnitTestCase { - public function up() { - $this->class = RedirectResponse::class; + public function getReponseClassName(): string { + return RedirectResponse::class; } public function testCanConstructWihtoutArguments() { - $test_class = $this->class; + $test_class = $this->getReponseClassName(); $response = new $test_class(); + $this->assertEquals('', $response->getContent()); $this->assertEquals(ELGG_HTTP_FOUND, $response->getStatusCode()); $this->assertEquals(REFERRER, $response->getForwardURL()); @@ -25,7 +26,7 @@ public function testCanConstructWithArguments() { $status_code = ELGG_HTTP_PERMANENTLY_REDIRECT; $forward_url = REFERRER; - $test_class = $this->class; + $test_class = $this->getReponseClassName(); $response = new $test_class($forward_url, $status_code); $this->assertEquals('', $response->getContent()); diff --git a/engine/tests/phpunit/unit/Elgg/Http/ResponseUnitTest.php b/engine/tests/phpunit/unit/Elgg/Http/ResponseUnitTestCase.php similarity index 87% rename from engine/tests/phpunit/unit/Elgg/Http/ResponseUnitTest.php rename to engine/tests/phpunit/unit/Elgg/Http/ResponseUnitTestCase.php index ed629fdbb15..d6a84a72118 100644 --- a/engine/tests/phpunit/unit/Elgg/Http/ResponseUnitTest.php +++ b/engine/tests/phpunit/unit/Elgg/Http/ResponseUnitTestCase.php @@ -3,13 +3,14 @@ namespace Elgg\Http; use Elgg\Exceptions\InvalidArgumentException; + /** * @group HttpService * @group UnitTests */ -abstract class ResponseUnitTest extends \Elgg\UnitTestCase { +abstract class ResponseUnitTestCase extends \Elgg\UnitTestCase { - public $class; + abstract public function getReponseClassName(): string; abstract public function testCanConstructWihtoutArguments(); @@ -19,8 +20,7 @@ abstract public function testCanConstructWithArguments(); * @dataProvider validContentValuesProvider */ public function testCanSetContent($value) { - - $test_class = $this->class; + $test_class = $this->getReponseClassName(); $response = new $test_class(); $response->setContent($value); @@ -45,8 +45,7 @@ public function validContentValuesProvider() { * @dataProvider invalidContentValuesProvider */ public function testThrowsExceptionForInvalidContent($value) { - - $test_class = $this->class; + $test_class = $this->getReponseClassName(); $response = new $test_class(); $this->expectException(InvalidArgumentException::class); @@ -71,8 +70,7 @@ function () { * @dataProvider validStatusCodesProvider */ public function testCanSetStatusCode($value) { - - $test_class = $this->class; + $test_class = $this->getReponseClassName(); $response = new $test_class(); $response->setStatusCode($value); @@ -92,8 +90,7 @@ public function validStatusCodesProvider() { * @dataProvider invalidStatusCodesProvider */ public function testThrowsExceptionForInvalidStatusCodes($value) { - - $test_class = $this->class; + $test_class = $this->getReponseClassName(); $response = new $test_class(); $this->expectException(InvalidArgumentException::class); @@ -116,8 +113,7 @@ public function invalidStatusCodesProvider() { * @dataProvider invalidStatusCodesTypesProvider */ public function testThrowsExceptionForInvalidStatusCodesTypes($value) { - - $test_class = $this->class; + $test_class = $this->getReponseClassName(); $response = new $test_class(); $this->expectException(\TypeError::class); @@ -138,8 +134,7 @@ public function invalidStatusCodesTypesProvider() { * @dataProvider validForwardURLsProvider */ public function testCanSetForwardURL($value) { - - $test_class = $this->class; + $test_class = $this->getReponseClassName(); $response = new $test_class(); $response->setForwardURL($value); @@ -157,8 +152,8 @@ public function validForwardURLsProvider() { } public function testCanSetHeaders() { - - $response = new $this->class(); + $test_class = $this->getReponseClassName(); + $response = new $test_class(); $this->assertEquals([], $response->getHeaders()); $response->setHeaders(['Content-Type' => 'application/json']); @@ -169,9 +164,8 @@ public function testCanSetHeaders() { * @dataProvider statusCodesProvider */ public function testCanResolveStatusCodes($code, $status) { - - $test_class = $this->class; - $response = new $test_class; + $test_class = $this->getReponseClassName(); + $response = new $test_class(); $response->setStatusCode($code); $this->assertEquals($status[0], $response->isInformational()); @@ -184,7 +178,6 @@ public function testCanResolveStatusCodes($code, $status) { } public function statusCodesProvider() { - $codes = []; foreach (range(100, 599) as $code) { $codes[] = [ @@ -219,8 +212,7 @@ public function statusCodesProvider() { } public function testCanSetException() { - - $test_class = $this->class; + $test_class = $this->getReponseClassName(); $response = new $test_class(); $ex = new \Exception('foo'); diff --git a/views/default/page/elements/html.php b/views/default/page/elements/html.php index 77db9e99ea9..5e65769d5f7 100644 --- a/views/default/page/elements/html.php +++ b/views/default/page/elements/html.php @@ -8,7 +8,7 @@ * @uses $vars['body'] The main content of the page */ // Set the content type -elgg_set_http_header("Content-type: text/html; charset=UTF-8"); +elgg_set_http_header("Content-type: text/html; charset=utf-8"); $lang = elgg_get_current_language(); diff --git a/views/default/resources/robots.txt.php b/views/default/resources/robots.txt.php index a69a9520485..8e41b6cdb02 100644 --- a/views/default/resources/robots.txt.php +++ b/views/default/resources/robots.txt.php @@ -1,11 +1,12 @@ getMetadata('robots.txt'); $plugin_content = elgg_trigger_event_results('robots.txt', 'site', ['site' => $site], ''); -if ($plugin_content) { - $content = $content . "\n\n" . $plugin_content; +if (!empty($plugin_content) && is_string($plugin_content)) { + $content .= PHP_EOL . PHP_EOL . $plugin_content; } + echo $content; diff --git a/views/json/page/default.php b/views/json/page/default.php index b9e399abe8a..7b36aca093f 100644 --- a/views/json/page/default.php +++ b/views/json/page/default.php @@ -5,6 +5,6 @@ * @uses $vars['body'] */ -elgg_set_http_header("Content-Type: application/json;charset=utf-8"); +elgg_set_http_header("Content-Type: application/json; charset=utf-8"); echo elgg_extract('body', $vars, ''); diff --git a/views/rss/page/default.php b/views/rss/page/default.php index 7713bb52b90..ec03e2afb61 100644 --- a/views/rss/page/default.php +++ b/views/rss/page/default.php @@ -33,21 +33,20 @@ $namespaces = elgg_view('extensions/xmlns'); $extensions = elgg_view('extensions/channel'); - // allow caching as required by stupid MS products for https feeds. elgg_set_http_header('Pragma: public'); -elgg_set_http_header("Content-Type: text/xml;charset=utf-8"); +elgg_set_http_header("Content-Type: text/xml; charset=utf-8"); echo ""; echo << - <![CDATA[$title]]> - $url - - - $extensions - $body + <![CDATA[{$title}]]> + {$url} + + + {$extensions} + {$body} END;