Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:113264
MIME-Version: 1.0
References: <CAGgaK7KScGnNaCSnz6szCzJ3jndoipH0=yonW6FPKR7GbkPdLw@mail.gmail.com>
 <CA+kxMuRVwhMjZxA0ctz4PK9VNvNTYSga6bg1V54KeqHXMguQgw@mail.gmail.com>
 <CAGgaK7JJO8bit-dADUpP013PgiyvKzd6Jy6anemwzTkx4UpiUA@mail.gmail.com> <CAF+90c8iMTdWb+sonJYCaao229u-tU1haAwOTfM61XhH1e_jWQ@mail.gmail.com>
In-Reply-To: <CAF+90c8iMTdWb+sonJYCaao229u-tU1haAwOTfM61XhH1e_jWQ@mail.gmail.com>
Date: Thu, 25 Feb 2021 14:18:09 +0100
Message-ID: <CAF+90c-qmKOv9L-BtAEOTSHvec_cCpsrqtu72Ez8EZgvNka+XQ@mail.gmail.com>
To: Max Semenik <maxsem.wiki@gmail.com>
Cc: Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="00000000000097f2bd05bc28fc51"
Subject: Re: [PHP-DEV] RFC: Error backtraces
From: nikita.ppv@gmail.com (Nikita Popov)

--00000000000097f2bd05bc28fc51
Content-Type: text/plain; charset="UTF-8"

On Wed, Jun 3, 2020 at 9:04 AM Nikita Popov <nikita.ppv@gmail.com> wrote:

> On Tue, Jun 2, 2020 at 10:57 PM Max Semenik <maxsem.wiki@gmail.com> wrote:
>
>> On Sun, May 31, 2020 at 4:47 PM Nikita Popov <nikita.ppv@gmail.com>
>> wrote:
>>
>> > I'm concerned about the performance implications of this change.
>> Backtrace
>> > gathering is quite expensive and doing this for every diagnostic will
>> have
>> > a large performance impact if there are many of them. This is okay if
>> your
>> > code is intended to be diagnostics clean, but unfortunately this is not
>> a
>> > given. If you generate 10k deprecation warnings during a request, you're
>> > definitely going to feel those backtraces.
>> >
>>
>> INI setting should take care of this. If your code is crap, don't flip it
>> on.
>>
>
> There's a couple of problems with this line of argumentation:
>
> First, even if the code is crap, wouldn't it still want to have stack
> traces for fatal errors? Why should crap code be denied stack traces? Isn't
> that the code that needs them most? If the code weren't crap it wouldn't be
> causing fatal errors anyway ;)
>
> Second, PHP has plenty of functions (especially I/O functions) where
> ignoring notices and warnings is a requirement. You are not interested in
> back traces for errors your ignore. Keep in mind that error_get_last() data
> is populated independently of whether errors are suppressed or not.
>
> Similarly, if your code promoted all warnings to exceptions using a custom
> error handler, then you'll perform a duplicate backtrace calculation, first
> when the warning is originally through, and then again if you construct the
> exception. (This is less bad than other cases, because it's "just" twice as
> slow.)
>
> Finally, even if we completely disregard the question of performance, back
> traces are very noisy. You get one fatal error per request, but you might
> easily get 1k warnings if something happens to trigger in a loop. With back
> traces, those 1k warnings will be 100k lines.
>
> I'm not sure if limiting it to just fatals is the best choice, but I do
> think that this needs a bit more control. And limited to just backtraces
> for fatals, I think this functionality should be enabled by default.
>
> This might also want to integrate with the zend.exception_ignore_args
> option (or, because that one is exception-specific, a renamed option that
> does the same thing). We will want to disable argument gathering in a
> default production configuration, because the arguments may contain
> sensitive data that must not appear inside log files.
>
> Regards,
> Nikita
>

To put my feedback into more actionable form: Rather than adding an ini
setting that enables error backtraces for all diagnostics, I'd make it a
mask of error types for which a backtrace should be captured, and default
it to fatal errors only. So error_reporting=E_ALL,
error_backtrace=E_ERROR|E_CORE_ERROR|E_COMPILE_ERROR|E_USER_ERROR|E_RECOVERABLE_ERROR|E_PARSE.
It might be handy to expose the internal E_FATAL_ERRORS constant we have
for that.

I think this should give a very sensible default behavior, and people who
want to capture backtraces for all errors still have an option to do so.

Finally, I'm wondering if the backtrace in error_get_last() shouldn't be in
array form, rather than string form.

Regards,
Nikita

--00000000000097f2bd05bc28fc51--