On Fri, May 10, 2019 at 2:52 PM Riikka Kalliomäki <
riikka.kalliom...@riimu.net> wrote:
> > The new code has what looks like, to me, a refactor bug. I'd expect that,
> when one adds the JSON_THROW_ON_ERROR flag, one must also remove the
> subsequent json_last_error() handling, as it's no longer relevant to that
> call and instead relevant to the most recent prior call.
>
> This does create a kind of an unfortunate situation with vendor
> libraries, however. Any library that accepts arbitrary json flags and
> uses json_last_error() for error handling will be broken if
> JSON_THROW_ON_ERROR is passed as a flag.
>
I would expect such libraries to be sensitive to the possible variations
implied by accepting arbitrary arguments, and would compensate accordingly.
For example:
function library_json_decode(string $json, int $options) {
if (70300 <= PHP_VERSION_ID) {
$options &= ~JSON_THROW_ON_ERROR;
}
$decoded = json_decode($json, $options);
if (JSON_ERROR_NONE !== json_last_error()) {
throw new LibraryException(json_last_error_msg(), json_last_error());
}
return $decoded;
}
This means that library consumers effectively may have to become aware
> of how a library internally deals with json errors.
>
> It kind of turns JSON_THROW_ON_ERROR into unconventional BC break,
> because any code that takes arbitrary flags and uses json_last_error()
> needs to account for it. In some cases, I imagine, it can be necessary
> to add a "json_encode(null);" before the actual json handling just to
> reset the error state, just in case.
>
I agree this is possible, but I contend any function taking arbitrary
options needs to understand the implication of those options. It's in many
ways just like any other user-supplied data: authors must understand the
possible values available for the supported versions and handle the
accordingly.
> I wouldn't be surprised if this becomes a source of unexpected bugs in
> json handling code down the line. In my opinion, it's very unexpected
> behaviour that json_last_error() can reflect the state prior to the
> previous json_encode/json_decode call depending on the flags passed to
> the function as this has not been the case before.
>
> Perhaps it should be documented in the BC breaks section for 7.3 that
> json_last_error() may now reflect the state of arbitrary previous
> call?
>
Yes, better documentation surrounding JSON_THROW_ON_ERROR would be
welcomed. The docs are, right now, quite lean.
