Show better exception messages when occurring in admin

[Pierstoval]

Related to #246

Here is the "new" exception layout in non-debug mode:

And here is what it resembles when in debug mode (we add the backtrace, that's all):

I was mostly inspired by the good error pages from the TwigBundle , so don't mind if it looks similar :)

Before moving on something else I'd like to have some opinions about it :)

Ping @javiereguiluz @ogizanagi @fadoe

[ogizanagi]

Actually, I might have missed the point. Can you explain what does this PR brings to easyadmin instead of having the default ExceptionListener ? :confused: (I tried it, and I'm only loosing informations in debug mode :/ )

[Pierstoval]

Actually, the goal is to avoid having the dirty "Error 500" page when in prod. There's still plenty of work to do with this feature, this PR is just a base setup to see what we truely need and what we can do for it :)

Maybe we can disable this listener while in debug mode ?

[ogizanagi]

@Pierstoval : Why providing another page in debug mode then ? Why not keeping the default one handled by HttpKernel and the TwigBundle ? You don't need to duplicate those features, right ? :confused:

Regarding the production error page, this should be configured by the developer in his application. Is the aim of this PR to provide a standalone error page in the EasyAdminBundle for production in order to have nothing more than installing easyadminbundle to have a fully workable production backend ?

[Pierstoval]

Regarding the production error page, this should be configured by the developer in his application. Is the aim of this PR to provide a standalone error page in the EasyAdminBundle for production ?

Yeah, it's meant to replace the ugly non-styled "Error 500" page in prod. Plus, if we provide a template for this, we might then add it to config for the user to override it in a very simple way, much better than overriding TwigBundle's one.

[ogizanagi]

@Pierstoval : Ok. Thanks for clarifying this :smiley: I'm not sure I personally agree with this feature, but anyway, IMO, the debug page should not change. Everything is handled perfectly with the HttpKernel Exception listener and the TwigBundle. There is no need to duplicate this logic.

[Pierstoval]

I truely understand, but sometimes you need something better to check in prod what happened. Anyway, you can read the #246 issue to know why I came up to this ;)

[ogizanagi]

You might have planned this, but :

• If the exception is an HttpException, you should display the status code and status text instead of the Exception code only.
• If it isn't an HttpException, you should transform it and display it as a 500.
• On the production page, you should display anything else than the status code and text + anything that could help the user (not the developper, so displaying any exception is wrong) to understand what happened.

I'll see and try your next suggestions before making my opinion. ^^

[javiereguiluz]

Thanks @Pierstoval for working on this. I like the overall idea. I just want to make two minor changes, but first I'll wait for you to solve the issue pointed by @ogizanagi in https://github.com/javiereguiluz/EasyAdminBundle/pull/313#discussion_r30503507 Thanks!

[javiereguiluz]

I've reviewed this PR and I agree with @ogizanagi: I think it's better to not change the exception page for the dev environment.

Regarding the implementation, instead of using a listener, do you think it could be possible to just wrap the entire index() method with a try..catch block, catch any exception and render the new error page if necessary?

[ogizanagi]

Regarding the implementation, instead of using a listener, do you think it could be possible to just wrap the entire index() method with a try..catch block, catch any exception and render the new error page if necessary?

:+1: Also, it will solves https://github.com/javiereguiluz/EasyAdminBundle/pull/313#discussion_r30503507

[Pierstoval]

@javiereguiluz I don't think that wrapping the indexAction method may be good, because overriden actions or custom actions might not handle exceptions the same way, which sounds wrong to me.

I'm gonna work on this tomorrow and provide handling for custom controllers and only in non-debug environment (I prefer relying on the kernel.debug param instead of kernel.environment for compatibility reasons).

[ogizanagi]

I prefer relying on the kernel.debug param instead of kernel.environment for compatibility reasons

That's indeed the best choice IMO. @javiereguiluz , @Pierstoval : What do you think about making this listener disableable (enabled by default) through easyadmin config ? One might prefer keeping the standard error templates they define for the whole application with twig error templating overriding :innocent:

I don't think that wrapping the indexAction method may be good, because overriden actions or custom actions might not handle exceptions the same way, which sounds wrong to me.

I don't see the issue ? Your listener will catch and render the exception with the new error template for any exception, right ? And it is already catching it for any easyadmin actions, even custom (except route based ones). This behavior will not differ from the try/catch in indexAction implementation or am I wrong ? :/

[ogizanagi]

By the way, what would be the solution for route based custom actions ? It feels strange that method based actions are handled by this, and not route ones ?

[Pierstoval]

By the way, what would be the solution for route based custom actions ? It feels strange that method based actions are handled by this, and not route ones ?

Whatever we can do, route based actions would be handled only if they execute a controller that extends the AdminController. We have no way to check that a route is behind EasyAdmin, because it may only use the layout, which we cannot detect in the ExceptionListener.

I don't see the issue ? Your listener will catch and render the exception with the new error template for any exception, right ? And it is already catching it for any easyadmin actions, even custom (except route based ones). This behavior will not differ from the try/catch in indexAction implementation or am I wrong ? :/

It will work only if you execute the parent::indexAction() method in the child controller, but in some very complex cases, one might not want to execute the default indexAction, or copy/paste the original behavior to add some custom logic. And then, he may change the way the controller handles the exception, which is not the goal of this PR. The best solution is, indeed, to handle the exception in a kernel.exception listener.

What's good with it is that, as it's a simple service, we can manage it by proposing to disable it in the configuration, and the best for me, as said in above comments, is to add the exception templates in the configuration to add them to the template customization system implemented in v1.4 , which will allow users to add custom details in the different templates. I'm also thinking about template customization with a per-exception-class system, like easy_admin/errors/RuntimeException.html.twig for instance.

[ogizanagi]

Whatever we can do, route based actions would be handled only if they execute a controller that extends the AdminController. We have no way to check that a route is behind EasyAdmin, because it may only use the layout, which we cannot detect in the ExceptionListener.

Actually, you could retrieve the route name from the request. But it will (slightly) complicate things if we need to retrieve and check against the list of routes used by easyadmin and custom actions.

It will work only if you execute the parent::indexAction() method in the child controller...

Indeed, you were right. :)

[Pierstoval]

Let's wait for tomorrow until I can work on this :wink:

[javiereguiluz]

add the exception templates in the configuration to add them to the template customization system implemented in v1.4 , which will allow users to add custom details in the different templates.

Please don't do that. Allowing to use a different exception template for each entity is too much overengineering. I don't think there is a reasonable use case for doing that.

[Pierstoval]

Ok no problem for that :)

[Pierstoval]

Here we are, made some updates : allowed to disable the exception listener with the exception_listener.enabled EasyAdmin config parameter. Also search in the resolved controller whether it extends the EasyAdmin controller.

Just need to change the status code if the exception is an HttpException

[javiereguiluz]

I've dedicated some time to this PR and I still don't understand it. The original feature request was: "if some Doctrine error happens, please don't render a 404 error page and display an appropriate error message".

But this PR creates a full-featured exception listener that captures all exceptions and recreates Symfony's exception pages. I think that this is too much. Moreover, the desired behavior is the following:

• In dev environment, do nothing. Symfony will take care of everything.
• In prod environment, display a beautiful and useful error page, like we already do for some errors.

@Pierstoval I'm really sorry but I need to close this PR without merging it :cry:

[Pierstoval]

Actually, as said, I don't want to rely on dev nor prod, but more on kernel.debug, because a staging/integration environment may also need to show proper error messages.

If you want, I can simplify this PR, but it's "harder" to catch Doctrine-specific exceptions than catch all exceptions and show their message. Doing Doctrine-specific catch would be too specific. As it's an open-source project, we must do our best to allow maximum flexibility and standardization.

We need some debate, but I still think this feature can be useful :smiley: :+1:

[javiereguiluz]

In #368 I'm trying to fix the original issue reported in #246.