Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] json_encode() / json_decode() warnings

Posted by Craig Duncan 
Craig Duncan
[PHP-DEV] json_encode() / json_decode() warnings
July 27, 2017 05:50PM
Hi internals,

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

```php
$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
Niklas Keller
Re: [PHP-DEV] json_encode() / json_decode() warnings
July 27, 2017 06:00PM
2017-07-27 17:41 GMT+02:00 Craig Duncan <[email protected]>:

> Hi internals,
>
> When using `json_encode()` and `json_decode()` it is required that you
> manually check for errors after every call, eg:
>
> ```php
> $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
Craig Duncan
Re: [PHP-DEV] json_encode() / json_decode() warnings
July 27, 2017 06:10PM
On 27 July 2017 at 16:57, Niklas Keller <[email protected]> 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
Giovanni Giacobbi
Re: [PHP-DEV] json_encode() / json_decode() warnings
July 28, 2017 09:10AM
On 27 July 2017 at 18:00, Craig Duncan <[email protected]> wrote:

> On 27 July 2017 at 16:57, Niklas Keller <[email protected]> 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
>

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:

```php
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
Niklas Keller
Re: [PHP-DEV] json_encode() / json_decode() warnings
July 28, 2017 09:10AM
2017-07-28 8:56 GMT+02:00 Giovanni Giacobbi <[email protected]>:

> On 27 July 2017 at 18:00, Craig Duncan <[email protected]> wrote:
>
>> On 27 July 2017 at 16:57, Niklas Keller <[email protected]> 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
>>
>
> 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:
>
> ```php
> 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
Ryan Jentzsch
Re: [PHP-DEV] json_encode() / json_decode() warnings
July 28, 2017 12:10PM
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.

On Fri, Jul 28, 2017 at 12:59 AM, Niklas Keller <[email protected]> wrote:

> 2017-07-28 8:56 GMT+02:00 Giovanni Giacobbi <[email protected]>:
>
> > On 27 July 2017 at 18:00, Craig Duncan <[email protected]> wrote:
> >
> >> On 27 July 2017 at 16:57, Niklas Keller <[email protected]> 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
> >>
> >
> > 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:
> >
> > ```php
> > 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
>
Thomas Hruska
Re: [PHP-DEV] json_encode() / json_decode() warnings
July 28, 2017 03:40PM
On 7/28/2017 3:03 AM, Ryan Jentzsch wrote:
> 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/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Andrea Faulds
Re: [PHP-DEV] json_encode() / json_decode() warnings
July 29, 2017 04:20PM
Hi Duncan,

Craig Duncan wrote:
> On 27 July 2017 at 16:57, Niklas Keller <[email protected]> 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/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Andrea Faulds
Re: [PHP-DEV] json_encode() / json_decode() warnings
July 29, 2017 06:00PM
Hi everyone,

Andrea Faulds wrote:
> Craig Duncan wrote:
>> On 27 July 2017 at 16:57, Niklas Keller <[email protected]> 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/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Craig Duncan
Re: [PHP-DEV] json_encode() / json_decode() warnings
July 29, 2017 06:20PM
On 29 July 2017 at 15:16, Andrea Faulds <[email protected]> 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.
Andrea Faulds
Re: [PHP-DEV] json_encode() / json_decode() warnings
July 29, 2017 07:00PM
Hi Craig,

Craig Duncan wrote:
> On 29 July 2017 at 15:16, Andrea Faulds <[email protected]> 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/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Niklas Keller
Re: [PHP-DEV] json_encode() / json_decode() warnings
July 29, 2017 08:20PM
Andrea Faulds <[email protected]> schrieb am Sa., 29. Juli 2017, 18:55:

> Hi Craig,
>
> Craig Duncan wrote:
> > On 29 July 2017 at 15:16, Andrea Faulds <[email protected]> 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

>
Jakub Zelenka
Re: [PHP-DEV] json_encode() / json_decode() warnings
July 30, 2017 09:10PM
On Sat, Jul 29, 2017 at 7:10 PM, Niklas Keller <[email protected]> wrote:

> Andrea Faulds <[email protected]> schrieb am Sa., 29. Juli 2017, 18:55:
>
> > Hi Craig,
> >
> > Craig Duncan wrote:
> > > On 29 July 2017 at 15:16, Andrea Faulds <[email protected]> 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. :)
Andrea Faulds
Re: [PHP-DEV] json_encode() / json_decode() warnings
August 01, 2017 05:30AM
Jakub Zelenka wrote:
> On Sat, Jul 29, 2017 at 7:10 PM, Niklas Keller <[email protected]> wrote:
>
>> Andrea Faulds <[email protected]> schrieb am Sa., 29. Juli 2017, 18:55:
>>
>>> Hi Craig,
>>>
>>> Craig Duncan wrote:
>>>> On 29 July 2017 at 15:16, Andrea Faulds <[email protected]> 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/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sorry, only registered users may post in this forum.

Click here to login