Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HttpKernel] Break up the ExceptionListener into two separated listeners #18552

Closed
devsdmf opened this issue Apr 14, 2016 · 3 comments
Closed

Comments

@devsdmf
Copy link

devsdmf commented Apr 14, 2016

Actually the Exception event listener from the HttpKernel component has two different responsibilities, first is to catch the exception and log it using the logger object injected in the constructor of the class, and second to generate an error response object to send to the user.

I tought to seperate theses two responsibilities into two different listeners because I need to my application log the exceptions but I need to show an error response in the JSON format, but if I catch the onKernelException event before the ExceptionListener I need to rewrite the logging method in my own Listener, otherwise if I set the priority of my listener to answer to the event after the ExceptionListener it won't work because that listener already setted an response object and stopped the event propagation.

PS: Write my own logging routine is not a good options because I need to check the default logging format of the Symfony framework or a separated log file just for this case and my log parser will need to deal with two different files.

@devsdmf devsdmf changed the title [HttpKernel] Break the ExceptionListener into two separated listeners [HttpKernel] Break up the ExceptionListener into two separated listeners Apr 14, 2016
kimlai added a commit to lrqdo/RestProblemBundle that referenced this issue May 11, 2016
The exception listener was not compatible with Symfony's exception
listener: they both set a Response, so they both stop the exception
event propagation.

With this change, the listener first stores a reference to the thrown
exception, then lets Symfony's exception listener log the exception and
rendering the error page (unfortunately this is done in the same
listener, an issue has been opened
[here](symfony/symfony#18552)). Then the
listener waits for a response event, checks the status code of the
response, and if it's an error, it's returns the RestProblem custom
response as it did before, thus keeping backward compatibility.
@kimlai
Copy link
Contributor

kimlai commented May 11, 2016

Hello @devsdmf, have you fixed your problem?

As I looked for a solution for the same problem I saw different ways of solving it, maybe it can help.

The easiest one is to make sure that your request's format is correctly set to 'json' (there are a couple of ways of achieving that, the easiest being Request::setRequestFormat($format)). If that's the case, twig will use a default json template to render the error in JSON. I suppose that you can customize this template the same way that you would for HTML error pages.

You can also override the exception controller defined in the exception listener if you need some custom logic.

One problem with those approaches is that the exception gets flattened, which might not be desirable (it removes the Exception::getTraceAsString() method that is quite nice for the dev environment). In this case you can use the FOSRestBundle approach, which is to extend the listener and pass the exception as is. I think in their case it's needed because since they don't use twig, the listener is not even active (which is a design smell IMHO, why would an app that's not using twig loose exception logging?).

The last (and dirtiest) option that I see is to create a custom listener that does two things

  • keeps a reference to the thrown exception when a kernel.exception event occurs.
  • overrides the response with whatever you want when a kernel.response event occurs if the reference is not null.

I still agree that breaking the listener into two separate ones looks like a better architecture, but I felt that sharing workarounds might be helpful.

@devsdmf
Copy link
Author

devsdmf commented May 18, 2016

Hello @kimlai, thank you for your answer, I'm working in another project now (Symfony 2.3) and I didn't solved that.

For the request format, I was using the JsonResponse object of the HttpFoundation component so I guess that it was not the problem.

About override the default exception controller, as I said, I didn't want to rewrite an identical routine just to take the problem in standby.

Anyway, as you said, separate theses concepts is a better architecture design and I guess that it would be implemented in future versions of Symfony. For now, I appreciate your help and tips, I will took note about it for the case that it happen again with me in the future.

@fabpot
Copy link
Member

fabpot commented Oct 10, 2018

Closing as it won't happen. Logging is just a side-effect of the listener, not a main feature. If you like to log, that's totally possible via a new listener for which you would inject the same logger as Symfony.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants