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

Add ErrorHandler component#31065

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
fabpot merged 1 commit intosymfony:4.4fromyceruto:error_exception_component
Jun 27, 2019

Conversation

@yceruto
Copy link
Member

@ycerutoyceruto commentedApr 10, 2019•
edited
Loading

QA
Branch?4.4
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?yes
Tests pass?no
Fixed tickets#25905,#26448
LicenseMIT
Doc PRTODO

Mainly for API-based apps that don't require TwigBundle to get the correct exception response according to the request format (aka_format attribute).

exception_response

✔️RFC7807 compliant for JSON and XML formats.

This introduce a newErrorRenderer service that render aFlattenException into a given format:

useSymfony\Component\ErrorHandler\ErrorRenderer\ErrorRenderer;useSymfony\Component\ErrorHandler\ErrorRenderer\HtmlErrorRenderer;useSymfony\Component\ErrorHandler\ErrorRenderer\JsonErrorRenderer;$renderers = [newHtmlErrorRenderer(),newJsonErrorRenderer(),// ...];$errorRenderer =newErrorRenderer($renderers);returnnewResponse($errorRenderer->render($exception,$request->getRequestFormat()),$exception->getStatusCode(),$exception->getHeaders());

The built-in error renderers are:

FormatClass
htmlHtmlErrorRenderer
jsonJsonErrorRenderer
xml, atomXmlErrorRenderer
txtTxtErrorRenderer

And you can add your own error renderer by implementing theErrorRendererInterface and tagging it witherror_handler.renderer in your service definition.

Creating your own error renderer for a built-in format will end up replacing the related built-in error renderer.

Demo:https://github.com/yceruto/error-handler-app (add custom error renderer)

Taluu, hhamon, onEXHovia, phansys, yceruto, Jibbarth, BafS, linaori, 94noni, hubbyist, and 21 more reacted with thumbs up emojiArnaudTarroux, fancyweb, Kocal, Taluu, yceruto, kevin-verschaeve, mickaelandrieu, Guikingone, Simperfit, juuuuuu, and 30 more reacted with heart emoji
@ycerutoyceruto changed the title[ErrorException] Add ErrorException componentAdd ErrorException componentApr 10, 2019
@ycerutoycerutoforce-pushed theerror_exception_component branch from42d5a64 to3f9b448CompareApril 10, 2019 16:28
@Simperfit
Copy link
Contributor

This is gonna be really nice !

@nicolas-grekasnicolas-grekas added this to thenext milestoneApr 11, 2019
@ycerutoycerutoforce-pushed theerror_exception_component branch from3f9b448 to342ee6eCompareApril 11, 2019 19:31
@nicolas-grekas
Copy link
Member

Would it make sense no name the componentErrorHandler?

@yceruto
Copy link
MemberAuthor

Would it make sense no name the component ErrorHandler?

Does that make me think that we should moveErrorHandler and ExceptionHandler classes from the Debug component to here, also? Currently, this component only solve the convertion of the exception to a formatted output (html, json, etc) but there is noregister()/handle() method involved at this moment.

@stof
Copy link
Member

The discussion we had with@fabpot and@nicolas-grekas in Brussels concluded that we should indeed create a new component for error handling. Having the production error handling being a responsibility of a component named debug looks weird, as prod is not in debug mode.

OskarStark, hhamon, hubbyist, yaffol, and wadjeroudi reacted with thumbs up emoji

@stof
Copy link
Member

I don't think having an ErrorException class being responsible both for representing the Exception and for rendering it in multiple formats is the right architecture for the component though. This is not extendable at all, and cannot even be configurable (as you cannot instantiate the rendering layer in a place configuring it).

To me, we need 3separate things in the component:

  • the ExceptionHandler/ErrorHandler (moved from Debug)
  • the value object representing the actual exception
  • a renderer layer knowing how to render the exception in different format (HTML, JSON corresponding to some standard format, XML, etc...)

And then, the Twig-based HTML error page should ideally become a hook into this renderer layer rather than being a totally separate rendering system.

@yceruto
Copy link
MemberAuthor

yceruto commentedApr 12, 2019•
edited
Loading

I agree, we should also discuss about BC then, there is a lot of exception classes in Debug component used by these two handlers and we wouldn't want this new component require tosymfony/debug :)

@ycerutoycerutoforce-pushed theerror_exception_component branch 3 times, most recently fromcb2ac97 to50ec30aCompareApril 12, 2019 19:34
@ycerutoyceruto changed the titleAdd ErrorException componentAdd ErrorHandler componentApr 12, 2019
@ycerutoycerutoforce-pushed theerror_exception_component branch from50ec30a tocc01b78CompareApril 12, 2019 19:56
@yceruto
Copy link
MemberAuthor

yceruto commentedApr 12, 2019•
edited
Loading

Update

  • Component renamed to "ErrorHandler" as suggested by Nicolas (it's a better name IMO too).
  • Almost everything was moved from the Debug component because interdependence between handlers -> classes/interface, exceptDebug class.

Still working on the renderer layer...

Copy link
Contributor

@TaluuTaluu left a comment

Choose a reason for hiding this comment

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

How about being able to support more formats ? Wouldn't it be better to have some sort of factory so that we can use whatever format we want ?

@ycerutoycerutoforce-pushed theerror_exception_component branch fromcc01b78 to8d641e3CompareApril 12, 2019 20:10
@yceruto
Copy link
MemberAuthor

How about being able to support more formats ? Wouldn't it be better to have some sort of factory so that we can use whatever format we want ?

Thanks@Taluu for your input, yes I'm working on it.

Taluu reacted with thumbs up emoji

@ycerutoycerutoforce-pushed theerror_exception_component branch from8d641e3 toa82888eCompareApril 13, 2019 14:43
@yceruto
Copy link
MemberAuthor

yceruto commentedApr 14, 2019•
edited
Loading

Update

  • Added error renderer layer with interface + DI tagerror_handler.renderer to add custom format/renderer:
interface ErrorRendererInterface{/**     * Gets the format of the content.     *     * @return string The content format     */publicstaticfunctiongetFormat():string;/**     * Renders an Exception and returns the Response content.     *     * @return string The Response content as a string     */publicfunctionrender(FlattenException$exception):string;}
  • Added FrameworkBundle integration.

@yceruto
Copy link
MemberAuthor

And then, the Twig-based HTML error page should ideally become a hook into this renderer layer rather than being a totally separate rendering system.

There are many changes (a big diff), most are movements + deprecations, so I prefer to leave this last issue for another PR

@ycerutoycerutoforce-pushed theerror_exception_component branch fromed3480a to3c05010CompareApril 14, 2019 01:25
@yceruto
Copy link
MemberAuthor

yceruto commentedApr 14, 2019•
edited
Loading

@ycerutoyceruto marked this pull request as ready for reviewApril 14, 2019 02:47
@fabpot
Copy link
Member

Thank you@yceruto.

@fabpotfabpot merged commit7057244 intosymfony:4.4Jun 27, 2019
fabpot added a commit that referenced this pull requestJun 27, 2019
This PR was merged into the 4.4 branch.Discussion----------Add ErrorHandler component| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | no| Fixed tickets |#25905,#26448| License       | MIT| Doc PR        | TODOMainly for API-based apps that don't require TwigBundle to get the correct exception response according to the request format (aka `_format` attribute).![exception_response](https://user-images.githubusercontent.com/2028198/55509651-713dc700-562a-11e9-8b98-bef3b0229397.gif):heavy_check_mark: [RFC7807](https://tools.ietf.org/html/rfc7807) compliant for JSON and XML formats.---This introduce a new `ErrorRenderer` service that render a `FlattenException` into a given format:```phpuse Symfony\Component\ErrorHandler\ErrorRenderer\ErrorRenderer;use Symfony\Component\ErrorHandler\ErrorRenderer\HtmlErrorRenderer;use Symfony\Component\ErrorHandler\ErrorRenderer\JsonErrorRenderer;$renderers = [    new HtmlErrorRenderer(),    new JsonErrorRenderer(),    // ...];$errorRenderer = new ErrorRenderer($renderers);return new Response(    $errorRenderer->render($exception, $request->getRequestFormat()),    $exception->getStatusCode(),    $exception->getHeaders());```The built-in error renderers are:| Format | Class || --- | --- || html | HtmlErrorRenderer || json | JsonErrorRenderer || xml, atom | XmlErrorRenderer || txt | TxtErrorRenderer |And you can add your own error renderer by implementing the `ErrorRendererInterface` and tagging it with `error_handler.renderer` in your service definition.Creating your own error renderer for a built-in format will end up replacing the related built-in error renderer.Demo:https://github.com/yceruto/error-handler-app ([add custom error renderer](yceruto/error-handler-app@06fc647))Commits-------7057244 Added ErrorHandler component
@ycerutoyceruto deleted the error_exception_component branchJune 27, 2019 12:46
@driesvints
Copy link
Contributor

driesvints commentedJul 4, 2019•
edited
Loading

@fabpot@yceruto the removed public methods likegetContent andgetStylesheet on theSymfony\Component\Debug\ExceptionHandler class are breaking changes according to SemVer. Can you please revert these removals or introduce this new functionality on 5.0 instead?

These changes will, for example break Lumen:laravel/lumen-framework#935

yceruto reacted with thumbs up emoji

@Tobion
Copy link
Contributor

please open a new issue

@driesvints
Copy link
Contributor

@Tobion done:#32371

nicolas-grekas added a commit that referenced this pull requestJul 9, 2019
…(1st step) (yceruto)This PR was squashed before being merged into the 4.4 branch (closes#32377).Discussion----------[Debug] Restoring back the state of the Debug component (1st step)| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets |#32371| License       | MITAfter a good discussion with@nicolas-grekas, we made the decision to split the current `ErrorCatcher` component into several: * `ErrorHandler` it would be the Debug component before these changes#31065, with everything related to ErrorHandler, Debug, DebugClassLoader classes and change its name. * `ErrorDumper` it would be the current ErrorCatcher but with FlattenException + the new error renderer system only.This is the first step, then we can deprecate everything for the Debug component in favor of the ErrorHandler and ErrorDumper components, **BUT without moving any code !!**, that would give us more freedom to do it correctly in the new components.NOTE: For this PR I've copy the `Debug` component directory from the revision prior to merged commit#31065 in 4.4 branch.Commits-------eda49e2 [Debug] Restoring back the state of the Debug component (1st step)
symfony-splitter pushed a commit to symfony/var-dumper that referenced this pull requestJul 9, 2019
…(1st step) (yceruto)This PR was squashed before being merged into the 4.4 branch (closes #32377).Discussion----------[Debug] Restoring back the state of the Debug component (1st step)| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets |symfony/symfony#32371| License       | MITAfter a good discussion with@nicolas-grekas, we made the decision to split the current `ErrorCatcher` component into several: * `ErrorHandler` it would be the Debug component before these changessymfony/symfony#31065, with everything related to ErrorHandler, Debug, DebugClassLoader classes and change its name. * `ErrorDumper` it would be the current ErrorCatcher but with FlattenException + the new error renderer system only.This is the first step, then we can deprecate everything for the Debug component in favor of the ErrorHandler and ErrorDumper components, **BUT without moving any code !!**, that would give us more freedom to do it correctly in the new components.NOTE: For this PR I've copy the `Debug` component directory from the revision prior to merged commitsymfony/symfony#31065 in 4.4 branch.Commits-------eda49e295e [Debug] Restoring back the state of the Debug component (1st step)
Tobion added a commit that referenced this pull requestJul 24, 2019
…formats and using ErrorRenderer as fallback (yceruto)This PR was merged into the 4.4 branch.Discussion----------[TwigBundle] Deprecating error templates for non-html formats and using ErrorRenderer as fallback| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -In the previous [PR](#31065) we created a new mechanism to render any PHP error/exception in a formatted string, which if the FB is enabled, would return an HTTP Response according to the preferred Request format (html, json, xml, txt, etc.), but when installing the TwigBundle this rendering mechanism is replaced by the current ExceptionController.This ExceptionController allows us to render custom error pages based on Twig in many formats, just what is already supported with the new ErrorRenderer component, so let's deprecate this in favor of the native.Commits-------bf0c24a Deprecating error templates for non-html formats and using ErrorRenderer
@nicolas-grekasnicolas-grekas modified the milestones:next,4.4Oct 27, 2019
This was referencedNov 12, 2019
@ghost
Copy link

ghost commentedDec 7, 2019•
edited by ghost
Loading

ErrorHandler respectingContent-Type is great in API projects! Thanks!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz left review comments

@derrabusderrabusderrabus left review comments

@fabpotfabpotfabpot approved these changes

@srozesrozeAwaiting requested review from sroze

+5 more reviewers

@TaluuTaluuTaluu left review comments

@TobionTobionTobion left review comments

@kevin-verschaevekevin-verschaevekevin-verschaeve left review comments

@vudaltsovvudaltsovvudaltsov left review comments

@onEXHoviaonEXHoviaonEXHovia left review comments

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.

14 participants

@yceruto@Simperfit@nicolas-grekas@stof@fabpot@driesvints@Tobion@javiereguiluz@Taluu@derrabus@kevin-verschaeve@vudaltsov@onEXHovia@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp