Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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.

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

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:4.4fromnicolas-grekas:eh-utils
Jun 24, 2021

Conversation

@nicolas-grekas
Copy link
Member

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#41745
LicenseMIT
Doc PR-

*/
publicstaticfunctiongetSeverity($throwable):int
{
if ($throwableinstanceof \ErrorException) {

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 forinstanceof 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 makeSilencedErrorContext extends \ErrorException will be better solution?

Copy link
MemberAuthor

@nicolas-grekasnicolas-grekasJun 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.

benbor reacted with thumbs up emoji

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 words76e7944

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
MemberAuthor

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.

benbor reacted with thumbs up emoji
@nicolas-grekasnicolas-grekas merged commit119b3ec intosymfony:4.4Jun 24, 2021
@nicolas-grekasnicolas-grekas deleted the eh-utils branchJune 28, 2021 07:38
This was referencedJun 30, 2021
PhilETaylor pushed a commit to PhilETaylor/symfony that referenced this pull requestSep 6, 2023
…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       |Fixsymfony#41745| License       | MIT| Doc PR        | -Commits-------07407c7 [ErrorHandler] fix handling buffered SilencedErrorContext
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ycerutoycerutoyceruto approved these changes

+1 more reviewer

@benborbenborbenbor approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

4 participants

@nicolas-grekas@yceruto@benbor@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp