json_encode() / json_decode() warnings

  100061
July 27, 2017 15:41 php@duncanc.co.uk (Craig Duncan)
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
  100062
July 27, 2017 15:57 me@kelunik.com (Niklas Keller)
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: > > ```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
  100063
July 27, 2017 16:00 php@duncanc.co.uk (Craig Duncan)
On 27 July 2017 at 16:57, Niklas Keller <me@kelunik.com> 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
  100068
July 28, 2017 06:56 giovanni@giacobbi.net (Giovanni Giacobbi)
On 27 July 2017 at 18:00, Craig Duncan <php@duncanc.co.uk> wrote:

> On 27 July 2017 at 16:57, Niklas Keller <me@kelunik.com> 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
  100069
July 28, 2017 06:59 me@kelunik.com (Niklas Keller)
2017-07-28 8:56 GMT+02:00 Giovanni Giacobbi <giovanni@giacobbi.net>:

> On 27 July 2017 at 18:00, Craig Duncan <php@duncanc.co.uk> wrote: > >> On 27 July 2017 at 16:57, Niklas Keller <me@kelunik.com> 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
  100070
July 28, 2017 10:03 ryan.jentzsch@gmail.com (Ryan Jentzsch)
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 <me@kelunik.com> wrote:

> 2017-07-28 8:56 GMT+02:00 Giovanni Giacobbi <giovanni@giacobbi.net>: > > > On 27 July 2017 at 18:00, Craig Duncan <php@duncanc.co.uk> wrote: > > > >> On 27 July 2017 at 16:57, Niklas Keller <me@kelunik.com> 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 >
  100081
July 28, 2017 13:30 thruska@cubiclesoft.com (Thomas Hruska)
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/
  100103
July 29, 2017 14:16 ajf@ajf.me (Andrea Faulds)
Hi Duncan,

Craig Duncan wrote:
> On 27 July 2017 at 16:57, Niklas Keller <me@kelunik.com> 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/
  100105
July 29, 2017 15:53 ajf@ajf.me (Andrea Faulds)
Hi everyone,

Andrea Faulds wrote:
> Craig Duncan wrote: >> On 27 July 2017 at 16:57, Niklas Keller <me@kelunik.com> 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/
  100106
July 29, 2017 16:08 php@duncanc.co.uk (Craig Duncan)
On 29 July 2017 at 15:16, Andrea Faulds <ajf@ajf.me> 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.
  100108
July 29, 2017 16:55 ajf@ajf.me (Andrea Faulds)
Hi Craig,

Craig Duncan wrote:
> On 29 July 2017 at 15:16, Andrea Faulds <ajf@ajf.me> 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/
  100111
July 29, 2017 18:10 me@kelunik.com (Niklas Keller)
Andrea Faulds <ajf@ajf.me> schrieb am Sa., 29. Juli 2017, 18:55:

> Hi Craig, > > Craig Duncan wrote: > > On 29 July 2017 at 15:16, Andrea Faulds <ajf@ajf.me> 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
>
  100126
July 30, 2017 19:01 bukka@php.net (Jakub Zelenka)
On Sat, Jul 29, 2017 at 7:10 PM, Niklas Keller <me@kelunik.com> wrote:

> Andrea Faulds <ajf@ajf.me> schrieb am Sa., 29. Juli 2017, 18:55: > > > Hi Craig, > > > > Craig Duncan wrote: > > > On 29 July 2017 at 15:16, Andrea Faulds <ajf@ajf.me> 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. :)
  100141
August 1, 2017 03:26 ajf@ajf.me (Andrea Faulds)
Jakub Zelenka wrote:
> On Sat, Jul 29, 2017 at 7:10 PM, Niklas Keller <me@kelunik.com> wrote: > >> Andrea Faulds <ajf@ajf.me> schrieb am Sa., 29. Juli 2017, 18:55: >> >>> Hi Craig, >>> >>> Craig Duncan wrote: >>>> On 29 July 2017 at 15:16, Andrea Faulds <ajf@ajf.me> 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/