From 73fe7fa7b2e1fdbbf412669498dd16bea86dfb83 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 14:13:47 +0900 Subject: [PATCH 01/17] docs: improve PHPDoc comments --- system/API/ResponseTrait.php | 12 ++++++++---- system/RESTful/ResourceController.php | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/system/API/ResponseTrait.php b/system/API/ResponseTrait.php index 30b5165d8e..ece7effb0a 100644 --- a/system/API/ResponseTrait.php +++ b/system/API/ResponseTrait.php @@ -64,10 +64,11 @@ trait ResponseTrait /** * How to format the response data. - * Either 'json' or 'xml'. If blank will be - * determined through content negotiation. + * Either 'json' or 'xml'. If null is set, it will be determined through + * content negotiation. * - * @var string + * @var string|null + * @phpstan-var 'json'|'xml'|null */ protected $format = 'json'; @@ -294,7 +295,7 @@ trait ResponseTrait // -------------------------------------------------------------------- /** - * Handles formatting a response. Currently makes some heavy assumptions + * Handles formatting a response. Currently, makes some heavy assumptions * and needs updating! :) * * @param array|string|null $data @@ -350,6 +351,9 @@ trait ResponseTrait /** * Sets the format the response should be in. * + * @param string|null $format Response format + * @phpstan-param 'json'|'xml' $format + * * @return $this */ protected function setResponseFormat(?string $format = null) diff --git a/system/RESTful/ResourceController.php b/system/RESTful/ResourceController.php index 3409bab6d4..072b52ee82 100644 --- a/system/RESTful/ResourceController.php +++ b/system/RESTful/ResourceController.php @@ -106,7 +106,7 @@ class ResourceController extends BaseResource /** * Set/change the expected response representation for returned objects * - * @param string $format json/xml + * @param string $format Response format * @phpstan-param 'json'|'xml' $format * * @return void From 9a77d28cf9c34903c7e52ba575ad1131da3c6dce Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 14:14:14 +0900 Subject: [PATCH 02/17] fix: type error PHP Deprecated: strtolower(): Passing null to parameter #1 ($string) of type string is deprecated --- system/API/ResponseTrait.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/API/ResponseTrait.php b/system/API/ResponseTrait.php index ece7effb0a..ae2183e55c 100644 --- a/system/API/ResponseTrait.php +++ b/system/API/ResponseTrait.php @@ -358,7 +358,7 @@ trait ResponseTrait */ protected function setResponseFormat(?string $format = null) { - $this->format = strtolower($format); + $this->format = ($format === null) ? null : strtolower($format); return $this; } From 2e87a5814b015287cf5c7b782e8b61a49ed17122 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 14:30:46 +0900 Subject: [PATCH 03/17] test: break long line --- tests/system/API/ResponseTraitTest.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/system/API/ResponseTraitTest.php b/tests/system/API/ResponseTraitTest.php index a168cc4071..459e204d51 100644 --- a/tests/system/API/ResponseTraitTest.php +++ b/tests/system/API/ResponseTraitTest.php @@ -75,7 +75,12 @@ final class ResponseTraitTest extends CIUnitTestCase Factories::injectMock('config', 'Cookie', $cookie); if ($this->request === null) { - $this->request = new MockIncomingRequest($config, new SiteURI($config, $routePath), null, new UserAgent()); + $this->request = new MockIncomingRequest( + $config, + new SiteURI($config, $routePath), + null, + new UserAgent() + ); $this->response = new MockResponse($config); } From 74560befe2df22ff2db2c6cd90cd7ab468d88873 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 14:47:37 +0900 Subject: [PATCH 04/17] test: replace deprecated Response::getReason() --- tests/system/API/ResponseTraitTest.php | 34 +++++++++++++------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/system/API/ResponseTraitTest.php b/tests/system/API/ResponseTraitTest.php index 459e204d51..d40928960b 100644 --- a/tests/system/API/ResponseTraitTest.php +++ b/tests/system/API/ResponseTraitTest.php @@ -126,7 +126,7 @@ final class ResponseTraitTest extends CIUnitTestCase $this->invoke($controller, 'respondCreated', [['id' => 3], 'A Custom Reason']); - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); $this->assertSame(201, $this->response->getStatusCode()); $expected = <<<'EOH' @@ -233,7 +233,7 @@ final class ResponseTraitTest extends CIUnitTestCase $this->assertSame(201, $this->response->getStatusCode()); $this->assertSame('something', $this->response->getBody()); $this->assertStringStartsWith('text/html', $this->response->getHeaderLine('Content-Type')); - $this->assertSame('Created', $this->response->getReason()); + $this->assertSame('Created', $this->response->getReasonPhrase()); } public function testRespondWithCustomReason(): void @@ -243,7 +243,7 @@ final class ResponseTraitTest extends CIUnitTestCase $this->invoke($controller, 'respond', ['something', 201, 'A Custom Reason']); $this->assertSame(201, $this->response->getStatusCode()); - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); } public function testFailSingleMessage(): void @@ -262,7 +262,7 @@ final class ResponseTraitTest extends CIUnitTestCase ]; $this->assertSame($this->formatter->format($expected), $this->response->getBody()); $this->assertSame(500, $this->response->getStatusCode()); - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); } public function testCreated(): void @@ -271,7 +271,7 @@ final class ResponseTraitTest extends CIUnitTestCase $this->invoke($controller, 'respondCreated', [['id' => 3], 'A Custom Reason']); - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); $this->assertSame(201, $this->response->getStatusCode()); $this->assertSame($this->formatter->format(['id' => 3]), $this->response->getBody()); } @@ -282,7 +282,7 @@ final class ResponseTraitTest extends CIUnitTestCase $this->invoke($controller, 'respondDeleted', [['id' => 3], 'A Custom Reason']); - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); $this->assertSame(200, $this->response->getStatusCode()); $this->assertSame($this->formatter->format(['id' => 3]), $this->response->getBody()); } @@ -293,7 +293,7 @@ final class ResponseTraitTest extends CIUnitTestCase $this->invoke($controller, 'respondUpdated', [['id' => 3], 'A Custom Reason']); - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); $this->assertSame(200, $this->response->getStatusCode()); $this->assertSame($this->formatter->format(['id' => 3]), $this->response->getBody()); } @@ -311,7 +311,7 @@ final class ResponseTraitTest extends CIUnitTestCase 'error' => 'Nope', ], ]; - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); $this->assertSame(401, $this->response->getStatusCode()); $this->assertSame($this->formatter->format($expected), $this->response->getBody()); } @@ -329,7 +329,7 @@ final class ResponseTraitTest extends CIUnitTestCase 'error' => 'Nope', ], ]; - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); $this->assertSame(403, $this->response->getStatusCode()); $this->assertSame($this->formatter->format($expected), $this->response->getBody()); } @@ -341,7 +341,7 @@ final class ResponseTraitTest extends CIUnitTestCase $this->invoke($controller, 'respondNoContent', ['']); $this->assertStringStartsWith('application/json', $this->response->getHeaderLine('Content-Type')); - $this->assertSame('No Content', $this->response->getReason()); + $this->assertSame('No Content', $this->response->getReasonPhrase()); $this->assertSame(204, $this->response->getStatusCode()); } @@ -358,7 +358,7 @@ final class ResponseTraitTest extends CIUnitTestCase 'error' => 'Nope', ], ]; - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); $this->assertSame(404, $this->response->getStatusCode()); $this->assertSame($this->formatter->format($expected), $this->response->getBody()); } @@ -376,7 +376,7 @@ final class ResponseTraitTest extends CIUnitTestCase 'error' => 'Nope', ], ]; - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); $this->assertSame(400, $this->response->getStatusCode()); $this->assertSame($this->formatter->format($expected), $this->response->getBody()); } @@ -395,7 +395,7 @@ final class ResponseTraitTest extends CIUnitTestCase 'bar' => 'No way', ], ]; - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); $this->assertSame(400, $this->response->getStatusCode()); $this->assertSame($this->formatter->format($expected), $this->response->getBody()); } @@ -413,7 +413,7 @@ final class ResponseTraitTest extends CIUnitTestCase 'error' => 'Nope', ], ]; - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); $this->assertSame(409, $this->response->getStatusCode()); $this->assertSame($this->formatter->format($expected), $this->response->getBody()); } @@ -431,7 +431,7 @@ final class ResponseTraitTest extends CIUnitTestCase 'error' => 'Nope', ], ]; - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); $this->assertSame(410, $this->response->getStatusCode()); $this->assertSame($this->formatter->format($expected), $this->response->getBody()); } @@ -449,7 +449,7 @@ final class ResponseTraitTest extends CIUnitTestCase 'error' => 'Nope', ], ]; - $this->assertSame('A Custom Reason', $this->response->getReason()); + $this->assertSame('A Custom Reason', $this->response->getReasonPhrase()); $this->assertSame(429, $this->response->getStatusCode()); $this->assertSame($this->formatter->format($expected), $this->response->getBody()); } @@ -460,7 +460,7 @@ final class ResponseTraitTest extends CIUnitTestCase $this->invoke($controller, 'failServerError', ['Nope.', 'FAT-CHANCE', 'A custom reason.']); - $this->assertSame('A custom reason.', $this->response->getReason()); + $this->assertSame('A custom reason.', $this->response->getReasonPhrase()); $this->assertSame(500, $this->response->getStatusCode()); $this->assertSame($this->formatter->format([ 'status' => 500, From d51dca323e77bd18ad1ab11e3100c9fa5426bd68 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 14:43:05 +0900 Subject: [PATCH 05/17] test: move empty lines --- tests/system/API/ResponseTraitTest.php | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/system/API/ResponseTraitTest.php b/tests/system/API/ResponseTraitTest.php index d40928960b..5f0a5564eb 100644 --- a/tests/system/API/ResponseTraitTest.php +++ b/tests/system/API/ResponseTraitTest.php @@ -151,8 +151,8 @@ final class ResponseTraitTest extends CIUnitTestCase { $this->formatter = null; $controller = $this->makeController(); - $payload = ['answer' => 42]; + $payload = ['answer' => 42]; $this->invoke($controller, 'respond', [$payload]); $expected = <<<'EOH' @@ -167,12 +167,12 @@ final class ResponseTraitTest extends CIUnitTestCase { $this->formatter = null; $controller = $this->makeController(); - $payload = [ + + $payload = [ 1, 2, 3, ]; - $this->invoke($controller, 'respond', [$payload]); $expected = <<<'EOH' @@ -189,9 +189,10 @@ final class ResponseTraitTest extends CIUnitTestCase { $this->formatter = null; $controller = $this->makeController(); - $payload = new stdClass(); - $payload->name = 'Tom'; - $payload->id = 1; + + $payload = new stdClass(); + $payload->name = 'Tom'; + $payload->id = 1; $this->invoke($controller, 'respond', [(array) $payload]); From 4831fbc308f2a334d81f72a19519c8623bd0b471 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 14:44:59 +0900 Subject: [PATCH 06/17] test: break long lines --- tests/system/API/ResponseTraitTest.php | 33 +++++++++++++++++++++----- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/tests/system/API/ResponseTraitTest.php b/tests/system/API/ResponseTraitTest.php index 5f0a5564eb..ba5b144e69 100644 --- a/tests/system/API/ResponseTraitTest.php +++ b/tests/system/API/ResponseTraitTest.php @@ -386,7 +386,11 @@ final class ResponseTraitTest extends CIUnitTestCase { $controller = $this->makeController(); - $this->invoke($controller, 'failValidationErrors', [['foo' => 'Nope', 'bar' => 'No way'], 'FAT CHANCE', 'A Custom Reason']); + $this->invoke( + $controller, + 'failValidationErrors', + [['foo' => 'Nope', 'bar' => 'No way'], 'FAT CHANCE', 'A Custom Reason'] + ); $expected = [ 'status' => 400, @@ -493,10 +497,18 @@ final class ResponseTraitTest extends CIUnitTestCase $_SERVER['CONTENT_TYPE'] = $mimeType; $this->makeController([], '', ['Accept' => $mimeType]); - $this->assertSame($mimeType, $this->request->getHeaderLine('Accept'), 'Request header...'); + $this->assertSame( + $mimeType, + $this->request->getHeaderLine('Accept'), + 'Request header...' + ); $this->response->setContentType($contentType); - $this->assertSame($contentType, $this->response->getHeaderLine('Content-Type'), 'Response header pre-response...'); + $this->assertSame( + $contentType, + $this->response->getHeaderLine('Content-Type'), + 'Response header pre-response...' + ); $_SERVER = $original; } @@ -583,7 +595,10 @@ final class ResponseTraitTest extends CIUnitTestCase $this->invoke($controller, 'respondCreated', [['id' => 3], 'A Custom Reason']); - $this->assertStringStartsWith(config('Format')->supportedResponseFormats[0], $response->getHeaderLine('Content-Type')); + $this->assertStringStartsWith( + config('Format')->supportedResponseFormats[0], + $response->getHeaderLine('Content-Type') + ); } public function testResponseFormat(): void @@ -594,13 +609,19 @@ final class ResponseTraitTest extends CIUnitTestCase $this->invoke($controller, 'setResponseFormat', ['json']); $this->invoke($controller, 'respond', [$data, 201]); - $this->assertStringStartsWith('application/json', $this->response->getHeaderLine('Content-Type')); + $this->assertStringStartsWith( + 'application/json', + $this->response->getHeaderLine('Content-Type') + ); $this->assertSame($this->formatter->format($data), $this->response->getJSON()); $this->invoke($controller, 'setResponseFormat', ['xml']); $this->invoke($controller, 'respond', [$data, 201]); - $this->assertStringStartsWith('application/xml', $this->response->getHeaderLine('Content-Type')); + $this->assertStringStartsWith( + 'application/xml', + $this->response->getHeaderLine('Content-Type') + ); } public function testXMLResponseFormat(): void From b1fab78c5e12afcfa6772d1547527331acca7bdf Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 15:00:17 +0900 Subject: [PATCH 07/17] test: remove unneeded `&` --- tests/system/API/ResponseTraitTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/system/API/ResponseTraitTest.php b/tests/system/API/ResponseTraitTest.php index ba5b144e69..391b43f7e5 100644 --- a/tests/system/API/ResponseTraitTest.php +++ b/tests/system/API/ResponseTraitTest.php @@ -105,7 +105,7 @@ final class ResponseTraitTest extends CIUnitTestCase protected $response; protected $formatter; - public function __construct(&$request, &$response, &$formatter) + public function __construct($request, $response, $formatter) { $this->request = $request; $this->response = $response; @@ -584,7 +584,7 @@ final class ResponseTraitTest extends CIUnitTestCase protected $request; protected $response; - public function __construct(&$request, &$response) + public function __construct($request, $response) { $this->request = $request; $this->response = $response; From e273b5ad8955d37d17d608ff0b9529f4ca45934d Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 15:20:52 +0900 Subject: [PATCH 08/17] refactor: first check mime and will handle string data as JSON. --- system/API/ResponseTrait.php | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/system/API/ResponseTrait.php b/system/API/ResponseTrait.php index ae2183e55c..5ff36a0f56 100644 --- a/system/API/ResponseTrait.php +++ b/system/API/ResponseTrait.php @@ -304,20 +304,13 @@ trait ResponseTrait */ protected function format($data = null) { - // If the data is a string, there's not much we can do to it... - if (is_string($data)) { - // The content type should be text/... and not application/... - $contentType = $this->response->getHeaderLine('Content-Type'); - $contentType = str_replace('application/json', 'text/html', $contentType); - $contentType = str_replace('application/', 'text/', $contentType); - $this->response->setContentType($contentType); - $this->format = 'html'; - - return $data; - } - $format = Services::format(); - $mime = "application/{$this->format}"; + + if ($this->format === null) { + $mime = $format->getConfig()->supportedResponseFormats[0]; + } else { + $mime = "application/{$this->format}"; + } // Determine correct response type through content negotiation if not explicitly declared if ( @@ -339,6 +332,18 @@ trait ResponseTrait $this->formatter = $format->getFormatter($mime); } + // If the data is a string, there's not much we can do to it... + if (is_string($data)) { + // The content type should be text/... and not application/... + $contentType = $this->response->getHeaderLine('Content-Type'); + $contentType = str_replace('application/json', 'text/html', $contentType); + $contentType = str_replace('application/', 'text/', $contentType); + $this->response->setContentType($contentType); + $this->format = 'html'; + + return $data; + } + if ($mime !== 'application/json') { // Recursively convert objects into associative arrays // Conversion not required for JSONFormatter From 21dc53c77edff9cfbb77a2aed9ba6399742aba37 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 15:43:31 +0900 Subject: [PATCH 09/17] test: remove code to set response content type Test code should not set the conetnt type. ResponseTrait has the responsibility. --- tests/system/API/ResponseTraitTest.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/system/API/ResponseTraitTest.php b/tests/system/API/ResponseTraitTest.php index 391b43f7e5..b7f4ed2111 100644 --- a/tests/system/API/ResponseTraitTest.php +++ b/tests/system/API/ResponseTraitTest.php @@ -92,9 +92,6 @@ final class ResponseTraitTest extends CIUnitTestCase foreach ($headers as $key => $value) { $this->request->setHeader($key, $value); - if (($key === 'Accept') && ! is_array($value)) { - $this->response->setContentType($value); - } } // Create the controller class finally. From 8c7a905523c4829a07996004e3aa5c9faa9120bc Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 15:56:24 +0900 Subject: [PATCH 10/17] test: extract createAppConfig() and createCookieConfig() --- tests/system/API/ResponseTraitTest.php | 44 ++++++++++---------------- 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/tests/system/API/ResponseTraitTest.php b/tests/system/API/ResponseTraitTest.php index b7f4ed2111..49383b21c1 100644 --- a/tests/system/API/ResponseTraitTest.php +++ b/tests/system/API/ResponseTraitTest.php @@ -44,7 +44,7 @@ final class ResponseTraitTest extends CIUnitTestCase $this->formatter = new JSONFormatter(); } - protected function makeController(array $userConfig = [], string $routePath = '', array $userHeaders = []) + private function createAppConfig(): App { $config = new App(); @@ -60,6 +60,11 @@ final class ResponseTraitTest extends CIUnitTestCase $config->{$key} = $value; } + return $config; + } + + private function createCookieConfig(): Cookie + { $cookie = new Cookie(); foreach ([ @@ -74,6 +79,14 @@ final class ResponseTraitTest extends CIUnitTestCase } Factories::injectMock('config', 'Cookie', $cookie); + return $cookie; + } + + protected function makeController(array $userConfig = [], string $routePath = '', array $userHeaders = []) + { + $config = $this->createAppConfig(); + $cookie = $this->createCookieConfig(); + if ($this->request === null) { $this->request = new MockIncomingRequest( $config, @@ -544,33 +557,8 @@ final class ResponseTraitTest extends CIUnitTestCase public function testFormatByRequestNegotiateIfFormatIsNotJsonOrXML(): void { - $config = new App(); - - foreach ([ - 'baseURL' => 'http://example.com/', - 'uriProtocol' => 'REQUEST_URI', - 'defaultLocale' => 'en', - 'negotiateLocale' => false, - 'supportedLocales' => ['en'], - 'CSPEnabled' => false, - 'proxyIPs' => [], - ] as $key => $value) { - $config->{$key} = $value; - } - - $cookie = new Cookie(); - - foreach ([ - 'prefix' => '', - 'domain' => '', - 'path' => '/', - 'secure' => false, - 'httponly' => false, - 'samesite' => 'Lax', - ] as $key => $value) { - $cookie->{$key} = $value; - } - Factories::injectMock('config', 'Cookie', $cookie); + $config = $this->createAppConfig(); + $cookie = $this->createCookieConfig(); $request = new MockIncomingRequest($config, new SiteURI($config), null, new UserAgent()); $response = new MockResponse($config); From e7bc4cec2f2e1818edc53ac9003c00ee632be541 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 16:00:08 +0900 Subject: [PATCH 11/17] test: remove unused param $userConfig --- tests/system/API/ResponseTraitTest.php | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/system/API/ResponseTraitTest.php b/tests/system/API/ResponseTraitTest.php index 49383b21c1..602c437adf 100644 --- a/tests/system/API/ResponseTraitTest.php +++ b/tests/system/API/ResponseTraitTest.php @@ -82,7 +82,7 @@ final class ResponseTraitTest extends CIUnitTestCase return $cookie; } - protected function makeController(array $userConfig = [], string $routePath = '', array $userHeaders = []) + protected function makeController(string $routePath = '', array $userHeaders = []) { $config = $this->createAppConfig(); $cookie = $this->createCookieConfig(); @@ -132,7 +132,10 @@ final class ResponseTraitTest extends CIUnitTestCase public function testNoFormatterJSON(): void { $this->formatter = null; - $controller = $this->makeController([], '', ['Accept' => 'application/json']); + $controller = $this->makeController( + '', + ['Accept' => 'application/json'] + ); $this->invoke($controller, 'respondCreated', [['id' => 3], 'A Custom Reason']); @@ -150,7 +153,10 @@ final class ResponseTraitTest extends CIUnitTestCase public function testNoFormatter(): void { $this->formatter = null; - $controller = $this->makeController([], '', ['Accept' => 'application/json']); + $controller = $this->makeController( + '', + ['Accept' => 'application/json'] + ); $this->invoke($controller, 'respondCreated', ['A Custom Reason']); @@ -506,7 +512,7 @@ final class ResponseTraitTest extends CIUnitTestCase $original = $_SERVER; $_SERVER['CONTENT_TYPE'] = $mimeType; - $this->makeController([], '', ['Accept' => $mimeType]); + $this->makeController('', ['Accept' => $mimeType]); $this->assertSame( $mimeType, $this->request->getHeaderLine('Accept'), From a29c5f29ce0f34b957fb11a578ae9d35ab59748a Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 16:02:40 +0900 Subject: [PATCH 12/17] test: extract createRequestAndResponse() --- tests/system/API/ResponseTraitTest.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/system/API/ResponseTraitTest.php b/tests/system/API/ResponseTraitTest.php index 602c437adf..e4b93065f2 100644 --- a/tests/system/API/ResponseTraitTest.php +++ b/tests/system/API/ResponseTraitTest.php @@ -82,7 +82,7 @@ final class ResponseTraitTest extends CIUnitTestCase return $cookie; } - protected function makeController(string $routePath = '', array $userHeaders = []) + private function createRequestAndResponse(string $routePath = '', array $userHeaders = []): void { $config = $this->createAppConfig(); $cookie = $this->createCookieConfig(); @@ -106,6 +106,11 @@ final class ResponseTraitTest extends CIUnitTestCase foreach ($headers as $key => $value) { $this->request->setHeader($key, $value); } + } + + protected function makeController(string $routePath = '', array $userHeaders = []) + { + $this->createRequestAndResponse($routePath, $userHeaders); // Create the controller class finally. return new class ($this->request, $this->response, $this->formatter) { From 61a4694d799d76d824bd873afde59bc253823cc1 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 16:23:30 +0900 Subject: [PATCH 13/17] fix: ResponseTrait can't return string data as JSON --- system/API/ResponseTrait.php | 14 ++++-- tests/system/API/ResponseTraitTest.php | 66 +++++++++++++++++++++++++- 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/system/API/ResponseTrait.php b/system/API/ResponseTrait.php index 5ff36a0f56..07fe43cefc 100644 --- a/system/API/ResponseTrait.php +++ b/system/API/ResponseTrait.php @@ -22,6 +22,9 @@ use Config\Services; * Provides common, more readable, methods to provide * consistent HTTP responses under a variety of common * situations when working as an API. + * + * @property bool $stringAsHtml Whether to treat string data as HTML in JSON response. + * Setting `true` is only for backward compatibility. */ trait ResponseTrait { @@ -68,7 +71,7 @@ trait ResponseTrait * content negotiation. * * @var string|null - * @phpstan-var 'json'|'xml'|null + * @phpstan-var 'html'|'json'|'xml'|null */ protected $format = 'json'; @@ -332,8 +335,13 @@ trait ResponseTrait $this->formatter = $format->getFormatter($mime); } - // If the data is a string, there's not much we can do to it... - if (is_string($data)) { + $asHtml = $this->stringAsHtml ?? false; + + // Returns as HTML. + if ( + ($mime === 'application/json' && $asHtml && is_string($data)) + || ($mime !== 'application/json' && is_string($data)) + ) { // The content type should be text/... and not application/... $contentType = $this->response->getHeaderLine('Content-Type'); $contentType = str_replace('application/json', 'text/html', $contentType); diff --git a/tests/system/API/ResponseTraitTest.php b/tests/system/API/ResponseTraitTest.php index e4b93065f2..b54f3037b2 100644 --- a/tests/system/API/ResponseTraitTest.php +++ b/tests/system/API/ResponseTraitTest.php @@ -85,7 +85,7 @@ final class ResponseTraitTest extends CIUnitTestCase private function createRequestAndResponse(string $routePath = '', array $userHeaders = []): void { $config = $this->createAppConfig(); - $cookie = $this->createCookieConfig(); + $this->createCookieConfig(); if ($this->request === null) { $this->request = new MockIncomingRequest( @@ -165,7 +165,38 @@ final class ResponseTraitTest extends CIUnitTestCase $this->invoke($controller, 'respondCreated', ['A Custom Reason']); + $this->assertSame('"A Custom Reason"', $this->response->getBody()); + } + + public function testNoFormatterWithStringAsHtmlTrue(): void + { + $this->formatter = null; + + $this->createRequestAndResponse('', ['Accept' => 'application/json']); + + $controller = new class ($this->request, $this->response, $this->formatter) { + use ResponseTrait; + + protected $request; + protected $response; + protected $formatter; + protected bool $stringAsHtml = true; + + public function __construct($request, $response, $formatter) + { + $this->request = $request; + $this->response = $response; + $this->formatter = $formatter; + } + }; + + $this->invoke($controller, 'respondCreated', ['A Custom Reason']); + $this->assertSame('A Custom Reason', $this->response->getBody()); + $this->assertStringStartsWith( + 'text/html', + $this->response->getHeaderLine('Content-Type') + ); } public function testAssociativeArrayPayload(): void @@ -252,6 +283,37 @@ final class ResponseTraitTest extends CIUnitTestCase $this->invoke($controller, 'respond', ['something', 201]); + $this->assertSame(201, $this->response->getStatusCode()); + $this->assertSame('"something"', $this->response->getBody()); + $this->assertStringStartsWith( + 'application/json', + $this->response->getHeaderLine('Content-Type') + ); + $this->assertSame('Created', $this->response->getReasonPhrase()); + } + + public function testRespondSetsCorrectBodyAndStatusWithStringAsHtmlTrue(): void + { + $this->createRequestAndResponse(); + + $controller = new class ($this->request, $this->response, $this->formatter) { + use ResponseTrait; + + protected $request; + protected $response; + protected $formatter; + protected bool $stringAsHtml = true; + + public function __construct($request, $response, $formatter) + { + $this->request = $request; + $this->response = $response; + $this->formatter = $formatter; + } + }; + + $this->invoke($controller, 'respond', ['something', 201]); + $this->assertSame(201, $this->response->getStatusCode()); $this->assertSame('something', $this->response->getBody()); $this->assertStringStartsWith('text/html', $this->response->getHeaderLine('Content-Type')); @@ -569,7 +631,7 @@ final class ResponseTraitTest extends CIUnitTestCase public function testFormatByRequestNegotiateIfFormatIsNotJsonOrXML(): void { $config = $this->createAppConfig(); - $cookie = $this->createCookieConfig(); + $this->createCookieConfig(); $request = new MockIncomingRequest($config, new SiteURI($config), null, new UserAgent()); $response = new MockResponse($config); From 8df04e817d9125d1e95984c5f6a1bb58810e60c1 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 16:46:01 +0900 Subject: [PATCH 14/17] docs: update user guide --- user_guide_src/source/outgoing/api_responses.rst | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/user_guide_src/source/outgoing/api_responses.rst b/user_guide_src/source/outgoing/api_responses.rst index 5735db7fb8..6654a190da 100644 --- a/user_guide_src/source/outgoing/api_responses.rst +++ b/user_guide_src/source/outgoing/api_responses.rst @@ -31,12 +31,17 @@ Handling Response Types When you pass your data in any of these methods, they will determine the data type to format the results as based on the following criteria: -* If data is a string, it will be treated as HTML to send back to the client. -* If data is an array, it will be formatted according to the controller's ``$this->format`` value. If that is empty, - it will try to negotiate the content type with what the client asked for, defaulting to JSON - if nothing else has been specified within **Config/Format.php**, the ``$supportedResponseFormats`` property. +* The format is determined according to the controller's ``$this->format`` value. + If that is ``null``, it will try to negotiate the content type with what the + client asked for, defaulting to the first element (JSON by default) in the + ``$supportedResponseFormats`` property within **app/Config/Format.php**. +* The data will be formatted according to the format. If the format is not JSON + and data is a string, it will be treated as HTML to send back to the client. -To define the formatter that is used, edit **Config/Format.php**. The ``$supportedResponseFormats`` contains a list of +.. note:: Prior to v4.5.0, due to a bug, if data is a string, it will be treated + as HTML even if the format is JSON. + +To define the formatter that is used, edit **app/Config/Format.php**. The ``$supportedResponseFormats`` contains a list of mime types that your application can automatically format the response for. By default, the system knows how to format both XML and JSON responses: From 2d2318b239948dbad955d7269c2df5cc7d1790e8 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 17:33:17 +0900 Subject: [PATCH 15/17] refactor: use ternary operator --- system/API/ResponseTrait.php | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/system/API/ResponseTrait.php b/system/API/ResponseTrait.php index 07fe43cefc..6342db73b7 100644 --- a/system/API/ResponseTrait.php +++ b/system/API/ResponseTrait.php @@ -309,11 +309,8 @@ trait ResponseTrait { $format = Services::format(); - if ($this->format === null) { - $mime = $format->getConfig()->supportedResponseFormats[0]; - } else { - $mime = "application/{$this->format}"; - } + $mime = ($this->format === null) ? $format->getConfig()->supportedResponseFormats[0] + : "application/{$this->format}"; // Determine correct response type through content negotiation if not explicitly declared if ( From 654616f1054910cc222d2e23ca0c192e6bf9622d Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 31 Jan 2024 17:44:11 +0900 Subject: [PATCH 16/17] test: update failed test --- tests/system/Test/ControllerTestTraitTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/system/Test/ControllerTestTraitTest.php b/tests/system/Test/ControllerTestTraitTest.php index b995930acf..fae14d2629 100644 --- a/tests/system/Test/ControllerTestTraitTest.php +++ b/tests/system/Test/ControllerTestTraitTest.php @@ -175,9 +175,9 @@ final class ControllerTestTraitTest extends CIUnitTestCase ->controller(Popcorn::class) ->execute('weasel'); - $body = $result->response()->getBody(); // empty - $this->assertEmpty($body); - $this->assertFalse($result->isOK()); + $body = $result->response()->getBody(); // empty string as JSON + $this->assertSame('""', $body); + $this->assertTrue($result->isOK()); } public function testRedirect(): void From fac0fe226e12d998601a3d4aa496b033e12c7001 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 2 Feb 2024 10:19:14 +0900 Subject: [PATCH 17/17] docs: add changelog and upgrade --- user_guide_src/source/changelogs/v4.5.0.rst | 7 +++++++ user_guide_src/source/installation/upgrade_450.rst | 14 ++++++++++++++ user_guide_src/source/outgoing/api_responses.rst | 2 ++ 3 files changed, 23 insertions(+) diff --git a/user_guide_src/source/changelogs/v4.5.0.rst b/user_guide_src/source/changelogs/v4.5.0.rst index be8e048c49..459bea88e2 100644 --- a/user_guide_src/source/changelogs/v4.5.0.rst +++ b/user_guide_src/source/changelogs/v4.5.0.rst @@ -51,6 +51,13 @@ Due to a bug fix, the behavior has changed so that options passed to the outer ``group()`` are merged with the options of the inner ``group()``. See :ref:`Upgrading Guide ` for details. +API\\ResponseTrait +------------------ + +Now when a response format is JSON, if you pass string data, the framework returns +a JSON response. In previous versions, it returned a HTML response. +See :ref:`Upgrading Guide ` for details. + Factories class --------------- diff --git a/user_guide_src/source/installation/upgrade_450.rst b/user_guide_src/source/installation/upgrade_450.rst index 92eb37b80d..dfa636f567 100644 --- a/user_guide_src/source/installation/upgrade_450.rst +++ b/user_guide_src/source/installation/upgrade_450.rst @@ -155,6 +155,20 @@ reversed. Previous: route1 → route2 → filter1 → filter2 Now: route2 → route1 → filter2 → filter1 +.. _upgrade-450-api-response-trait: + +API\\ResponseTrait and String Data +================================== + +In previous versions, if you pass string data to a trait method, the framework +returned an HTML response, even if the response format was determined to be JSON. + +Now if you pass string data, it returns a JSON response correctly. See also +:ref:`api-response-trait-handling-response-types`. + +You can keep the behavior in previous versions if you set the ``$stringAsHtml`` +property to ``true`` in your controller. + FileLocator::findQualifiedNameFromPath() ======================================== diff --git a/user_guide_src/source/outgoing/api_responses.rst b/user_guide_src/source/outgoing/api_responses.rst index 6654a190da..8b5564aa1e 100644 --- a/user_guide_src/source/outgoing/api_responses.rst +++ b/user_guide_src/source/outgoing/api_responses.rst @@ -24,6 +24,8 @@ exist for the most common use cases: .. literalinclude:: api_responses/002.php +.. _api-response-trait-handling-response-types: + *********************** Handling Response Types ***********************