Skip to content

Loading…

Send charset in Content-Type when Content is JSON. #965

Merged
merged 4 commits into from

6 participants

@frederikweber

This change will send 'Content-Type: application/json; charset=UTF-8' if the Content-Type is application/json.

@lorenzo
CakePHP member

Actually the fix should be to hardcode somehow 'UTF-8' since that is the only possible value for json encoding

@frederikweber

I've updated the code that the charset=UTF-8 is just send if the Content-Type is application/json.

@pocketcrocodile

Since the default character encoding for json is utf8, the browser expects utf8 output, so we do not have to handle this by default (if we would create an option set for overwriting web-server settings, this could be useful).

@frederikweber

Some JSON parser on android need this option or they will crash. It's not false to send the charset with application/json. Almost all big APIs send the charset with the Content-Type.

@ADmad ADmad commented on an outdated diff
lib/Cake/Network/CakeResponse.php
@@ -411,6 +411,8 @@ protected function _setContentType() {
}
if (strpos($this->_contentType, 'text/') === 0) {
$this->header('Content-Type', "{$this->_contentType}; charset={$this->_charset}");
+ } else if ($this->_contentType === 'application/json') {
@ADmad CakePHP member
ADmad added a note

elseif is generally preferred over else if.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dereuromark
CakePHP member

Yeah, we talked about it yesterday, after the Stackoverflow ticket has been created.
I agree that that it would not hurt to send it - but it is also redundant for the above reasons.
Weird, that the android parser crashes. Sounds like a bug to me...
But if the others are fine adding this redundancy, why not.

@lorenzo
CakePHP member

I'm fine with the patch, it just needs unit tests so it can be merged in

@frederikweber

In which testcase would you add the test? Or should I create a new testcase? I just notice that there are no tests for getters/setters...

@dereuromark
CakePHP member

probably in CakeResponseTest - around "testSendChangingContentYype" - although this test can be removed since its a duplicate of the following "testSendChangingContentType"

@frederikweber

Is the test and the code ok?

@markstory
CakePHP member

Did you run the tests? Running them will let you know if the changes and code are ok :)

@frederikweber

Yes of course I did.

@markstory
CakePHP member

Well then things look good to me :)

@dereuromark
CakePHP member

:+1:

@markstory markstory merged commit 04d4abf into cakephp:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Showing with 7 additions and 5 deletions.
  1. +2 −0 lib/Cake/Network/CakeResponse.php
  2. +5 −5 lib/Cake/Test/Case/Network/CakeResponseTest.php
View
2 lib/Cake/Network/CakeResponse.php
@@ -411,6 +411,8 @@ protected function _setContentType() {
}
if (strpos($this->_contentType, 'text/') === 0) {
$this->header('Content-Type', "{$this->_contentType}; charset={$this->_charset}");
+ } elseif ($this->_contentType === 'application/json') {
+ $this->header('Content-Type', "{$this->_contentType}; charset=UTF-8");
} else {
$this->header('Content-Type', "{$this->_contentType}");
}
View
10 lib/Cake/Test/Case/Network/CakeResponseTest.php
@@ -199,7 +199,7 @@ public function testSend() {
* Tests the send method and changing the content type
*
*/
- public function testSendChangingContentYype() {
+ public function testSendChangingContentType() {
$response = $this->getMock('CakeResponse', array('_sendHeader', '_sendContent', '_setCookies'));
$response->type('mp3');
$response->body('the response body');
@@ -215,12 +215,12 @@ public function testSendChangingContentYype() {
}
/**
- * Tests the send method and changing the content type
+ * Tests the send method and changing the content type to JSON
*
*/
- public function testSendChangingContentType() {
+ public function testSendChangingContentTypeJSON() {
$response = $this->getMock('CakeResponse', array('_sendHeader', '_sendContent', '_setCookies'));
- $response->type('mp3');
+ $response->type('json');
$response->body('the response body');
$response->expects($this->once())->method('_sendContent')->with('the response body');
$response->expects($this->at(0))->method('_setCookies');
@@ -229,7 +229,7 @@ public function testSendChangingContentType() {
$response->expects($this->at(2))
->method('_sendHeader')->with('Content-Length', 17);
$response->expects($this->at(3))
- ->method('_sendHeader')->with('Content-Type', 'audio/mpeg');
+ ->method('_sendHeader')->with('Content-Type', 'application/json; charset=UTF-8');
$response->send();
}
Something went wrong with that request. Please try again.