Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:105785
To: internals@lists.php.net
References: <CA+kxMuS6gnPZqd-e5UV_zmE3q9ZpXS+UBSGNvF7hiagY15LFJw@mail.gmail.com>
Date: Fri, 24 May 2019 02:33:34 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:52.0)
 Gecko/20100101 Firefox/52.0 SeaMonkey/2.49.2
MIME-Version: 1.0
In-Reply-To: <CA+kxMuS6gnPZqd-e5UV_zmE3q9ZpXS+UBSGNvF7hiagY15LFJw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: JSON_THROW_ON_ERROR implementation detail
From: ajf@ajf.me (Andrea Faulds)
Message-ID: <php.internals-105785@news.php.net>

Hi Dan,

Sorry, I think I should directly address your points.

Dan Ackroyd wrote:
> 
> Apparently there is an implementation detail in JSON_THROW_ON_ERROR
> that differs in the RFC text, from the discussion on list
> http://news.php.net/php.internals/100569:
> 
>> I decided to reset it to no error because there's no
>> previous error that needs handling by error-checking code, the exception
>> takes care of that.
> 
> RFC text:
> 
>> When passed this flag, the error behaviour of these functions is changed.
>> The global  error state is left untouched, and if an error occurs that would
>> otherwise set it, these functions instead throw a JsonException with the
>> message and code set to whatever json_last_error() and json_last_error_msg()
>> would otherwise be respectively.

That difference came about, IIRC, because someone objected to the 
behaviour in, I think, a pull request comment. I think they were right, 
because…

> 
> The implementation is highly surprising and can lead to bugs.
> 
> Imagine this code exists in a code base.
> 
> ---------------------
> 
> $foo = json_decode('[bar');
> // many lines of code.
> // many lines of code.
> // many lines of code.
> 
> $bar = json_encode('Hello world!');
> if (json_last_error() !== JSON_ERROR_NONE) {
>      throw new Exception("encoding went wrong!");
> }
> 
> echo "All is fine";
> // output is "All is fine"
> ---------------------
> 
> 
> And to start moving to have json_encode throw on error, they start
> using the flag on the json_encode
> 
> ----------------------
> 
> $foo = json_decode('[bar');
> // many lines of code.
> // many lines of code.
> // many lines of code.
> 
> $bar = json_encode('Hello world!', JSON_THROW_ON_ERROR);
> if (json_last_error() !== JSON_ERROR_NONE) {
>      throw new Exception("encoding went wrong!");
> }
> 
> echo "All is fine";
> 
> -------------------
> 
> The result of this is that the user throw exception will be thrown
> even though the encode worked correctly. https://3v4l.org/JJD5g >
> This is highly surprising*, and doesn't seem the right thing to be doing.

The problem with that code is it mixes two intentionally separate 
error-handling mechanisms.

If you want to use the old last_error system:

   $bar = json_encode('Hello world!');
   if (json_last_error() !== JSON_ERROR_NONE) {
        // Handle error case
   }

If you want to use the new exception system:

   try {
       $bar = json_encode('Hello world!', JSON_THROW_ON_ERROR);
   } catch (JsonException $err) {
       // Handle error case
   }

These examples are internally consistent. The first (implicitly) 
requests the old error handling approach, then checks for an error the 
old way. The second explicitly requests the new error handling approach, 
then checks for an error the new way.

But your new example is not internally consistent, it asks for the new 
approach then tries to handle it the old way. It's true, it will break 
if the old-system global error state is dirty. But it will also break if 
there is an *actual error*:

   $bar = json_encode($unencodable, JSON_THROW_ON_ERROR); // throws 
JsonException, unhandled

The following line checking json_last_error() would never even be executed.

Let's imagine we were to make json_last_error() still be set if 
JSON_THROW_ON_ERROR was set. It would then be possible to write this:

   try {
       $bar = json_encode('Hello world!', JSON_THROW_ON_ERROR);
   } catch (JsonException $err) {
       if (json_last_error() !== JSON_ERROR_NONE) {
           // Handle error
       }
   }

But this is redundant, because when we've caught a JsonException we 
already know there was an error. Likewise if we just use 
json_last_error() to find out what type of error, this is also 
redundant, because $err already contains that information.

It would also, in that hypothetical scenario, be possible to write this:

    try {
       $bar = json_encode('Hello world!', JSON_THROW_ON_ERROR);
    } catch (JsonException $err) { }
    if (json_last_error() !== JSON_ERROR_NONE) {
       // Handle error
    }

This… would work, but it defeats the purpose of using 
JSON_THROW_ON_ERROR at all.

I agree it might be surprising to have json_last_error() not be touched 
when you use JSON_THROW_ON_ERROR, but I assign more value to keeping the 
error mechanisms cleanly separated than to allowing strangely-written 
self-defeating code to not crash.

At first I assumed you were making an argument about compatibility 
between PHP versions, where on an older version the JSON_THROW_ON_ERROR 
constant wouldn't do anything. But because the constant wouldn't exist, 
you couldn't use it without causing some error, so you'd really need to 
write:

     if (constant('JSON_THROW_ON_ERROR') !== NULL) {
         $bar = json_encode($foo, JSON_THROW_ON_ERROR);
     } else {
         $bar = json_encode($foo);
         if (json_last_error() !== JSON_ERROR_NONE) {
             throw new Exception(…);
         }
     }

Thanks,
Andrea