Bug #61537 [Com]: json_encode() incorrectly truncates/discards information

Edit report at https://bugs.php.net/bug.php?id=61537&edit=1 ID: 61537 Comment by: theanomaly dot is at gmail dot com Reported by: joey@php.net Summary: json_encode() incorrectly truncates/discards information Status: Assigned Type: Bug Package: JSON related Operating System: all PHP Version: 5.4.0 Assigned To: aharvey Block user comment: N Private report: N New Comment: The patch submitted fixes the problem. json_ecode() has been incorrectly documented all along. Instead of json_encode() returning false it has been gladly returning a string containing "null" to replace invalid data. Now, "null" is valid json, but the problem is the user never ends up knowing that json_encode() silently failed unless they explicitly check json_last_error() every single time they call json_encode(). Clearly this was not the documented behavior. It should be the case that when the user tries... <?php if (!(json_encode() = $json)) { /* Then we have valid json to work with and there is no error */ } else { /* json_encode() failed and we should check json_last_error() in order to know why */ ?> I also submitted a patch to fix the problem not realizing there was another patch already there which fixes it too and allows it throw the error. https://github.com/srgoogleguy/php- src/commit/ba181fce117a3a7d1c7668cf30744b3d7cd7cdbf However, now there is a test that's going to fail ext/json/tests/bug43941.phpt which is testing for undocumented behavior. It was in response to this older bug report: https://bugs.php.net/bug.php?id=43941 However, that fix never really fixed the problem. It just made it worse. In my opinion we should remove this test. I just wanted aharvey@php.net to also look at in case they feel the test should remain or be modified to comply with the spec. Thanks. Previous Comments: ------------------------------------------------------------------------ [2012-04-02 01:19:07] aharvey@php.net Haven't heard any screaming, so let's go with option 2. ------------------------------------------------------------------------ [2012-03-28 08:11:39] yohgaki@php.net I prefer option 2. It's an error condition anyway. ------------------------------------------------------------------------ [2012-03-28 05:12:08] aharvey@php.net I think we've got two options here: 1. Change the documented behaviour to note that invalid UTF-8 strings are encoded as NULL, rather than causing json_encode() to return false, and advise users to check json_last_error() if they're concerned about the output. 2. Change the json_encode() behaviour to match the documentation. Quick and dirty POC patch which adds a switch to fall back to the current behaviour: https://gist.github.com/7735daabc56fb38eb511 Option 2 has a BC concern: we've never advertised json_encode() as not returning false in this case, but people may rely on it. Either way, I think we should probably drop the if (PG(display_errors)) { ... } around the warning in json_escape_string(). Anyone else have thoughts? ------------------------------------------------------------------------ [2012-03-28 05:01:34] joey@php.net Per documentation: Return Values: Returns a JSON encoded string on success or FALSE on failure. ------------------------------------------------------------------------ [2012-03-28 05:01:01] joey@php.net Description: ------------ When json_encode() cannot correctly encode its argument into JSON, the expected behaviour is that it will return false, and people can check json_last_error() to see why it failed. Currently, it will replace all data it could not encode with the string 'null'. This can lead to what appears to be silently discarding data. Test script: --------------- <?php var_dump(json_encode(chr(215)), json_last_error()); Expected result: ---------------- bool(false) int(5) Actual result: -------------- string(4) 'null' int(5) ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=61537&edit=1