Skip to content

[5.4] JsonResponse unsupported type error handling if flag set - #18917

Merged
taylorotwell merged 1 commit into
laravel:5.4from
0xb4lint:5.4
Apr 26, 2017
Merged

[5.4] JsonResponse unsupported type error handling if flag set#18917
taylorotwell merged 1 commit into
laravel:5.4from
0xb4lint:5.4

Conversation

@0xb4lint

Copy link
Copy Markdown
Contributor

Currently, Illuminate\Http\JsonResponse does not support JSON_PARTIAL_OUTPUT_ON_ERROR flag check on json_encode() error.

If the JsonResponse constructor $data argument contains a resource (maybe deep inside in an error stack - $exception->getTrace()) there is an InvalidArgumentException thrown (because of only the json_last_error() is checked).

According to PHP Docs there is a flag (JSON_PARTIAL_OUTPUT_ON_ERROR) for generating NULL values on json_encode() error - such as resource encoding.

JSON_ERROR_UNSUPPORTED_TYPE (integer)
A value of an unsupported type was given to json_encode(), such as a resource. If the JSON_PARTIAL_OUTPUT_ON_ERROR option was given, NULL will be encoded in the place of the unsupported value. Available since PHP 5.5.0.

So the expected behavior is no InvalidArgumentException is thrown if the value of json_last_error() is JSON_ERROR_UNSUPPORTED_TYPE and the JSON_PARTIAL_OUTPUT_ON_ERROR flag is set (because the generated JSON is valid).

Comment thread src/Illuminate/Http/JsonResponse.php Outdated
*/
protected function hasJsonError($jsonLastError)
{
return $jsonLastError !== JSON_ERROR_NONE && ($jsonLastError !== JSON_ERROR_UNSUPPORTED_TYPE || ! ($this->encodingOptions & JSON_PARTIAL_OUTPUT_ON_ERROR));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line is pretty hard to read. Is it possible to break it down into something simpler?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@taylorotwell you're absolutely right, please check it ;)

+ handling JSON_PARTIAL_OUTPUT_ON_ERROR flag (e.g. w/ resource)
@taylorotwell

Copy link
Copy Markdown
Member

I'm sorry this conditional is still basically unreadable. I think it needs some of the not conditions reversed and maybe the whole method renamed.

@taylorotwell
taylorotwell merged commit 908a2fe into laravel:5.4 Apr 26, 2017
@taylorotwell

Copy link
Copy Markdown
Member

I reversed the whole conditional and renamed the method. Much easier to understand. Thanks.

@0xb4lint

Copy link
Copy Markdown
Contributor Author

Thank you @taylorotwell! ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants