On Wed, Jun 3, 2020 at 9:04 AM Nikita Popov <nikita....@gmail.com> wrote:
> On Tue, Jun 2, 2020 at 10:57 PM Max Semenik <maxsem.w...@gmail.com> wrote: > >> On Sun, May 31, 2020 at 4:47 PM Nikita Popov <nikita....@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
