Fix withStatus() overwriting content-type

The withStatus() method would use the `_contentType` property which can
contain stale information if `withHeader()` is used to set the
content-type. Instead of storing potentially divergent information we
can make the content-type header authoritative and do type munging in
getType().

While this could be a breaking change we make precious few promises
around protected properties. This change should be backported to 3.x as
well to resolve the same problem there.

Backport fixes from #14211 to 3.x

src/Http/Response.php

@@ -364,14 +364,6 @@ class Response implements ResponseInterface
     protected $_status = 200;
 
     /**
-     * Content type to send. This can be an 'extension' that will be transformed using the $_mimetypes array
-     * or a complete mime-type
-     *
-     * @var string
-     */
-    protected $_contentType = 'text/html';
-
-    /**
      * File object for file to be read out as response
      *
      * @var \Cake\Filesystem\File|null
@@ -468,10 +460,11 @@ class Response implements ResponseInterface
             $options['charset'] = Configure::read('App.encoding');
         }
         $this->_charset = $options['charset'];
+        $type = 'text/html';
         if (isset($options['type'])) {
-            $this->_contentType = $this->resolveType($options['type']);
+            $type = $this->resolveType($options['type']);
         }
-        $this->_setContentType();
+        $this->_setContentType($type);
         $this->_cookies = new CookieCollection();
     }
 
@@ -538,7 +531,6 @@ class Response implements ResponseInterface
         $codeMessage = $this->_statusCodes[$this->_status];
         $this->_setCookies();
         $this->_sendHeader("{$this->_protocol} {$this->_status} {$codeMessage}");
-        $this->_setContentType();
 
         foreach ($this->headers as $header => $values) {
             foreach ((array)$values as $value) {
@@ -578,9 +570,10 @@ class Response implements ResponseInterface
      * Formats the Content-Type header based on the configured contentType and charset
      * the charset will only be set in the header if the response is of type text/*
      *
+     * @param string $type The type to set.
      * @return void
      */
-    protected function _setContentType()
+    protected function _setContentType($type)
     {
         if (in_array($this->_status, [304, 204])) {
             $this->_clearHeader('Content-Type');
@@ -594,15 +587,18 @@ class Response implements ResponseInterface
         $charset = false;
         if (
             $this->_charset &&
-            (strpos($this->_contentType, 'text/') === 0 || in_array($this->_contentType, $whitelist))
+            (
+                strpos($type, 'text/') === 0 ||
+                in_array($type, $whitelist, true)
+            )
         ) {
             $charset = true;
         }
 
-        if ($charset) {
-            $this->_setHeader('Content-Type', "{$this->_contentType}; charset={$this->_charset}");
+        if ($charset && strpos($type, ';') === false) {
+            $this->_setHeader('Content-Type', "{$type}; charset={$this->_charset}");
         } else {
-            $this->_setHeader('Content-Type', (string)$this->_contentType);
+            $this->_setHeader('Content-Type', $type);
         }
     }
 
@@ -1008,7 +1004,11 @@ class Response implements ResponseInterface
             $reasonPhrase = $this->_statusCodes[$code];
         }
         $this->_reasonPhrase = $reasonPhrase;
-        $this->_setContentType();
+
+        // These status codes don't have bodies and can't have content-types.
+        if (in_array($code, [304, 204], true)) {
+            $this->_clearHeader('Content-Type');
+        }
     }
 
     /**
@@ -1146,8 +1146,7 @@ class Response implements ResponseInterface
         if (strpos($contentType, '/') === false) {
             return false;
         }
-        $this->_contentType = $contentType;
-        $this->_setContentType();
+        $this->_setContentType($contentType);
 
         return $contentType;
     }
@@ -1175,7 +1174,12 @@ class Response implements ResponseInterface
      */
     public function getType()
     {
-        return $this->_contentType;
+        $header = $this->getHeaderLine('Content-Type');
+        if (strpos($header, ';') !== false) {
+            return explode(';', $header)[0];
+        }
+
+        return $header;
     }
 
     /**
@@ -1191,8 +1195,7 @@ class Response implements ResponseInterface
     {
         $mappedType = $this->resolveType($contentType);
         $new = clone $this;
-        $new->_contentType = $mappedType;
-        $new->_setContentType();
+        $new->_setContentType($mappedType);
 
         return $new;
     }
@@ -1276,7 +1279,7 @@ class Response implements ResponseInterface
             return $this->_charset;
         }
         $this->_charset = $charset;
-        $this->_setContentType();
+        $this->_setContentType($this->getType());
 
         return $this->_charset;
     }
@@ -1301,7 +1304,7 @@ class Response implements ResponseInterface
     {
         $new = clone $this;
         $new->_charset = $charset;
-        $new->_setContentType();
+        $new->_setContentType($this->getType());
 
         return $new;
     }
@@ -2870,7 +2873,7 @@ class Response implements ResponseInterface
     {
         return [
             'status' => $this->_status,
-            'contentType' => $this->_contentType,
+            'contentType' => $this->getType(),
             'headers' => $this->headers,
             'file' => $this->_file,
             'fileRange' => $this->_fileRange,

tests/TestCase/Http/ResponseTest.php

@@ -314,10 +314,10 @@ class ResponseTest extends TestCase
             'Original object should not be modified'
         );
         $this->assertSame('application/pdf', $new->getHeaderLine('Content-Type'));
-        $this->assertSame(
-            'application/json',
-            $new->withType('json')->getHeaderLine('Content-Type')
-        );
+
+        $json = $new->withType('json');
+        $this->assertSame('application/json', $json->getHeaderLine('Content-Type'));
+        $this->assertSame('application/json', $json->getType());
     }
 
     /**
@@ -338,6 +338,11 @@ class ResponseTest extends TestCase
             $response->withType('custom/stuff')->getHeaderLine('Content-Type'),
             'Should allow arbitrary types'
         );
+        $this->assertEquals(
+            'text/html; charset=UTF-8',
+            $response->withType('text/html; charset=UTF-8')->getHeaderLine('Content-Type'),
+            'Should allow charset types'
+        );
     }
 
     /**
@@ -556,12 +561,45 @@ class ResponseTest extends TestCase
         $new = $response->withType('pdf')
             ->withStatus(204);
         $this->assertFalse($new->hasHeader('Content-Type'));
-        $this->assertSame(204, $new->getStatusCode());
+        $this->assertSame('', $new->getType());
+        $this->assertSame(204, $new->getStatusCode(), 'Status code should clear content-type');
 
         $response = new Response();
         $new = $response->withStatus(304)
             ->withType('pdf');
-        $this->assertFalse($new->hasHeader('Content-Type'));
+        $this->assertSame('', $new->getType());
+        $this->assertFalse(
+            $new->hasHeader('Content-Type'),
+            'Type should not be retained because of status code.'
+        );
+
+        $response = new Response();
+        $new = $response
+            ->withHeader('Content-Type', 'application/json')
+            ->withStatus(204);
+        $this->assertFalse($new->hasHeader('Content-Type'), 'Should clear direct header');
+        $this->assertSame('', $new->getType());
+    }
+
+    /**
+     * Test that setting status codes doesn't overwrite content-type
+     *
+     * @return void
+     */
+    public function testWithStatusDoesNotChangeContentType()
+    {
+        $response = new Response();
+        $new = $response->withHeader('Content-Type', 'application/json')
+            ->withStatus(403);
+        $this->assertSame('application/json', $new->getHeaderLine('Content-Type'));
+        $this->assertSame(403, $new->getStatusCode());
+
+        $response = new Response();
+        $new = $response->withStatus(403)
+            ->withHeader('Content-Type', 'application/json');
+        $this->assertSame('application/json', $new->getHeaderLine('Content-Type'));
+        $this->assertSame(403, $new->getStatusCode());
+        $this->assertSame('application/json', $new->getType());
     }
 
     /**