-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Handle JSON errors when decoding #68
Conversation
if (!\is_array($content)) { | ||
throw new \RuntimeException('Invalid JSON: '.json_last_error_msg()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!\is_array($content)) { | |
throw new \RuntimeException('Invalid JSON: '.json_last_error_msg()); | |
} | |
if (JSON_ERROR_NONE !== json_last_error()) { | |
throw new \JsonException(json_last_error_msg()); | |
} | |
if (!\is_array($content)) { | |
throw new \InvalidArgumentException('Invalid JSON data: expected array, got "%s"', gettype($content))); | |
} |
How about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsonException
is only available from PHP 7.4 😉
and with 7.4 we could just tell JSON to throw that exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we could, but why would we need different exceptions at all? 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using the polyfill already since #63
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the PR to work similar to the fix from #63.
Are you OK with these changes?
👍 |
not sure if we should handle that exception …