Welcome! Log In Create A New Profile

Advanced

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

Posted by 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
On Fri, Jul 28, 2017 at 12:59 PM, Craig Duncan <[email protected]> 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
Hi,

On Fri, Jul 28, 2017 at 1:59 PM, Craig Duncan <[email protected]> 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.

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Hi Nikita,

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


On 28 July 2017 at 12:07, Nikita Popov <[email protected]> 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
>
>
On Fri, Jul 28, 2017 at 11:59 AM, Craig Duncan <[email protected]> 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
On Fri, Jul 28, 2017 at 12:23 PM, Craig Duncan <[email protected]> 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!
On 28 July 2017 at 12:36, Jakub Zelenka <[email protected]> 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.
Hi,

On Fri, Jul 28, 2017 at 2:39 PM, Jakub Zelenka <[email protected]> wrote:
> On Fri, Jul 28, 2017 at 12:23 PM, Craig Duncan <[email protected]> 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.

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On 28 July 2017 at 12:46, Andrey Andreev <[email protected]> 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
Sorry, only registered users may post in this forum.

Click here to login