[ErrorHandler] fix handling buffered SilencedErrorContext #41760
Conversation
nicolas-grekas
commented
Jun 21, 2021
nicolas-grekas
commented
| Q | A |
|---|---|
| Branch? | 4.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Tickets | Fix #41745 |
| License | MIT |
| Doc PR | - |
| { | ||
| if ($throwable instanceof \ErrorException) { |
There was a problem hiding this comment.
Hello.
Such change (extra check for instanceof SilencedErrorContext) will lead other tools (PSR3 compatible loggers), which works with \ErrorException and don't know about Symfony to copy-paste exactly the same statement (|| $throwable instanceof SilencedErrorContext) in their implementation.
Don't you think, that make SilencedErrorContext extends \ErrorException will be better solution?
There was a problem hiding this comment.
There are no contracts with PSR3 loggers. An entry named "exception" can hold any type.
Also, extending ErrorException would mean storing a stack trace, which is precisely the thing we want to avoid here.
There was a problem hiding this comment.
After few hours I found one more place in the symfony error-handler codebase where || $ instanceof SilencedErrorContext may be missed
https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/ErrorHandler/Resources/views/logs.html.php#L20
extending ErrorException would mean storing a stack trace, which is precisely the thing we want to avoid here.
SilencedErrorContext has "lightTrace" so, as I understand the question is not in the performance (moreover $silencedErrorCache works well)
Correct me if I wrong, you don't want to storing trace only for better VarDumper usage #19568?
As I can see Symfony components designed like "independents" from each other. And this ErrorHandler so cool to be used outside of Symfony Framework (fe by people like me :) ) . I think it will be better to parametrize such tweaks (as backtrace depth) via __constructor than hardcode them for other component needs.
One more example of my words 76e7944
Let me know if the implementation of such Idea (backtrace depth) has a chance to be merged and I try my best to prepare such PR.
There was a problem hiding this comment.
Good catch, I updated the template file.
SilencedErrorContext is meant to represent silenced PHP errors.
We want it to be as light as possible, that's why they are very simple value objects and should stay as is.
…ontext (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [ErrorHandler] fix handling buffered SilencedErrorContext | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix symfony#41745 | License | MIT | Doc PR | - Commits ------- 07407c7 [ErrorHandler] fix handling buffered SilencedErrorContext
