Skip to content

JsonResponse::setData with JSON_THROW_ON_ERROR can throw when there is no error #31447

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

Jump to bottom
Closed
lt opened this issue May 9, 2019 · 5 comments
Closed

JsonResponse::setData with JSON_THROW_ON_ERROR can throw when there is no error #31447

lt opened this issue May 9, 2019 · 5 comments
Labels
Bug HttpFoundation Status: Needs Review

Comments

@lt
Copy link

lt commented May 9, 2019 •
edited
Loading

Symfony version(s) affected: 4.2.5

Description

JsonResponse::setData checks json_last_error to determine whether or not encoding was successful, however when JSON_THROW_ON_ERROR is set as an encoding option, the error state is not cleared.

This means when a previous JSON operation that does not use JSON_THROW_ON_ERROR has failed, JsonResponse::setData will throw an exception even when the encode operation was successful.

How to reproduce

<?php declare(strict_types=1);

require 'vendor/autoload.php';

$response = new \Symfony\Component\HttpFoundation\JsonResponse();
$response->setEncodingOptions(JSON_THROW_ON_ERROR);

// Commenting this out prevents the exception from setData
json_decode('bad data, but carry on');

$response->setData('this is fine');

PHP Fatal error:  Uncaught InvalidArgumentException: Syntax error in /var/www/html/vendor/symfony/http-foundation/JsonResponse.php:152                                                                                                       
Stack trace:
#0 /var/www/html/test.php(12): Symfony\Component\HttpFoundation\JsonResponse->setData('"this is fine"')
#1 {main}
  thrown in /var/www/html/vendor/symfony/http-foundation/JsonResponse.php on line 152

Possible Solution

Check encoding options for JSON_THROW_ON_ERROR when checking json_last_error()

Additional context

https://bugs.php.net/bug.php?id=77997
@nicolas-grekas
Copy link
Member

Related to https://externals.io/message/105653

@joris-ati4
Copy link

I am currently looking into this issue and am not sure we can prevent this behaviour because it would be impossible to know if the user did a json_(encode / decode) before calling setData with JSON_THROW_ERROR as encodingOption.

Even if the user set the encodingOption to JSON_THROW_ERROR and we added a check before doing json_last_error(), we cannot now which json function has thrown the error. Another problem is that the try catch only works for php7.3 and we have to use json_last_error() for php7.2 and php7.1

Any suggestions on what to do with this issue ?

@JJarrie
Copy link
Contributor

JJarrie commented Jun 3, 2019

What about just fixing by internal flag's checking? Just to have explicit errors catching in the component, waiting the discussion about the behavior of the global error state be finished.

        if (!($this->encodingOptions & 4194304) && JSON_ERROR_NONE !== json_last_error()) {
            throw new \InvalidArgumentException(json_last_error_msg());
        }

@nicolas-grekas nicolas-grekas mentioned this issue Jun 4, 2019
Merged
@nicolas-grekas
Copy link
Member

Please check #31860

@fabpot fabpot closed this as completed Jun 5, 2019
fabpot added a commit that referenced this issue Jun 5, 2019
@fabpot
bug #31860 [HttpFoundation] work around PHP 7.3 bug related to json_e…
2ae0580 
…ncode() (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpFoundation] work around PHP 7.3 bug related to json_encode()

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31447
| License       | MIT
| Doc PR        | -

I know, this doesn't make any sense.
`json_encode` embeds global state behind the scene :(

For reference, I asked on php-internals what they think about this:
https://externals.io/message/105653#105838

Commits
-------

e6e6301 [HttpFoundation] work around PHP 7.3 bug related to json_encode()
@nicolas-grekas
Copy link
Member

Better fix in #31869

@phil-davis phil-davis mentioned this issue Jun 27, 2019
Merged
nicolas-grekas added a commit that referenced this issue Jun 28, 2019
@nicolas-grekas
bug #32206 Catch JsonException and rethrow in JsonEncode (phil-davis)
2bab37d 
This PR was merged into the 3.4 branch.

Discussion
----------

Catch JsonException and rethrow in JsonEncode

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | adjustment to implementation of previous PRs for issue #31447
| License       | MIT
| Doc PR        | not applicable

PR #31860 provided handling of PHP  7.3 `JSON_THROW_ON_ERROR` behavior in the various `JsonEncode` and related classes/methods.

PR #31869 adjusted that. In particular, it adjusted ` src/Symfony/Component/Serializer/Encoder/JsonDecode.php` so that it catches any `JsonException` and re-throws it as `NotEncodableValueException`. That preserves the previous behavior of `JsonDecode:decode` - it always throws `NotEncodableValueException` when something goes wrong.

IMO `JsonEncode:encode` needs the same logic. At the moment, if a caller specifies `JSON_THROW_ON_ERROR` then the method can throw `JsonException`, but actually the "standard" for `JsonEncode:encode` is that it throws `NotEncodableValueException`

Adjust `JsonEncode:encode` to catch `JsonException` and rethrow it as `NotEncodableValueException`

Commits
-------

9c76790 Catch JsonException and rethrow in JsonEncode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug HttpFoundation Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@fabpot @javiereguiluz @nicolas-grekas @lt @JJarrie @joris-ati4 @carsonbot