[PHP-DEV] [RFC] Raise warnings for json_encode() and json_decode() issues

  100071
July 28, 2017 10:59 php@duncanc.co.uk (Craig Duncan)
Hi internals.

As my initial thread about introducing warnings to the JSON functions was
not immediately flooded with disagreement I took the liberty of creating an
RFC for official discussion.

The proposal is to have `json_encode()` and `json_decode()` raise warnings
whenever a failure occurs, instead of requiring the developer to call
`json_last_error()` each time.

The functionality of `json_last_error()` and `json_last_error_msg()` are
unaffected and they can still be used in exactly the same way they are today

https://wiki.php.net/rfc/json_encode_decode_errors

Thanks,
Craig
  100072
July 28, 2017 11:07 nikita.ppv@gmail.com (Nikita Popov)
On Fri, Jul 28, 2017 at 12:59 PM, Craig Duncan <php@duncanc.co.uk> wrote:

> Hi internals. > > As my initial thread about introducing warnings to the JSON functions was > not immediately flooded with disagreement I took the liberty of creating an > RFC for official discussion. > > The proposal is to have `json_encode()` and `json_decode()` raise warnings > whenever a failure occurs, instead of requiring the developer to call > `json_last_error()` each time. > > The functionality of `json_last_error()` and `json_last_error_msg()` are > unaffected and they can still be used in exactly the same way they are > today > > https://wiki.php.net/rfc/json_encode_decode_errors > > Thanks, > Craig >
Operations that are expected to fail should never generate warnings. We should not design functions such that their correct use requires the use of the error suppression operator. I certainly agree that the current situation is not good and that the json functions really ought to be throwing exceptions, but adding a warning now is only going to make this worse. Nikita
  100074
July 28, 2017 11:23 php@duncanc.co.uk (Craig Duncan)
Hi Nikita,

Thanks for your input. Would you vote yes for throwing an exception?


On 28 July 2017 at 12:07, Nikita Popov ppv@gmail.com> wrote:

> > Operations that are expected to fail should never generate warnings. We > should not design functions such that their correct use requires the use of > the error suppression operator. > > I certainly agree that the current situation is not good and that the json > functions really ought to be throwing exceptions, but adding a warning now > is only going to make this worse. > > Nikita > >
  100076
July 28, 2017 11:39 bukka@php.net (Jakub Zelenka)
On Fri, Jul 28, 2017 at 12:23 PM, Craig Duncan <php@duncanc.co.uk> wrote:

> Hi Nikita, > > Thanks for your input. Would you vote yes for throwing an exception? > > Just to clarify exceptions are no go for 7.x. It would have to be 8.x and
it would be a huge BC break so I'm quite confident that this will fail the vote!
  100078
July 28, 2017 11:46 narf@devilix.net (Andrey Andreev)
Hi,

On Fri, Jul 28, 2017 at 2:39 PM, Jakub Zelenka <bukka@php.net> wrote:
> On Fri, Jul 28, 2017 at 12:23 PM, Craig Duncan <php@duncanc.co.uk> wrote: > >> Hi Nikita, >> >> Thanks for your input. Would you vote yes for throwing an exception? >> >> > Just to clarify exceptions are no go for 7.x. It would have to be 8.x and > it would be a huge BC break so I'm quite confident that this will fail the > vote!
This is a long shot, but here's an idea: an OOP API throwing exceptions. Would require more work both to implement and use, but it would surely be less error-prone and there'd be no BC break. Cheers, Andrey.
  100079
July 28, 2017 11:51 php@duncanc.co.uk (Craig Duncan)
On 28 July 2017 at 12:46, Andrey Andreev <narf@devilix.net> wrote:

> > This is a long shot, but here's an idea: an OOP API throwing exceptions. > > Would require more work both to implement and use, but it would surely > be less error-prone and there'd be no BC break. >
Hi Andrey, Yes that would resolve all BC issues, however that wouldn't help with new developers picking up the `json_*` functions and using them dangerously. As pointed out in the original thread, there are plenty of wrappers that provide this feature, the problem is that core functions have a huge gotcha. If we introduced a new API then we'd need to deprecate the `json_*` functions to make users aware, which I imagine would be the most contentious thing suggested thus far
  100073
July 28, 2017 11:08 narf@devilix.net (Andrey Andreev)
Hi,

On Fri, Jul 28, 2017 at 1:59 PM, Craig Duncan <php@duncanc.co.uk> wrote:
> Hi internals. > > As my initial thread about introducing warnings to the JSON functions was > not immediately flooded with disagreement I took the liberty of creating an > RFC for official discussion. > > The proposal is to have `json_encode()` and `json_decode()` raise warnings > whenever a failure occurs, instead of requiring the developer to call > `json_last_error()` each time. > > The functionality of `json_last_error()` and `json_last_error_msg()` are > unaffected and they can still be used in exactly the same way they are today > > https://wiki.php.net/rfc/json_encode_decode_errors >
I too think that JSON error handling can be a bit tedious at times, but I certainly prefer it that way than having to suppress all json_decode() calls. Cheers, Andrey.
  100075
July 28, 2017 11:36 bukka@php.net (Jakub Zelenka)
On Fri, Jul 28, 2017 at 11:59 AM, Craig Duncan <php@duncanc.co.uk> wrote:

> Hi internals. > > As my initial thread about introducing warnings to the JSON functions was > not immediately flooded with disagreement I took the liberty of creating an > RFC for official discussion. > > The proposal is to have `json_encode()` and `json_decode()` raise warnings > whenever a failure occurs, instead of requiring the developer to call > `json_last_error()` each time. > > The functionality of `json_last_error()` and `json_last_error_msg()` are > unaffected and they can still be used in exactly the same way they are > today > > https://wiki.php.net/rfc/json_encode_decode_errors > > I don't think this is a good idea. Especially in case of json_decode the
invalid JSON comes from sources that user does not have any control and can't prevent the fail. Emitting warning wouldn't be helpful and it would just make it more difficult to handle such cases. Also it would break a big amount of code because in case of converting errors to exceptions, you start getting a different exception so it would have to be handled. I would call the whole proposal just a big BC break (I'm aware that we don't consider adding warnings as BC but in this case it is) for no benefit at all! Cheers Jakub
  100077
July 28, 2017 11:41 php@duncanc.co.uk (Craig Duncan)
On 28 July 2017 at 12:36, Jakub Zelenka <bukka@php.net> wrote:

> > Also it would break a big amount of code because in case of converting > errors to exceptions, you start getting a different exception so it would > have to be handled. I would call the whole proposal just a big BC break > (I'm aware that we don't consider adding warnings as BC but in this case it > is) for no benefit at all! > > Hi Jakub, thanks for sharing you opinion.
While I agree there are BC concerns, I don't think it's accurate to say no benefit at all. I regularly see new (and experienced) developers using these functions without checking `json_last_error()`, trying to figure out why the app is failing later without any warnings in the log can be very difficult.