Reset JSON_G(error_code) after calls with JSON_THROW_ON_ERROR

[m3m0r7]

Description

The following code:

<?php
json_decode("\00invalid json");
var_dump(json_last_error());

json_decode("[]", true, 512, JSON_THROW_ON_ERROR);
var_dump(json_last_error());

Resulted in this output:

int(3)
int(3)

But I expected this output instead:

int(3)
int(0)

---

In json.c file, the function calls JSON_G(error_code) = error_code when I turn off flags of JSON_THROW_ON_ERROR.
but when I turn on flags of JSON_THROW_ON_ERROR, and then JSON_G(error_code) = error_code is not called probably.

PHP_JSON_API zend_result php_json_decode_ex(zval *return_value, const 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)) {
		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; // <--- here
		} else {
			zend_throw_exception(php_json_exception_ce, php_json_get_error_msg(error_code), error_code);
		}
		RETVAL_NULL();
		return FAILURE;
	}

	return SUCCESS;
}

PHP Version

PHP 8.1

Operating System

mac

[nielsdos]

I think the current behaviour is correct and not a bug
From https://www.php.net/manual/en/function.json-last-error.php :

"Returns the last error (if any) occurred during the last JSON encoding/decoding, which did not specify JSON_THROW_ON_ERROR."

So it looks at the last one which did not use that particular flag.

[cmb69]

Closing per @nielsdos' comment above.

[m3m0r7]

@nielsdos @cmb69

Thanks for replying!

The json_decode/encode is encapsulated by third-party libraries on PHP's eco-systems; for example, guzzlehttp and its dependent libraries of it.
therefore, it has more side effects to a main application when calling a library.
for example, when I use stripe SDK and thecodingmachine\safe\json_decode, there are throwing exceptions forever.
I'm thinking that will get a bug to a main application when remained this feature on many libraries probably.

[cmb69]

Okay, changing to feature request. What do you think about resetting JSON_G(error_code) after calls with JSON_THROW_ON_ERROR, @bukka?

[bukka]

So this is basically duplicate of https://bugs.php.net/bug.php?id=77997 but we prefer GH issues now so I will leave both issues open. You can also read some discussion about this in https://externals.io/message/105653 and also in the original PR: #2662 . TLDR is that both approaches have their pros and cons and we are not going to change the default behaviour - at least not likely as it would be a BC break. We could however introduce a special flag for those that want to also set json_last_error. That's probably the only thing we can do about this.

[m3m0r7]

@bukka
Thanks for replying. I have an idea that makes a function json_reset_last_error. What do you think about this one?
I cannot find it in the discussion. Of course, implementing a special flag that is okay.

In any case, I can reset JSON_G(error_code) in the userland - If it is possible, I'm okay.

[KapitanOczywisty]

Current behavior is weird and inconsistent with other extensions e.g. mysqli will set error property regardless if error is thrown.

@m3m0r7 In worst case you can call json_encode(0); to reset last error.

[m3m0r7]

@KapitanOczywisty
Yes, I know how to reset the last error but it is a very strange solution.
the json_encode/decode is calling JSON_G(error_code) = error_code in the internal (json_validate too).

[marmichalski]

This really is quite strange behavior that we've just stumbled upon, too. It was exposed to us by having a mix of vendor code running json_decode on invalid string and not caring about the error, and our code later using thecodingmachine/safe in combination with JSON_THROW_ON_ERROR flag (kinda unnecessary tbh), which has led to thecodingmachine/safe#451

[...] TLDR is that both approaches have their pros and cons

I would be interested in what is/are the pros of current behavior?

[nielsdos]

@marmichalski I think you're gonna have more luck asking on the mailing list as not a lot of people check the issue tracker actively in comparison to the mailing list. I guess the main compliant here would be BC behaviour.

[cmb69]

Tentatively marking as needs RFC.

[github-actions]

There has not been any recent activity in this feature request. It will automatically be closed in 14 days if no further action is taken. Please see https://github.com/probot/stale#is-closing-stale-issues-really-a-good-idea to understand why we auto-close stale feature requests.