From 529f527611f08db3d546257f239875a9f677bb71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Tue, 10 Oct 2017 14:44:41 +0200 Subject: [PATCH 1/6] Register all arguments of `header()` on the stack In order to be able to verify that we're calling the function with the correct values. --- test/Response/AbstractEmitterTest.php | 7 +++-- test/Response/SapiStreamEmitterTest.php | 2 +- test/ServerTest.php | 29 ++++++++---------- test/TestAsset/Functions.php | 40 ++++++++++++++++++++----- test/TestAsset/SapiResponse.php | 14 +++++++-- 5 files changed, 62 insertions(+), 30 deletions(-) diff --git a/test/Response/AbstractEmitterTest.php b/test/Response/AbstractEmitterTest.php index b73b17dd..fbc3f04b 100644 --- a/test/Response/AbstractEmitterTest.php +++ b/test/Response/AbstractEmitterTest.php @@ -40,8 +40,9 @@ public function testEmitsResponseHeaders() ob_start(); $this->emitter->emit($response); ob_end_clean(); - $this->assertContains('HTTP/1.1 200 OK', HeaderStack::stack()); - $this->assertContains('Content-Type: text/plain', HeaderStack::stack()); + + $this->assertTrue(HeaderStack::has('HTTP/1.1 200 OK')); + $this->assertTrue(HeaderStack::has('Content-Type: text/plain')); } public function testEmitsMessageBody() @@ -68,7 +69,7 @@ public function testDoesNotInjectContentLengthHeaderIfStreamSizeIsUnknown() $this->emitter->emit($response); ob_end_clean(); foreach (HeaderStack::stack() as $header) { - $this->assertNotContains('Content-Length:', $header); + $this->assertNotContains('Content-Length:', $header['header']); } } } diff --git a/test/Response/SapiStreamEmitterTest.php b/test/Response/SapiStreamEmitterTest.php index 909bf511..7be4bb6d 100644 --- a/test/Response/SapiStreamEmitterTest.php +++ b/test/Response/SapiStreamEmitterTest.php @@ -56,7 +56,7 @@ public function testDoesNotInjectContentLengthHeaderIfStreamSizeIsUnknown() $this->emitter->emit($response); ob_end_clean(); foreach (HeaderStack::stack() as $header) { - $this->assertNotContains('Content-Length:', $header); + $this->assertNotContains('Content-Length:', $header['header']); } } diff --git a/test/ServerTest.php b/test/ServerTest.php index 5d10f58f..9cef932e 100644 --- a/test/ServerTest.php +++ b/test/ServerTest.php @@ -151,8 +151,8 @@ public function testListenInvokesCallbackAndSendsResponse() $this->expectOutputString('FOOBAR'); $server->listen(); - $this->assertContains('HTTP/1.1 200 OK', HeaderStack::stack()); - $this->assertContains('Content-Type: text/plain', HeaderStack::stack()); + $this->assertTrue(HeaderStack::has('HTTP/1.1 200 OK')); + $this->assertTrue(HeaderStack::has('Content-Type: text/plain')); } public function testListenEmitsStatusHeaderWithoutReasonPhraseIfNoReasonPhrase() @@ -176,8 +176,8 @@ public function testListenEmitsStatusHeaderWithoutReasonPhraseIfNoReasonPhrase() $this->expectOutputString('FOOBAR'); $server->listen(); - $this->assertContains('HTTP/1.1 299', HeaderStack::stack()); - $this->assertContains('Content-Type: text/plain', HeaderStack::stack()); + $this->assertTrue(HeaderStack::has('HTTP/1.1 299')); + $this->assertTrue(HeaderStack::has('Content-Type: text/plain')); } public function testEnsurePercentCharactersDoNotResultInOutputError() @@ -200,8 +200,8 @@ public function testEnsurePercentCharactersDoNotResultInOutputError() $this->expectOutputString('100%'); $server->listen(); - $this->assertContains('HTTP/1.1 200 OK', HeaderStack::stack()); - $this->assertContains('Content-Type: text/plain', HeaderStack::stack()); + $this->assertTrue(HeaderStack::has('HTTP/1.1 200 OK')); + $this->assertTrue(HeaderStack::has('Content-Type: text/plain')); } public function testEmitsHeadersWithMultipleValuesMultipleTimes() @@ -228,19 +228,16 @@ public function testEmitsHeadersWithMultipleValuesMultipleTimes() $server->listen(); - $this->assertContains('HTTP/1.1 200 OK', HeaderStack::stack()); - $this->assertContains('Content-Type: text/plain', HeaderStack::stack()); - $this->assertContains( - 'Set-Cookie: foo=bar; expires=Wed, 1 Oct 2014 10:30; path=/foo; domain=example.com', - HeaderStack::stack() + $this->assertTrue(HeaderStack::has('HTTP/1.1 200 OK')); + $this->assertTrue(HeaderStack::has('Content-Type: text/plain')); + $this->assertTrue( + HeaderStack::has('Set-Cookie: foo=bar; expires=Wed, 1 Oct 2014 10:30; path=/foo; domain=example.com') ); - $this->assertContains( - 'Set-Cookie: bar=baz; expires=Wed, 8 Oct 2014 10:30; path=/foo/bar; domain=example.com', - HeaderStack::stack() + $this->assertTrue( + HeaderStack::has('Set-Cookie: bar=baz; expires=Wed, 8 Oct 2014 10:30; path=/foo/bar; domain=example.com') ); - $stack = HeaderStack::stack(); - return $stack; + return HeaderStack::stack(); } /** diff --git a/test/TestAsset/Functions.php b/test/TestAsset/Functions.php index d8e2b74a..f1d0a5e0 100644 --- a/test/TestAsset/Functions.php +++ b/test/TestAsset/Functions.php @@ -27,7 +27,7 @@ class HeaderStack { /** - * @var array + * @var string[][] */ private static $data = []; @@ -42,9 +42,9 @@ public static function reset() /** * Push a header on the stack * - * @param string $header + * @param string[] $header */ - public static function push($header) + public static function push(array $header) { self::$data[] = $header; } @@ -52,12 +52,30 @@ public static function push($header) /** * Return the current header stack * - * @return array + * @return string[][] */ public static function stack() { return self::$data; } + + /** + * Verify if there's a header line on the stack + * + * @param string $header + * + * @return bool + */ + public static function has($header) + { + foreach (self::$data as $item) { + if ($item['header'] === $header) { + return true; + } + } + + return false; + } } /** @@ -73,9 +91,17 @@ function headers_sent() /** * Emit a header, without creating actual output artifacts * - * @param string $value + * @param string $string + * @param bool $replace + * @param int|null $statusCode */ -function header($value) +function header($string, $replace = true, $statusCode = null) { - HeaderStack::push($value); + HeaderStack::push( + [ + 'header' => $string, + 'replace' => $replace, + 'status_code' => $statusCode, + ] + ); } diff --git a/test/TestAsset/SapiResponse.php b/test/TestAsset/SapiResponse.php index 2586f2bc..11a937b2 100644 --- a/test/TestAsset/SapiResponse.php +++ b/test/TestAsset/SapiResponse.php @@ -35,9 +35,17 @@ function headers_sent() /** * Emit a header, without creating actual output artifacts * - * @param string $value + * @param string $string + * @param bool $replace + * @param int|null $http_response_code */ -function header($value) +function header($string, $replace = true, $http_response_code = null) { - HeaderStack::push($value); + HeaderStack::push( + [ + 'header' => $string, + 'replace' => $replace, + 'status_code' => $http_response_code, + ] + ); } From 2eac047bfe87da62d890e074537fa09dab4e2d38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Tue, 10 Oct 2017 15:04:41 +0200 Subject: [PATCH 2/6] Ensure that `Set-Cookie` header is not replaced Although we have tests covering multiple `Set-Cookie` headers, they are not verifying if the function `header()` is being called correctly. --- test/Response/AbstractEmitterTest.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/Response/AbstractEmitterTest.php b/test/Response/AbstractEmitterTest.php index fbc3f04b..3616ba09 100644 --- a/test/Response/AbstractEmitterTest.php +++ b/test/Response/AbstractEmitterTest.php @@ -56,6 +56,24 @@ public function testEmitsMessageBody() $this->emitter->emit($response); } + public function testMultipleSetCookieHeadersAreNotReplaced() + { + $response = (new Response()) + ->withStatus(200) + ->withAddedHeader('Set-Cookie', 'foo=bar') + ->withAddedHeader('Set-Cookie', 'bar=baz'); + + $this->emitter->emit($response); + + $expectedStack = [ + ['header' => 'HTTP/1.1 200 OK', 'replace' => true, 'status_code' => null], + ['header' => 'Set-Cookie: foo=bar', 'replace' => false, 'status_code' => null], + ['header' => 'Set-Cookie: bar=baz', 'replace' => false, 'status_code' => null], + ]; + + $this->assertSame($expectedStack, HeaderStack::stack()); + } + public function testDoesNotInjectContentLengthHeaderIfStreamSizeIsUnknown() { $stream = $this->prophesize('Psr\Http\Message\StreamInterface'); From fcb80a8506ac66952c2fabfe9b38a8c44b6bc0ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Tue, 10 Oct 2017 15:07:46 +0200 Subject: [PATCH 3/6] Ensure that PHP doesn't override response code If we don't pass the response status code to the function `header()` PHP might silently change the response code, which is not really nice since it should be up for the developers to define how they design the application. This silent change usually happens when one uses "Location" with a status code that's not 201 or 3xx, and some people already discussed a lot and essencially they've said that PHP will not modify its behaviour regarding this because it allows high level code to modify (fix) this. References: - https://bugs.php.net/bug.php?id=70273 - https://bugs.php.net/bug.php?id=51749 - https://bugs.php.net/bug.php?id=74535 --- src/Response/SapiEmitterTrait.php | 10 +++++++--- test/Response/AbstractEmitterTest.php | 24 +++++++++++++++++++++--- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/src/Response/SapiEmitterTrait.php b/src/Response/SapiEmitterTrait.php index 936eb852..43ec7904 100644 --- a/src/Response/SapiEmitterTrait.php +++ b/src/Response/SapiEmitterTrait.php @@ -43,12 +43,14 @@ private function assertNoPreviousOutput() private function emitStatusLine(ResponseInterface $response) { $reasonPhrase = $response->getReasonPhrase(); + $statusCode = $response->getStatusCode(); + header(sprintf( 'HTTP/%s %d%s', $response->getProtocolVersion(), - $response->getStatusCode(), + $statusCode, ($reasonPhrase ? ' ' . $reasonPhrase : '') - )); + ), true, $statusCode); } /** @@ -63,6 +65,8 @@ private function emitStatusLine(ResponseInterface $response) */ private function emitHeaders(ResponseInterface $response) { + $statusCode = $response->getStatusCode(); + foreach ($response->getHeaders() as $header => $values) { $name = $this->filterHeader($header); $first = $name === 'Set-Cookie' ? false : true; @@ -71,7 +75,7 @@ private function emitHeaders(ResponseInterface $response) '%s: %s', $name, $value - ), $first); + ), $first, $statusCode); $first = false; } } diff --git a/test/Response/AbstractEmitterTest.php b/test/Response/AbstractEmitterTest.php index 3616ba09..8e52b618 100644 --- a/test/Response/AbstractEmitterTest.php +++ b/test/Response/AbstractEmitterTest.php @@ -66,9 +66,27 @@ public function testMultipleSetCookieHeadersAreNotReplaced() $this->emitter->emit($response); $expectedStack = [ - ['header' => 'HTTP/1.1 200 OK', 'replace' => true, 'status_code' => null], - ['header' => 'Set-Cookie: foo=bar', 'replace' => false, 'status_code' => null], - ['header' => 'Set-Cookie: bar=baz', 'replace' => false, 'status_code' => null], + ['header' => 'HTTP/1.1 200 OK', 'replace' => true, 'status_code' => 200], + ['header' => 'Set-Cookie: foo=bar', 'replace' => false, 'status_code' => 200], + ['header' => 'Set-Cookie: bar=baz', 'replace' => false, 'status_code' => 200], + ]; + + $this->assertSame($expectedStack, HeaderStack::stack()); + } + + public function testDoesNotLetResponseCodeBeOverriddenByPHP() + { + $response = (new Response()) + ->withStatus(202) + ->withAddedHeader('Location', 'http://api.my-service.com/12345678') + ->withAddedHeader('Content-Type', 'text/plain'); + + $this->emitter->emit($response); + + $expectedStack = [ + ['header' => 'HTTP/1.1 202 Accepted', 'replace' => true, 'status_code' => 202], + ['header' => 'Location: http://api.my-service.com/12345678', 'replace' => true, 'status_code' => 202], + ['header' => 'Content-Type: text/plain', 'replace' => true, 'status_code' => 202], ]; $this->assertSame($expectedStack, HeaderStack::stack()); From 35f5af155f126f7145bab821675ea00ebc991bd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Thu, 12 Oct 2017 17:11:40 +0200 Subject: [PATCH 4/6] Reverse emitStatusLine() and emitHeaders() calls To ensure that the emitted response will always be correct according with the response object. --- src/Response/SapiEmitter.php | 2 +- src/Response/SapiEmitterTrait.php | 6 ++++++ src/Response/SapiStreamEmitter.php | 2 +- test/Response/AbstractEmitterTest.php | 4 ++-- test/ServerTest.php | 7 +++++++ 5 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/Response/SapiEmitter.php b/src/Response/SapiEmitter.php index 0b0a7bd6..111e83be 100644 --- a/src/Response/SapiEmitter.php +++ b/src/Response/SapiEmitter.php @@ -26,8 +26,8 @@ public function emit(ResponseInterface $response) { $this->assertNoPreviousOutput(); - $this->emitStatusLine($response); $this->emitHeaders($response); + $this->emitStatusLine($response); $this->emitBody($response); } diff --git a/src/Response/SapiEmitterTrait.php b/src/Response/SapiEmitterTrait.php index 43ec7904..3eeb3a73 100644 --- a/src/Response/SapiEmitterTrait.php +++ b/src/Response/SapiEmitterTrait.php @@ -38,7 +38,13 @@ private function assertNoPreviousOutput() * Emits the status line using the protocol version and status code from * the response; if a reason phrase is available, it, too, is emitted. * + * It's important to mention that, in order to prevent PHP from changing + * the status code of the emitted response, this method should be called + * after `emitHeaders()` + * * @param ResponseInterface $response + * + * @see \Zend\Diactoros\Response\SapiEmitterTrait::emitHeaders() */ private function emitStatusLine(ResponseInterface $response) { diff --git a/src/Response/SapiStreamEmitter.php b/src/Response/SapiStreamEmitter.php index 4175cb07..e06c99f2 100644 --- a/src/Response/SapiStreamEmitter.php +++ b/src/Response/SapiStreamEmitter.php @@ -27,8 +27,8 @@ class SapiStreamEmitter implements EmitterInterface public function emit(ResponseInterface $response, $maxBufferLength = 8192) { $this->assertNoPreviousOutput(); - $this->emitStatusLine($response); $this->emitHeaders($response); + $this->emitStatusLine($response); $range = $this->parseContentRange($response->getHeaderLine('Content-Range')); diff --git a/test/Response/AbstractEmitterTest.php b/test/Response/AbstractEmitterTest.php index 8e52b618..48bb95ee 100644 --- a/test/Response/AbstractEmitterTest.php +++ b/test/Response/AbstractEmitterTest.php @@ -66,9 +66,9 @@ public function testMultipleSetCookieHeadersAreNotReplaced() $this->emitter->emit($response); $expectedStack = [ - ['header' => 'HTTP/1.1 200 OK', 'replace' => true, 'status_code' => 200], ['header' => 'Set-Cookie: foo=bar', 'replace' => false, 'status_code' => 200], ['header' => 'Set-Cookie: bar=baz', 'replace' => false, 'status_code' => 200], + ['header' => 'HTTP/1.1 200 OK', 'replace' => true, 'status_code' => 200], ]; $this->assertSame($expectedStack, HeaderStack::stack()); @@ -84,9 +84,9 @@ public function testDoesNotLetResponseCodeBeOverriddenByPHP() $this->emitter->emit($response); $expectedStack = [ - ['header' => 'HTTP/1.1 202 Accepted', 'replace' => true, 'status_code' => 202], ['header' => 'Location: http://api.my-service.com/12345678', 'replace' => true, 'status_code' => 202], ['header' => 'Content-Type: text/plain', 'replace' => true, 'status_code' => 202], + ['header' => 'HTTP/1.1 202 Accepted', 'replace' => true, 'status_code' => 202], ]; $this->assertSame($expectedStack, HeaderStack::stack()); diff --git a/test/ServerTest.php b/test/ServerTest.php index 9cef932e..5bdac59c 100644 --- a/test/ServerTest.php +++ b/test/ServerTest.php @@ -246,16 +246,23 @@ public function testEmitsHeadersWithMultipleValuesMultipleTimes() */ public function testHeaderOrderIsHonoredWhenEmitted($stack) { + $header = array_pop($stack); + $this->assertContains('Content-Type: text/plain', $header); + $header = array_pop($stack); $this->assertContains( 'Set-Cookie: bar=baz; expires=Wed, 8 Oct 2014 10:30; path=/foo/bar; domain=example.com', $header ); + $header = array_pop($stack); $this->assertContains( 'Set-Cookie: foo=bar; expires=Wed, 1 Oct 2014 10:30; path=/foo; domain=example.com', $header ); + + $header = array_pop($stack); + $this->assertContains('HTTP/1.1 200 OK', $header); } public function testListenPassesCallableArgumentToCallback() From 841856b08eb64a567c5401461d918ed0ab9a73d6 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 12 Oct 2017 10:24:30 -0500 Subject: [PATCH 5/6] Slight re-wording of notice in emitStatusLine docblock --- src/Response/SapiEmitterTrait.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Response/SapiEmitterTrait.php b/src/Response/SapiEmitterTrait.php index 3eeb3a73..ce9612c8 100644 --- a/src/Response/SapiEmitterTrait.php +++ b/src/Response/SapiEmitterTrait.php @@ -38,9 +38,9 @@ private function assertNoPreviousOutput() * Emits the status line using the protocol version and status code from * the response; if a reason phrase is available, it, too, is emitted. * - * It's important to mention that, in order to prevent PHP from changing - * the status code of the emitted response, this method should be called - * after `emitHeaders()` + * It is important to mention that this method should be called after + * `emitHeaders()` in order to prevent PHP from changing the status code of + * the emitted response. * * @param ResponseInterface $response * From 8b2bbca6cf503cf72e258969ec08434f8abae7d5 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 12 Oct 2017 10:24:35 -0500 Subject: [PATCH 6/6] Added CHANGELOG entries for #273 --- CHANGELOG.md | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67946007..d322efec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ All notable changes to this project will be documented in this file, in reverse chronological order by release. -## 1.6.1 - TBD +## 1.6.1 - 2017-10-12 ### Added @@ -10,7 +10,10 @@ All notable changes to this project will be documented in this file, in reverse ### Changed -- Nothing. +- [#273](https://github.com/zendframework/zend-diactoros/pull/273) updates each + of the SAPI emitter implementations to emit the status line after emitting + other headers; this is done to ensure that the status line is not overridden + by PHP. ### Deprecated @@ -22,7 +25,10 @@ All notable changes to this project will be documented in this file, in reverse ### Fixed -- Nothing. +- [#273](https://github.com/zendframework/zend-diactoros/pull/273) modifies how + the `SapiEmitterTrait` calls `header()` to ensure that a response code is + _always_ passed as the third argument; this is done to prevent PHP from + silently overriding it. ## 1.6.0 - 2017-09-13