Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:105671
MIME-Version: 1.0
References: <CA+kxMuS6gnPZqd-e5UV_zmE3q9ZpXS+UBSGNvF7hiagY15LFJw@mail.gmail.com>
 <CAEYWF=5E+ec9mn30GQu-wr61XSQKbymr9O8Mz5CDJ-p8oCwSBA@mail.gmail.com> <CA++kX6_4oRfGH1aNph9xZqEdyp6ep38qeMaJA1iK2aL5B6JbfQ@mail.gmail.com>
In-Reply-To: <CA++kX6_4oRfGH1aNph9xZqEdyp6ep38qeMaJA1iK2aL5B6JbfQ@mail.gmail.com>
Reply-To: bishop@php.net
Date: Fri, 10 May 2019 22:11:17 -0400
Message-ID: <CAEYWF=47uO2=ot5CmN3ZteM-RX-2zFqyQicrTh=ps2ZsNiFOKA@mail.gmail.com>
To: =?UTF-8?Q?Riikka_Kalliom=C3=A4ki?= <riikka.kalliomaki@riimu.net>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000002aea130588933482"
Subject: Re: [PHP-DEV] JSON_THROW_ON_ERROR implementation detail
From: bishop@php.net (Bishop Bettini)

--0000000000002aea130588933482
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, May 10, 2019 at 2:52 PM Riikka Kalliom=C3=A4ki <
riikka.kalliomaki@riimu.net> wrote:

> > The new code has what looks like, to me, a refactor bug. I'd expect tha=
t,
> 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 <=3D PHP_VERSION_ID) {
    $options &=3D ~JSON_THROW_ON_ERROR;
  }

  $decoded =3D json_decode($json, $options);
  if (JSON_ERROR_NONE !=3D=3D 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.

--0000000000002aea130588933482--