[Debug] generic ErrorHandler #10921
[Debug] generic ErrorHandler #10921
Conversation
|
Wow! You've putted a lot of effort to document your work, which is great! |
|
I just updated ErrorHandler to take advantage of UniversalErrorHandler. |
As long as you stick to http://symfony.com/bc all is fine :) |
| $stack = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 10); | ||
| } | ||
|
|
||
| self::$loggers['deprecation']->warning($message, array('type' => self::TYPE_DEPRECATION, 'stack' => $stack)); |
There was a problem hiding this comment.
Removing this line is the most severe BC break that this patch does: the deprecation logger is now going to receive a more standardized context as second argument (type, file, line, trace, level). So the change is about renaming stack to trace and having type's value set to E_DEPRECATED or E_USER_DEPRECATED, not self::TYPE_DEPRECATION anymore.
To me, this special const was a hack, so removing it is a good thing (c).
|
Just came close to the functionality provided by tracy and I'll switch back both the dev and prod error handlers. |
|
Uncaught exceptions, OOM and other fatal errors are now handled thanks to #10941 and updates in UniversalErrorHandler. I'm quite ok with the way things a "universalized" now, but that needs reviews. Feedback welcomed. I'll now work on reconfiguring Symfony for taking advantage of UniversalErrorHandler (last commit is a WIP). But I have a question: I'm not comfortable with the 3 monolog channels Symfony currently has (emergency, deprecation, scream). I'd prefer having only one for php-errors. But then, should I preserve BC, ie map the old channels somewhere? The UniversalErrorHandler is currently able to have one different logger/channel per error level, just to cope with Symfony's way of logging errors. But I'm not sure this is a best practice, so I'd prefer removing this possibility, if that's a Good Thing. Is it? |
|
@nicolas-grekas using a separate channel for deprecations was the way found to integrate deprecations in the profiler AFAIK. |
|
IMO emergency makes no sense because that could be on the main app channel or something with a log level set to EMERGENCY. deprecation it's good to keep it separated, and scream I am not quite sure what it is, errors that have an @ but scream was enabled or ..? |
|
@stof it maybe was the intention, but if I read the code correctly, channel information is lost during data collection, so the code relies on a side effect, based on |
Having N logger has a cost. So i'm 👎 to have a logger per php-errors by default. But indeed, if it's configurable (in a PHP way) it could be perfect. |
|
I pushed a new iteration, the PR is ready for review, please comment. Thanks for your feedback @lyrixx, with this PR, you should be able to plug e.g. Sentry by service configuration, no need to statically ErrorHandler::setLogger() directly anymore: |
| @@ -11,6 +11,7 @@ | |||
|
|
|||
| namespace Symfony\Bundle\FrameworkBundle\DependencyInjection; | |||
|
|
|||
| use Psr\Log\LogLevel; | |||
There was a problem hiding this comment.
currently, FrameworkBundle does not depend on psr/log in its composer.json. but this makes it required now (while the optional logger in some classes does not). You need to update the composer.json to require psr/log in the standalone bundle (the fullstack package already depends on psr/log)
|
I just discovered that something is missing since a long time in the configuration of the prod environment for error handling: the This led me to two points:
Also, I updated the behavior of the proposed error handler for E_RECOVERABLE_ERROR and E_USER_ERROR levels: these levels are special in that they are considered fatal by the native PHP error handler, even when @-silenced. Thus, the only viable option for a user error handler is to always throw an exception for these two types, no matter the error_reporting() level nor anything else. This special behavior is an other argument for always registering the error handler, even and especially in the prod env. |
|
This PR is ready for review/merge IMHO. |
| * | ||
| * @deprecated since 2.6, to be removed in 3.0. | ||
| */ | ||
| private $displayErrors = 0x1FFF; |
There was a problem hiding this comment.
a private property does not need deprecated. It can be removed directly as there is no BC concern for private stuff
There was a problem hiding this comment.
I prefer keeping the @deprecated notice because that will make easier to spot where code is to be removed when moving to 3.0.
I can't remove this private right now since it's used in a deprecated method.
|
I just merged AbstractExceptionHandler back into ExceptionHandler. |
| @@ -130,7 +132,7 @@ public function handle(\Exception $exception) | |||
| * @see sendPhpResponse | |||
| * @see createResponse | |||
| */ | |||
| private function failSafeHandle(\Exception $exception) | |||
| protected function failSafeHandle(\Exception $exception) | |||
There was a problem hiding this comment.
why are you changing the visibility ?
There was a problem hiding this comment.
Good catch, I missed telling about that: I think that we should open more custom use of the ExceptionHandler. The failSafeHandle() is the right hooking point. It's a more lightweight version of the previous AbstractExceptionHandler.
There was a problem hiding this comment.
sendPhpResponse() and createResponse() are public, so there is already a way to change the way this method works.
There was a problem hiding this comment.
I just removed the private failSafeHandle completely and replaced it by a call to sendPhpResponse().
Still ok for you?
|
except for the visibility change, I'm 👍 |
|
I'm 👍 on the ErrorHandler changes, I'm pretty confident that changes in the framework glue have been carefully reviewed by @stof |
|
Good to me 👍 |
This PR was merged into the 2.6-dev branch. Discussion ---------- [Debug] generic ErrorHandler | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | minor, see updated tests | Deprecations? | yes | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | none The proposed goal of this PR is to build a class that can serve as a foundation for a standard error handler, shared with other projects and not only used by the FrameworkBundle. This is a merge of my previous work on the subject (https://github.com/nicolas-grekas/Patchwork-Error-Logger/blob/master/class/Patchwork/PHP/InDepthErrorHandler.php) and recent improvements of error handling in Symfony's Debug\ErrorHandler. ExceptionHandler has a new AbstractExceptionHandler base class that factors out the handling of fatal errors casted to exceptions. ErrorHandler is introduced, which provides five bit fields that control how errors are handled: - thrownErrors: errors thrown as ContextErrorException - loggedErrors: logged errors, when not @-silenced - scopedErrors: errors thrown or logged with their local context - tracedErrors: errors logged with their trace, only once for repeated errors - screamedErrors: never @-silenced errors Each error level can be logged by a dedicated PSR-3 logger object. Screaming only applies to logging. Throwing takes precedence over logging. Uncaught exceptions are logged as E_ERROR. E_DEPRECATED and E_USER_DEPRECATED levels never throw. E_RECOVERABLE_ERROR and E_USER_ERROR levels always throw. Non catchable errors that can be detected at shutdown time are logged when the scream bit field allows so. As errors have a performance cost, repeated errors are all logged, so that the developer can see them and weight them as more important to fix than others of the same level. - [x] build a more generic ErrorHandler - [x] update service/listeners definitions to take advantage of the new interface of ErrorHandler - [x] add phpdocs - [x] add tests Commits ------- 1701447 [Debug] update FrameworkBundle and HttpKernel for new ErrorHandler 839e9ac [Debug] generalized ErrorHandler
|
First merge 👏 |
|
It's your turn now @romainneutron ! Let's help @fabpot scale with Symfony :) |
|
Hi @nicolas-grekas ErrorHandler::register()->throwAt(E_ALL, true);I started to debug ErrorHandler and found this PR. I see you made the following changes
But I really don't understand why? Could you please explain this point, or get me some post about it? |
|
Because a deprecation should never break the execution flow: they are just notices you can ignore - and you should log if you don't want to. |
|
Thank you, @nicolas-grekas for your faster reaction. public function throwAt($levels, $replace = false, $ignoreDeprecated = true)
{
$prev = $this->thrownErrors;
$this->thrownErrors = ($levels | E_RECOVERABLE_ERROR | E_USER_ERROR);
if ($ignoreDeprecated) {
$this->thrownErrors &= ~E_USER_DEPRECATED & ~E_DEPRECATED;
}
//...
} I will not change behavior by default, but allow to use the wondeful feature of see that errors while development. |
|
I don't think that's the appropriate place to do so. |
| */ | ||
| private function reRegister($prev) | ||
| { | ||
| if ($prev !== $this->thrownErrors | $this->loggedErrors) { |
There was a problem hiding this comment.
Sorry to be so late, but isn't there an operator precedence bug here? https://3v4l.org/UmbJk
There was a problem hiding this comment.
Looks like so! Please send a fix, branch 4.4
…avier) This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [Debug][ErrorHandler] fix operator precedence | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #10921 (comment) | License | MIT But I'm struggling to come up with a *failing* test scenario :/ if the bug is only causing evaluating the condition as truthy instead of false when `$prev` is already equal to `$this->thrownErrors | $this->loggedErrors`, I guess it will just re-register the same, without real visible effect? Commits ------- 7f68914 [Debug][ErrorHandler] fix operator precedence
The proposed goal of this PR is to build a class that can serve as a foundation for a standard error handler, shared with other projects and not only used by the FrameworkBundle.
This is a merge of my previous work on the subject (https://github.com/nicolas-grekas/Patchwork/blob/master/core/logger/class/Patchwork/PHP/InDepthErrorHandler.php) and recent improvements of error handling in Symfony's Debug\ErrorHandler.
ErrorHandler is introduced, which provides five bit fields that control how errors are handled:
Each error level can be logged by a dedicated PSR-3 logger object.
Screaming only applies to logging.
Throwing takes precedence over logging.
Uncaught exceptions are logged as E_ERROR.
E_DEPRECATED and E_USER_DEPRECATED levels never throw.
E_RECOVERABLE_ERROR and E_USER_ERROR levels always throw.
Non catchable errors that can be detected at shutdown time are logged when the scream bit field allows so.
As errors have a performance cost, repeated errors are all logged, so that the developer
can see them and weight them as more important to fix than others of the same level.