Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[ErrorHandler] merge and remove the ErrorRenderer component#34312
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| 'exception' =>$exception, | ||
| 'exception' =>newHttpException($code,'This is a sampleexception.'), | ||
| 'logger' =>null, | ||
| 'showException' =>false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We are missing this attribute to show the non-debug version of the response (useful to test custom Twig-based HTML error pages) on e.g./__error/404.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
However, I'm not sure it's useful for non-HTML formats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll let you follow up on this topic after merge if you don't mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done in#34331
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
e900eea to7f7312fCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
b2d6471 tod3a7481Comparesrc/Symfony/Component/ErrorHandler/Resources/views/exception_full.html.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| publicstaticfunctiongetSubscribedEvents() | ||
| { | ||
| return [ | ||
| KernelEvents::CONTROLLER_ARGUMENTS =>'onControllerArguments', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It would be much better if we could add this listener only when necessary, i.e. within theonKernelException method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I considered doing this but decided not: mutating the event-dispatcher at runtime is something we'd better avoid all the time. Also, this could introduce subtle order-dependent behaviors.
I think the check is specific enough for having it always enabled, with better unconditional behavior.
yceruto left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It looks good to me as far as I've tried. However, we need another iteration to restore the preview mechanism.
Thank you Nicolas for taking over and improving it.
fabpot commentedNov 12, 2019
Thank you@nicolas-grekas. |
…onent (nicolas-grekas, yceruto)This PR was merged into the 4.4 branch.Discussion----------[ErrorHandler] merge and remove the ErrorRenderer component| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | yes| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -This PR supersedes#34288.Here is what it does:- Merge the `ErrorRenderer` component into `ErrorHandler`- Add `ErrorRendererInterface::render(\Throwable $e): FlattenException` and refactor error renderers around it.- Add `FlattenException::setAsString()` to make the previous possible.- Add `CliErrorRenderer` to render error on the CLI too. This means `VarDumper` is now a required dependency of `ErrorHandler`. This paves the way to use it also for rendering HTML - the logic there is much more advanced than what `HtmlErrorRenderer` provides and ever should provide.- Make `BufferingLogger` map its collected logs to `error_log()` if they are not emptied before.- Remove some classes that are not needed anymore (`ErrorRenderer`, `ErrorRendererPass`, `HtmlErrorRendererInterface`)- Simplified the logic in `Debug::enable()` - nobody uses its arguments- Fix a few issues found meanwhile.With these changes, the component can be used standalone. One is now able to require only it, register it either with either `ErrorHandler::register()` or `Debug::enable()` and profit.Commits-------d1bf1ca [ErrorHandler] help finish the PR6c9157b [ErrorHandler] merge and remove the ErrorRenderer component
This PR was merged into the 4.4 branch.Discussion----------[TwigBundle] Restore the preview mechanism| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -follow up#34312Commits-------d3f1121 [TwigBundle] Restore the preview mechanism
Uh oh!
There was an error while loading.Please reload this page.
This PR supersedes#34288.
Here is what it does:
ErrorRenderercomponent intoErrorHandlerErrorRendererInterface::render(\Throwable $e): FlattenExceptionand refactor error renderers around it.FlattenException::setAsString()to make the previous possible.CliErrorRendererto render error on the CLI too. This meansVarDumperis now a required dependency ofErrorHandler. This paves the way to use it also for rendering HTML - the logic there is much more advanced than whatHtmlErrorRendererprovides and ever should provide.BufferingLoggermap its collected logs toerror_log()if they are not emptied before.ErrorRenderer,ErrorRendererPass,HtmlErrorRendererInterface)Debug::enable()- nobody uses its argumentsWith these changes, the component can be used standalone. One is now able to require only it, register it either with either
ErrorHandler::register()orDebug::enable()and profit.