Skip to content
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.

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

Catch JsonException and rethrow in JsonEncode #32206

Merged
merged 1 commit into from Jun 28, 2019
Merged

Catch JsonException and rethrow in JsonEncode #32206

merged 1 commit into from Jun 28, 2019

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Jun 27, 2019

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

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jun 28, 2019
@nicolas-grekas
Copy link
Member

Thank you @phil-davis.

@nicolas-grekas nicolas-grekas merged commit 9c76790 into symfony:3.4 Jun 28, 2019
nicolas-grekas added a commit that referenced this pull request Jun 28, 2019
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
@phil-davis phil-davis deleted the catch-JsonException-in-JsonEncode-3.4 branch June 28, 2019 12:18
This was referenced Jul 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants