Skip to content

Add JSON_THROW_ON_ERROR option for json_decode/encode() #2662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed

Add JSON_THROW_ON_ERROR option for json_decode/encode() #2662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fe44c50
Add JSON_THROW_EXCEPTIONS option for json_decode/encode()
hikari-no-yume Jul 29, 2017
bf3df52
Rename JSON_THROW_EXCEPTIONS to JSON_THROW_ON_ERROR
hikari-no-yume Aug 2, 2017
242d2f8
Use !(flag) rather than 0 == (flag)
hikari-no-yume Aug 2, 2017
d1e5e98
Split exceptions test into decode and encode tests
hikari-no-yume Aug 2, 2017
c2bf7c3
Make PHP_JSON_PARTIAL_OUTPUT_ON_ERROR override exceptions
hikari-no-yume Sep 13, 2017
3b20763
Don't touch global error flag if JSON_THROW_ON_ERROR set
hikari-no-yume Sep 24, 2017
6a4d3b6
s/ /\t/g
hikari-no-yume Oct 5, 2017
64a684d
NEWS and UPGRADING
hikari-no-yume Oct 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ PHP NEWS
- GD:
. Added support for WebP in imagecreatefromstring() (Andreas Treichel, cmb).

- JSON:
. Added JSON_THROW_ON_ERROR flag. (Andrea)

- LDAP:
. Added ldap_exop_refresh helper for EXOP REFRESH operation with dds overlay.
(Come)
Expand Down
14 changes: 14 additions & 0 deletions UPGRADING
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@ BCMath:
5. Changed Functions
========================================

JSON:
. A new flag has been added, JSON_THROW_ON_ERROR, which can be used with
json_decode() or json_encode() and causes these functions to throw a
JsonException upon an error, instead of setting the global error state that
is retrieved with json_last_error(). JSON_PARTIAL_OUTPUT_ON_ERROR takes
precedence over JSON_THROW_ON_ERROR.
(RFC: https://wiki.php.net/rfc/json_throw_on_error)

========================================
6. New Functions
========================================
Expand All @@ -67,6 +75,9 @@ Date:
7. New Classes and Interfaces
========================================

JSON:
. JsonException

========================================
8. Removed Extensions and SAPIs
========================================
Expand All @@ -90,6 +101,9 @@ Date:
10. New Global Constants
========================================

JSON:
. JSON_THROW_ON_ERROR

PGSQL:
. Requires Postgres 9.3
- PGSQL_DIAG_SCHEMA_NAME
Expand Down
97 changes: 63 additions & 34 deletions ext/json/json.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ static PHP_FUNCTION(json_last_error);
static PHP_FUNCTION(json_last_error_msg);

PHP_JSON_API zend_class_entry *php_json_serializable_ce;
PHP_JSON_API zend_class_entry *php_json_exception_ce;

PHP_JSON_API ZEND_DECLARE_MODULE_GLOBALS(json)

Expand Down Expand Up @@ -95,6 +96,9 @@ static PHP_MINIT_FUNCTION(json)
INIT_CLASS_ENTRY(ce, "JsonSerializable", json_serializable_interface);
php_json_serializable_ce = zend_register_internal_interface(&ce);

INIT_CLASS_ENTRY(ce, "JsonException", NULL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't seem to comply with this newly-accepted RFC: https://wiki.php.net/rfc/class-naming

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It complies with being consistent with existing JSON-related class names, such as the one two lines above you...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO JsonParseException suites better for json_decode, because it's parsing related exception, and for json_encode it would be for eg. JsonEncodeException or JsonSerializeException

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic I know ... I don't care that much, just noting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of those scenarios where PHP's case-insensitivity is actually useful. We could change the case of the existing class with no* BC break.

*except for autoloaded polyfills

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hikari-no-yume I don't see any value in renaming that. The RFC was just for the future scope so it certainly doesn't apply for JsonSerializable. It's already documented and people are used to that name so I wouldn't change it. I think that such change needs a separate RFC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically I'm not sure if we should introduce JsonException without RFC. It really goes against the linked class-naming RFC and if it's JSONException then it goes against consistency of this extension. I think we need an RFC in any case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFC just to resolve some inconsistency that's irrelevant due to case-insensitivity? I'm sorry I brought this up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP sadness. The naming RFC just shouldn't be a thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proclaiming anarchy "shouldn't be a thing", either. The respective RFC has been accepted, and so we have to comply – whether we like it or not.

php_json_exception_ce = zend_register_internal_class_ex(&ce, zend_ce_exception);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like this to extend RuntimeException, but that made PHP complain about this not implementing Throwable for some reason and throw an Exception instead. :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further investigation reveals it's because ext/json gets initialised before ext/spl, which provides RuntimeException. This could be worked around somehow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't extend runtime exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I'm happy not to! I'd be curious to hear your reasoning, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's simply not a reason to extend another exception. It doesn't add any benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. It's not worth the effort in any case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because SPL might not be loaded is a good reason. :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably any easy workaround. I also cba.


/* options for json_encode */
PHP_JSON_REGISTER_CONSTANT("JSON_HEX_TAG", PHP_JSON_HEX_TAG);
PHP_JSON_REGISTER_CONSTANT("JSON_HEX_AMP", PHP_JSON_HEX_AMP);
Expand All @@ -116,6 +120,7 @@ static PHP_MINIT_FUNCTION(json)
/* common options for json_decode and json_encode */
PHP_JSON_REGISTER_CONSTANT("JSON_INVALID_UTF8_IGNORE", PHP_JSON_INVALID_UTF8_IGNORE);
PHP_JSON_REGISTER_CONSTANT("JSON_INVALID_UTF8_SUBSTITUTE", PHP_JSON_INVALID_UTF8_SUBSTITUTE);
PHP_JSON_REGISTER_CONSTANT("JSON_THROW_ON_ERROR", PHP_JSON_THROW_ON_ERROR);

/* json error constants */
PHP_JSON_REGISTER_CONSTANT("JSON_ERROR_NONE", PHP_JSON_ERROR_NONE);
Expand Down Expand Up @@ -207,14 +212,50 @@ PHP_JSON_API int php_json_encode(smart_str *buf, zval *val, int options) /* {{{
}
/* }}} */

static const char *php_json_get_error_msg(php_json_error_code error_code) /* {{{ */
{
switch(error_code) {
case PHP_JSON_ERROR_NONE:
return "No error";
case PHP_JSON_ERROR_DEPTH:
return "Maximum stack depth exceeded";
case PHP_JSON_ERROR_STATE_MISMATCH:
return "State mismatch (invalid or malformed JSON)";
case PHP_JSON_ERROR_CTRL_CHAR:
return "Control character error, possibly incorrectly encoded";
case PHP_JSON_ERROR_SYNTAX:
return "Syntax error";
case PHP_JSON_ERROR_UTF8:
return "Malformed UTF-8 characters, possibly incorrectly encoded";
case PHP_JSON_ERROR_RECURSION:
return "Recursion detected";
case PHP_JSON_ERROR_INF_OR_NAN:
return "Inf and NaN cannot be JSON encoded";
case PHP_JSON_ERROR_UNSUPPORTED_TYPE:
return "Type is not supported";
case PHP_JSON_ERROR_INVALID_PROPERTY_NAME:
return "The decoded property name is invalid";
case PHP_JSON_ERROR_UTF16:
return "Single unpaired UTF-16 surrogate in unicode escape";
default:
return "Unknown error";
}
}
/* }}} */

PHP_JSON_API int php_json_decode_ex(zval *return_value, char *str, size_t str_len, zend_long options, zend_long depth) /* {{{ */
{
php_json_parser parser;

php_json_parser_init(&parser, return_value, str, str_len, (int)options, (int)depth);

if (php_json_yyparse(&parser)) {
JSON_G(error_code) = php_json_parser_error_code(&parser);
php_json_error_code error_code = php_json_parser_error_code(&parser);
if (!(options & PHP_JSON_THROW_ON_ERROR)) {
JSON_G(error_code) = error_code;
} else {
zend_throw_exception(php_json_exception_ce, php_json_get_error_msg(error_code), error_code);
}
RETVAL_NULL();
return FAILURE;
}
Expand Down Expand Up @@ -243,11 +284,19 @@ static PHP_FUNCTION(json_encode)
php_json_encode_init(&encoder);
encoder.max_depth = (int)depth;
php_json_encode_zval(&buf, parameter, (int)options, &encoder);
JSON_G(error_code) = encoder.error_code;

if (encoder.error_code != PHP_JSON_ERROR_NONE && !(options & PHP_JSON_PARTIAL_OUTPUT_ON_ERROR)) {
smart_str_free(&buf);
RETURN_FALSE;
if (!(options & PHP_JSON_THROW_ON_ERROR) || (options & PHP_JSON_PARTIAL_OUTPUT_ON_ERROR)) {
JSON_G(error_code) = encoder.error_code;
if (encoder.error_code != PHP_JSON_ERROR_NONE && !(options & PHP_JSON_PARTIAL_OUTPUT_ON_ERROR)) {
smart_str_free(&buf);
RETURN_FALSE;
}
} else {
if (encoder.error_code != PHP_JSON_ERROR_NONE) {
smart_str_free(&buf);
zend_throw_exception(php_json_exception_ce, php_json_get_error_msg(encoder.error_code), encoder.error_code);
RETURN_FALSE;
}
}

smart_str_0(&buf); /* copy? */
Expand Down Expand Up @@ -277,10 +326,16 @@ static PHP_FUNCTION(json_decode)
Z_PARAM_LONG(options)
ZEND_PARSE_PARAMETERS_END();

JSON_G(error_code) = PHP_JSON_ERROR_NONE;
if (!(options & PHP_JSON_THROW_ON_ERROR)) {
JSON_G(error_code) = PHP_JSON_ERROR_NONE;
}

if (!str_len) {
JSON_G(error_code) = PHP_JSON_ERROR_SYNTAX;
if (!(options & PHP_JSON_THROW_ON_ERROR)) {
JSON_G(error_code) = PHP_JSON_ERROR_SYNTAX;
} else {
zend_throw_exception(php_json_exception_ce, php_json_get_error_msg(PHP_JSON_ERROR_SYNTAX), PHP_JSON_ERROR_SYNTAX);
}
RETURN_NULL();
}

Expand Down Expand Up @@ -327,33 +382,7 @@ static PHP_FUNCTION(json_last_error_msg)
return;
}

switch(JSON_G(error_code)) {
case PHP_JSON_ERROR_NONE:
RETURN_STRING("No error");
case PHP_JSON_ERROR_DEPTH:
RETURN_STRING("Maximum stack depth exceeded");
case PHP_JSON_ERROR_STATE_MISMATCH:
RETURN_STRING("State mismatch (invalid or malformed JSON)");
case PHP_JSON_ERROR_CTRL_CHAR:
RETURN_STRING("Control character error, possibly incorrectly encoded");
case PHP_JSON_ERROR_SYNTAX:
RETURN_STRING("Syntax error");
case PHP_JSON_ERROR_UTF8:
RETURN_STRING("Malformed UTF-8 characters, possibly incorrectly encoded");
case PHP_JSON_ERROR_RECURSION:
RETURN_STRING("Recursion detected");
case PHP_JSON_ERROR_INF_OR_NAN:
RETURN_STRING("Inf and NaN cannot be JSON encoded");
case PHP_JSON_ERROR_UNSUPPORTED_TYPE:
RETURN_STRING("Type is not supported");
case PHP_JSON_ERROR_INVALID_PROPERTY_NAME:
RETURN_STRING("The decoded property name is invalid");
case PHP_JSON_ERROR_UTF16:
RETURN_STRING("Single unpaired UTF-16 surrogate in unicode escape");
default:
RETURN_STRING("Unknown error");
}

RETURN_STRING(php_json_get_error_msg(JSON_G(error_code)));
}
/* }}} */

Expand Down
1 change: 1 addition & 0 deletions ext/json/php_json.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ typedef enum {
/* json_decode() and json_encode() common options */
#define PHP_JSON_INVALID_UTF8_IGNORE (1<<20)
#define PHP_JSON_INVALID_UTF8_SUBSTITUTE (1<<21)
#define PHP_JSON_THROW_ON_ERROR (1<<22)

/* Internal flags */
#define PHP_JSON_OUTPUT_ARRAY 0
Expand Down
50 changes: 50 additions & 0 deletions ext/json/tests/json_decode_exceptions.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
--TEST--
Test json_decode() function : JSON_THROW_ON_ERROR flag
--FILE--
<?php

try {
var_dump(json_decode("{", false, 512, JSON_THROW_ON_ERROR));
} catch (JsonException $e) {
var_dump($e);
}

?>
--EXPECTF--
object(JsonException)#1 (7) {
["message":protected]=>
string(12) "Syntax error"
["string":"Exception":private]=>
string(0) ""
["code":protected]=>
int(4)
["file":protected]=>
string(%d) "%s"
["line":protected]=>
int(%d)
["trace":"Exception":private]=>
array(1) {
[0]=>
array(4) {
["file"]=>
string(%d) "%s"
["line"]=>
int(%d)
["function"]=>
string(11) "json_decode"
["args"]=>
array(4) {
[0]=>
string(1) "{"
[1]=>
bool(false)
[2]=>
int(512)
[3]=>
int(4194304)
}
}
}
["previous":"Exception":private]=>
NULL
}
56 changes: 56 additions & 0 deletions ext/json/tests/json_encode_exceptions.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
--TEST--
Test json_encode() function : JSON_THROW_ON_ERROR flag
--FILE--
<?php

try {
var_dump(json_encode("\x80", JSON_THROW_ON_ERROR));
} catch (JsonException $e) {
var_dump($e);
}

// JSON_PARTIAL_OUTPUT_ON_ERROR is incompatible with exceptions
// So it overrides it for the sake of working with wrappers that add the
// JSON_THROW_ON_ERROR flag
var_dump(json_encode("\x80", JSON_THROW_ON_ERROR | JSON_PARTIAL_OUTPUT_ON_ERROR));
var_dump(json_last_error());
var_dump(json_last_error_msg());

?>
--EXPECTF--
object(JsonException)#1 (7) {
["message":protected]=>
string(56) "Malformed UTF-8 characters, possibly incorrectly encoded"
["string":"Exception":private]=>
string(0) ""
["code":protected]=>
int(5)
["file":protected]=>
string(%d) "%s"
["line":protected]=>
int(%d)
["trace":"Exception":private]=>
array(1) {
[0]=>
array(4) {
["file"]=>
string(%d) "%s"
["line"]=>
int(%d)
["function"]=>
string(11) "json_encode"
["args"]=>
array(2) {
[0]=>
string(1) "%s"
[1]=>
int(4194304)
}
}
}
["previous":"Exception":private]=>
NULL
}
string(4) "null"
int(5)
string(56) "Malformed UTF-8 characters, possibly incorrectly encoded"
48 changes: 48 additions & 0 deletions ext/json/tests/json_exceptions_error_clearing.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
--TEST--
JSON_THROW_ON_ERROR: global error flag untouched
--FILE--
<?php

var_dump(json_last_error());

// here we cause a different kind of error to the following errors, so that
// we can be sure the global error state looking unchanged isn't coincidence
json_decode("\xFF");

var_dump(json_last_error());

try {
json_decode("", false, 512, JSON_THROW_ON_ERROR);
} catch (JsonException $e) {
echo "Caught JSON exception: ", $e->getCode(), PHP_EOL;
}

var_dump(json_last_error());

try {
json_decode("{", false, 512, JSON_THROW_ON_ERROR);
} catch (JsonException $e) {
echo "Caught JSON exception: ", $e->getCode(), PHP_EOL;
}

var_dump(json_last_error());


try {
json_encode(NAN, JSON_THROW_ON_ERROR);
} catch (JsonException $e) {
echo "Caught JSON exception: ", $e->getCode(), PHP_EOL;
}

var_dump(json_last_error());

?>
--EXPECT--
int(0)
int(5)
Caught JSON exception: 4
int(5)
Caught JSON exception: 4
int(5)
Caught JSON exception: 7
int(5)