Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:126120
Precedence: bulk
MIME-Version: 1.0
Date: Mon, 09 Dec 2024 15:34:42 +0100
To: erictnorris@gmail.com
Cc: PHP internals <internals@lists.php.net>
Subject: Re: [PHP-DEV] [RFC] [Discussion] Error backtraces v2
In-Reply-To: <CA+NMNO1LOXwLiRs51p-QnVkcs829RHxpZkC=ZArhFUTz6Qg-aA@mail.gmail.com>
References: <CA+NMNO1zgCLLDOaWhqg8ipOrLETRn3w_91JNdCJz0zrKmeRWyg@mail.gmail.com>
 <8f268a8e-1646-455e-a1c2-b030581dcb9f@bastelstu.be>
 <CA+NMNO1LOXwLiRs51p-QnVkcs829RHxpZkC=ZArhFUTz6Qg-aA@mail.gmail.com>
Message-ID: <a2fc46c291cb765dbfc7f4fa05f0cffc@bastelstu.be>
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

Am 2024-12-06 02:08, schrieb Eric Norris:
>> 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?"
> 
> I'm a little nervous about this change, because I was hoping to be a
> little more open-ended. For example, I'm not tied to the name
> 'error_backtrace_recording', so I wouldn't want someone to vote 'no'
> because they didn't like the setting name. Secondly, maybe we don't
> want the zend global (which I'll actually get to in a bit), and
> similarly I wouldn't want someone to vote 'no' because of that; I
> don't actually care about whether it's a global or not, I only care
> about getting backtraces for fatal errors.

I believe RFCs should be sufficiently precise so that one is able to 
implement the proposal reasonably unambiguously based on the RFC text 
alone. Given that you are concerned about the INI setting causing the 
RFC not to pass, it should be evident that this is such an important 
topic that it needs the RFC to be clear about what is proposed for 
voters to be able to vote on the full picture.

>> 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.
> 
> Great example, thank you. Since you are stating this is problematic -
> and I don't necessarily disagree - does this make allowing backtraces
> as implemented for non-fatal errors a non-starter? Presumably having a
> backtrace for a fatal error doesn't matter since everything will soon
> be destroyed in the process of shutting down.

Yes, I agree that having a stacktracke for fatal errors is a non-issue. 
For non-fatal errors the INI setting definitely is a problem. 
Unconditionally having a stacktrace for non-fatal errors I'm am not yet 
sure if this would be a problem or if it would be fine if it is a 
RFC-agreed-upon (breaking) change. Given that it's common to turn 
warnings and notices into Exceptions and thus to treat them as hard 
errors, I'd be inclined to say that the value-add of having backtraces 
for them is fairly small compared to the possible impact and that it 
makes sense not to include this in the RFC at all.

>> 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.
> 
> I would have preferred to make this a parameter to zend_error_cb, but
> I thought that changing the signature of that method might be too much
> of a backwards compatibility break (though this would only affect
> extensions). Making it a parameter would shorten the lifetime of the

Making breaking changes to the internal API is generally fine if there 
is a reason for making the change.

Best regards
Tim Düsterhus