json_encode() / json_decode() warnings

6 years ago by Craig Duncan — view source

unread

Hi internals,

When using json_encode() and json_decode() it is required that you
manually check for errors after every call, eg:

$data = json_decode("false");
if (json_last_error() !== JSON_ERROR_NONE) {
    throw new UnexpectedValueException(json_last_error_msg());
}

This isn't that unusual in PHP, however normally in these situations a
warning would be raised. But the JSON functions only raise warnings in a
couple of scenarios, most issues are completely silent.

I wanted to begin a discussion around changing this, so that warnings are
raised for any issues during json_encode() and json_decode().

I have an implementation ready and I'm happy to draft an RFC if this
suggestion doesn't receive universal hatred.

Thanks for your time,
Craig

6 years ago by Niklas Keller — view source

unread

2017-07-27 17:41 GMT+02:00 Craig Duncan php@duncanc.co.uk:

Hi internals,

When using json_encode() and json_decode() it is required that you
manually check for errors after every call, eg:
$data = json_decode("false");
if (json_last_error() !== JSON_ERROR_NONE) {
    throw new UnexpectedValueException(json_last_error_msg());
}
This isn't that unusual in PHP, however normally in these situations a
warning would be raised. But the JSON functions only raise warnings in a
couple of scenarios, most issues are completely silent.

I wanted to begin a discussion around changing this, so that warnings are
raised for any issues during json_encode() and json_decode().

I have an implementation ready and I'm happy to draft an RFC if this
suggestion doesn't receive universal hatred.

It should rather just throw exceptions. Warnings do not really allow error
handling, they just allow error reporting.

Regards, Niklas

6 years ago by Craig Duncan — view source

unread

It should rather just throw exceptions. Warnings do not really allow error
handling, they just allow error reporting.

Agreed, but I can't see Exceptions passing the vote. Warnings can be
silenced by those that don't care and converted to Exceptions by those that
do

6 years ago by Giovanni Giacobbi — view source

unread

It should rather just throw exceptions. Warnings do not really allow
error
handling, they just allow error reporting.

Agreed, but I can't see Exceptions passing the vote. Warnings can be
silenced by those that don't care and converted to Exceptions by those that
do

Error management is a painful topic in PHP, expecially when considering to
the way fopen() behaves, where you need to use the "@" suppression if you
want it to behave the way you expect it to behave.

About Exceptions you can easily build a framework around core functions,
for example this is in my core library for all projects:

function safe_json_decode($json = null) {
  if ($json == "")
    return null;

  $retval = json_decode($json, true, 512, JSON_BIGINT_AS_STRING);
  if (json_last_error() != JSON_ERROR_NONE)
    throw new JsonDecodeException(json_last_error_msg(), `json_last_error()`);

  return $retval;
}

So yes, the behaviour of json_decode() might not be optimal, but it's fine
the way it is.

--
Giovanni Giacobbi

6 years ago by Niklas Keller — view source

unread

2017-07-28 8:56 GMT+02:00 Giovanni Giacobbi giovanni@giacobbi.net:

It should rather just throw exceptions. Warnings do not really allow
error
handling, they just allow error reporting.

Agreed, but I can't see Exceptions passing the vote. Warnings can be
silenced by those that don't care and converted to Exceptions by those
that
do

Error management is a painful topic in PHP, expecially when considering to
the way fopen() behaves, where you need to use the "@" suppression if
you want it to behave the way you expect it to behave.

About Exceptions you can easily build a framework around core functions,
for example this is in my core library for all projects:
function safe_json_decode($json = null) {
  if ($json == "")
    return null;

  $retval = json_decode($json, true, 512, JSON_BIGINT_AS_STRING);
  if (json_last_error() != JSON_ERROR_NONE)
    throw new JsonDecodeException(json_last_error_msg(),
`json_last_error()`);

  return $retval;
}
So yes, the behaviour of json_decode() might not be optimal, but it's fine
the way it is.

Yes, I know. There's https://github.com/DaveRandom/ExceptionalJSON.

While the current API works, I'm not sure whether I'd say its fine.

Regards, Niklas

6 years ago by Ryan Jentzsch — view source

unread

I can't count how many times I've been bitten by this. From the infamous
fractal blog:

"Parts of PHP are practically designed to produce buggy code.
json_decode returns null for invalid input, even though null is also a
perfectly valid object for JSON to decode to—this function is completely
unreliable unless you also call json_last_error every time you use it."

Most functions in PHP return false as an indicator for an invalid call.
json_decode() returns null -- changing this to return false is also a
breaking change that may not survive a vote.

Because of the unreliability I don't use this function and always rely on
3rd party JSON libraries instead.

2017-07-28 8:56 GMT+02:00 Giovanni Giacobbi giovanni@giacobbi.net:
It should rather just throw exceptions. Warnings do not really allow
error
handling, they just allow error reporting.

Agreed, but I can't see Exceptions passing the vote. Warnings can be
silenced by those that don't care and converted to Exceptions by those
that
do

Error management is a painful topic in PHP, expecially when considering
to
the way fopen() behaves, where you need to use the "@" suppression if
you want it to behave the way you expect it to behave.

About Exceptions you can easily build a framework around core functions,
for example this is in my core library for all projects:
function safe_json_decode($json = null) {
  if ($json == "")
    return null;

  $retval = json_decode($json, true, 512, JSON_BIGINT_AS_STRING);
  if (json_last_error() != JSON_ERROR_NONE)
    throw new JsonDecodeException(json_last_error_msg(),
`json_last_error()`);

  return $retval;
}
So yes, the behaviour of json_decode() might not be optimal, but it's
fine
the way it is.
Yes, I know. There's https://github.com/DaveRandom/ExceptionalJSON.

While the current API works, I'm not sure whether I'd say its fine.

Regards, Niklas

6 years ago by Thomas Hruska — view source

unread

I can't count how many times I've been bitten by this. From the infamous
fractal blog:

"Parts of PHP are practically designed to produce buggy code.
json_decode returns null for invalid input, even though null is also a
perfectly valid object for JSON to decode to—this function is completely
unreliable unless you also call json_last_error every time you use it."

Most functions in PHP return false as an indicator for an invalid call.
json_decode() returns null -- changing this to return false is also a
breaking change that may not survive a vote.

'false' is also valid JSON.

--
Thomas Hruska
CubicleSoft President

I've got great, time saving software that you will find useful.

http://cubiclesoft.com/

And once you find my software useful:

http://cubiclesoft.com/donate/

6 years ago by Andrea Faulds — view source

unread

Hi Duncan,

Craig Duncan wrote:

It should rather just throw exceptions. Warnings do not really allow error
handling, they just allow error reporting.

Agreed, but I can't see Exceptions passing the vote. Warnings can be
silenced by those that don't care and converted to Exceptions by those that
do

Could we not simply make it a flag? e.g.

 $bar = json_encode($foo, JSON_THROW_EXCEPTIONS);
 $baz = json_decode($bar, false, 512, JSON_THROW_EXCEPTIONS);

That wouldn't break backwards-compatibility, but would still provide the
desired functionality. :)

--
Andrea Faulds
https://ajf.me/

6 years ago by Andrea Faulds — view source

unread

Hi everyone,

Andrea Faulds wrote:

Craig Duncan wrote:

It should rather just throw exceptions. Warnings do not really allow
error
handling, they just allow error reporting.

Agreed, but I can't see Exceptions passing the vote. Warnings can be
silenced by those that don't care and converted to Exceptions by those
that
do

Could we not simply make it a flag? e.g.
$bar = json_encode($foo, JSON_THROW_EXCEPTIONS);
$baz = json_decode($bar, false, 512, JSON_THROW_EXCEPTIONS);
That wouldn't break backwards-compatibility, but would still provide the
desired functionality. :)

I wrote a patch to add this flag: https://github.com/php/php-src/pull/2662

--
Andrea Faulds
https://ajf.me/

6 years ago by Craig Duncan — view source

unread

Could we not simply make it a flag? e.g.
$bar = json_encode($foo, JSON_THROW_EXCEPTIONS);
$baz = json_decode($bar, false, 512, JSON_THROW_EXCEPTIONS);
That wouldn't break backwards-compatibility, but would still provide the
desired functionality. :)

Hi Andrea, although that wouldn't break compatibility, it doesn't protect
new developers from using them dangerously.
That desired functionality is available in many userland libraries, I don't
think we gain much from adding it to core.
My aim is to make the core functions easier/safer to use out of the box.

6 years ago by Andrea Faulds — view source

unread

Hi Craig,

Craig Duncan wrote:

Could we not simply make it a flag? e.g.
$bar = json_encode($foo, JSON_THROW_EXCEPTIONS);
$baz = json_decode($bar, false, 512, JSON_THROW_EXCEPTIONS);
That wouldn't break backwards-compatibility, but would still provide the
desired functionality. :)
Hi Andrea, although that wouldn't break compatibility, it doesn't protect
new developers from using them dangerously.
That desired functionality is available in many userland libraries, I don't
think we gain much from adding it to core.
My aim is to make the core functions easier/safer to use out of the box.

That's true, but if we add it to core we can save people reimplementing
it themselves or adding an extra dependency, and perhaps more
pertinently, it could be the first step to making this the default
behaviour.

--
Andrea Faulds
https://ajf.me/

6 years ago by Niklas Keller — view source

unread

Andrea Faulds ajf@ajf.me schrieb am Sa., 29. Juli 2017, 18:55:

Hi Craig,

Craig Duncan wrote:
Could we not simply make it a flag? e.g.
$bar = json_encode($foo, JSON_THROW_EXCEPTIONS);
$baz = json_decode($bar, false, 512, JSON_THROW_EXCEPTIONS);
That wouldn't break backwards-compatibility, but would still provide the
desired functionality. :)
Hi Andrea, although that wouldn't break compatibility, it doesn't protect
new developers from using them dangerously.
That desired functionality is available in many userland libraries, I
don't
think we gain much from adding it to core.
My aim is to make the core functions easier/safer to use out of the box.
That's true, but if we add it to core we can save people reimplementing
it themselves or adding an extra dependency, and perhaps more
pertinently, it could be the first step to making this the default
behaviour.

Thanks for that very good idea.

@Sara: Can we please get that into 7.2?

Regards, Niklas

6 years ago by Jakub Zelenka — view source

unread

Andrea Faulds ajf@ajf.me schrieb am Sa., 29. Juli 2017, 18:55:
Hi Craig,

Craig Duncan wrote:
Could we not simply make it a flag? e.g.
$bar = json_encode($foo, JSON_THROW_EXCEPTIONS);
$baz = json_decode($bar, false, 512, JSON_THROW_EXCEPTIONS);
That wouldn't break backwards-compatibility, but would still provide
the
desired functionality. :)
Hi Andrea, although that wouldn't break compatibility, it doesn't
protect
new developers from using them dangerously.
That desired functionality is available in many userland libraries, I
don't
think we gain much from adding it to core.
My aim is to make the core functions easier/safer to use out of the
box.
That's true, but if we add it to core we can save people reimplementing
it themselves or adding an extra dependency, and perhaps more
pertinently, it could be the first step to making this the default
behaviour.
Thanks for that very good idea.

@Sara: Can we please get that into 7.2?

I agree that it might be a useful feature for some users but I don't see
any need to break a release rules for that (I mean adding new features in
beta stage). Also the PR needs to have a full agreement which is not the
case atm. (still some open questions) so I wouldn't definitely rush with
adding that to 7.2. This should go just to master if all parts are agreed
IMHO. That's of course up to RM and this is just my opinion. :)

6 years ago by Andrea Faulds — view source

unread

Jakub Zelenka wrote:

Andrea Faulds ajf@ajf.me schrieb am Sa., 29. Juli 2017, 18:55:
Hi Craig,

Craig Duncan wrote:
Could we not simply make it a flag? e.g.
$bar = json_encode($foo, JSON_THROW_EXCEPTIONS);
$baz = json_decode($bar, false, 512, JSON_THROW_EXCEPTIONS);
That wouldn't break backwards-compatibility, but would still provide
the
desired functionality. :)
Hi Andrea, although that wouldn't break compatibility, it doesn't
protect
new developers from using them dangerously.
That desired functionality is available in many userland libraries, I
don't
think we gain much from adding it to core.
My aim is to make the core functions easier/safer to use out of the
box.
That's true, but if we add it to core we can save people reimplementing
it themselves or adding an extra dependency, and perhaps more
pertinently, it could be the first step to making this the default
behaviour.
Thanks for that very good idea.

@Sara: Can we please get that into 7.2?
I agree that it might be a useful feature for some users but I don't see
any need to break a release rules for that (I mean adding new features in
beta stage). Also the PR needs to have a full agreement which is not the
case atm. (still some open questions) so I wouldn't definitely rush with
adding that to 7.2. This should go just to master if all parts are agreed
IMHO. That's of course up to RM and this is just my opinion. :)

There's no significant open questions, it's all bikeshedding over tiny
details.

--
Andrea Faulds
https://ajf.me/