8000 [ErrorHandler] fix handling buffered SilencedErrorContext by nicolas-grekas · Pull Request #41760 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[ErrorHandler] fix handling buffered SilencedErrorContext #41760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 24, 2021
Merged

[ErrorHandler] fix handling buffered SilencedErrorContext #41760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
[ErrorHandler] fix handling buffered SilencedErrorContext
  • Loading branch information
@nicolas-grekas
nicolas-grekas committed Jun 23, 2021
commit 07407c74cabe1a22199a5a27e2ccf47ded580a5f
2 changes: 1 addition & 1 deletion src/Symfony/Component/ErrorHandler/Resources/views/logs.html.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
$status = 'warning';
} else {
$severity = 0;
if (($exception = $log['context']['exception'] ?? null) instanceof \ErrorException) {
if (($exception = $log['context']['exception'] ?? null) instanceof \ErrorException || $exception instanceof \Symfony\Component\ErrorHandler\Exception\SilencedErrorContext) {
$severity = $exception->getSeverity();
}
$status = \E_DEPRECATED === $severity || \E_USER_DEPRECATED === $severity ? 'warning' : 'normal';
Expand Down
9 changes: 7 additions & 2 deletions src/Symfony/Component/ErrorHandler/ThrowableUtils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,19 @@

namespace Symfony\Component\ErrorHandler;

use Symfony\Component\ErrorHandler\Exception\SilencedErrorContext;

/**
* @internal
*/
class ThrowableUtils
{
public static function getSeverity(\Throwable $throwable): int
/**
* @param SilencedErrorContext|\Throwable
*/
public static function getSeverity($throwable): int
{
if ($throwable instanceof \ErrorException) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author
@nicolas-grekas nicolas-grekas Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ($throwable instanceof \ErrorException || $throwable instanceof SilencedErrorContext) {
return $throwable->getSeverity();
}

Expand Down
0