Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:126113
Message-ID: <8f268a8e-1646-455e-a1c2-b030581dcb9f@bastelstu.be>
Date: Thu, 5 Dec 2024 23:31:04 +0100
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PHP-DEV] [RFC] [Discussion] Error backtraces v2
To: erictnorris@gmail.com, PHP internals <internals@lists.php.net>
References: <CA+NMNO1zgCLLDOaWhqg8ipOrLETRn3w_91JNdCJz0zrKmeRWyg@mail.gmail.com>
Content-Language: en-US
In-Reply-To: <CA+NMNO1zgCLLDOaWhqg8ipOrLETRn3w_91JNdCJz0zrKmeRWyg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=C3=BCsterhus?=)

Hi

Thank you for your RFC. This time I'm making sure to comment already 
during the discussion period :-)

On 12/5/24 21:57, Eric Norris wrote:
> https://wiki.php.net/rfc/error_backtraces_v2
> 
> The RFC currently has two voting choices, the first is to accept an
> INI setting to configure an error mask for backtraces, and the second
> is to choose the default value for the error mask.

You are not restricted to Yes/No votes for the voting widgets. I suggest 
to rephrase the second voting widget to:

"What should the default value of the 'error_backtrace_recording' INI be?"

- E_FATAL_ERRORS
- 0

This avoids the complicated sentence above the voting widget.

The “php.ini Defaults” section should probably refer to the voting 
widget as well.

The first voting widget would probably be better phrased as:

"Add the 'error_backtrace_recording' INI as described?"

---

As for the RFC contents, I had a look at the previous discussion. Dan 
remarked that adding the backtrace to `error_get_last()` would keep any 
parameters to functions in the stack trace alive until the next error 
occurs. This can easily break RAII patterns, such as this example: 
https://3v4l.org/i6EOv

     <?php
     class Lock
     {
         public function __construct() { echo "Locked\n"; }
         public function __destruct() { echo "Unlocked\n"; }
     }

     function do_while_locked(Lock $lock)
     {
         // The exception implicitly keeps the lock alive.
         $GLOBALS['x'] = new Exception();
         echo "Doing something\n";
     }

     function foo()
     {
         $lock = new Lock();
         do_while_locked($lock);
         echo "Done\n";
     }

     foo();
     echo "After\n";

Normally one would expect the "Unlocked" to be printed between "Done" 
and "After", but this is not the case, because the Exception in the 
global keeps a reference to $lock within its backtrace. This would be 
the same if the Exception would instead be a `trigger_error()` that 
remains available via `error_get_last()`.

In other words, we would have a INI setting that changes application 
behavior in subtle ways, which is a no-no.

For the previous RFC this apparently was a non-issue, because the stack 
was stored as a string. But for your RFC this is an issue, because based 
on the tests it's stored as an array.

At the very least this gotcha should clearly be noted in the RFC text.

Best regards
Tim Düsterhus