Skip to content

Custom ExceptionController using extended class #5757

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

Closed
wants to merge 6 commits into from
Closed

Custom ExceptionController using extended class #5757

wants to merge 6 commits into from

Conversation

iagomls
Copy link

@iagomls iagomls commented Oct 5, 2015

Q A
Doc fix? no
New docs? yes
Applies to all
Fixed tickets no

Last week, i implemented a custom ExceptionController extending from the default class. I didn't find the complete information in this doc, so i made a change explaining a little better how it should be done.

@timglabisch
Copy link
Contributor

👍

and set the arguments to load the specific needed services::

# app/config/services.yml
appbundle.twig.controller.exception:
Copy link
Member

Choose a reason for hiding this comment

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

Should this service be called app.twig.exception_controller instead?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, af089f0.

@javiereguiluz
Copy link
Member

@iagomelanias thanks for proposing this improvement. We really like when people fill in the existing gaps in the documentation.

I've left a few comments and proposed fixes. Don't worry because your proposal is right. We just need to tweak it a bit to better fit the Symfony docs. Thanks!

@iagomls
Copy link
Author

iagomls commented Oct 24, 2015

Thank you @javiereguiluz. No problem, i have changed the commented lines. Could you review it again?

I hope i can contribute with the community more times, it's amazing to help a little bit the job you're doing. Thanks!

Instead of creating a new exception controller from scratch you can, of course,
also extend the default :class:`Symfony\\Bundle\\TwigBundle\\Controller\\ExceptionController`.
In that case, you might want to override one or both of the ``showAction()`` and
``findTemplate()`` methods. The latter one locates the template to be used.

To create your own controller logic extending from the ExceptionController, simply
Copy link
Member

Choose a reason for hiding this comment

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

the class name should be enclosed by double backticks

@xabbuh
Copy link
Member

xabbuh commented May 21, 2016

@iagomelanias Please apologise the long delay in reviewing your pull request. I have left some minor comments. But we can also address them while merging if you are not able to address them in the meantime.

@wouterj
Copy link
Member

wouterj commented May 21, 2016

Although I know we documented it this way, I can't see why we extend the base ExceptionController here. It seem to only cause confusion, as we suddently have to deal with controllers as services.

So while I like your contribution, I think we should rewrite it a bit:

  • Let's not extend the base ExceptionController from the TwigBundle, but use the main Controller class as base instead
  • Then remove the controllers as services stuff
  • Use AppBundle:Exception:show as exception controller setting value

This shows exactly what's needed for a custom exception controller: A normal controller action that is designed to show an error message.

Can you maybe make this change? (I would very much like that, as you'll get full credits for your work) If you can't, please say so and we can take this over.

@weaverryan
Copy link
Member

Closing this. See my comment here: #4181 (comment)

Unless someone can tell me otherwise, I don't even think that overriding the error controller should be something we recommend. Everything can be done in an event subscriber. I'd rather show just one, clean way of doing things :).

Thanks for the PR!

@weaverryan weaverryan closed this Jul 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants