Skip to content

Fix 12273 Added new ErrorController #12687

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 9, 2019
Merged

Fix 12273 Added new ErrorController #12687

merged 1 commit into from
Dec 9, 2019

Conversation

dbrekelmans
Copy link
Contributor

@dbrekelmans dbrekelmans commented Nov 24, 2019 •
edited
Loading

Partially fixes #12273

The Overriding the Default ErrorController paragraph in controller/error_pages.rst still needs to be updated. I struggled with this so I would really appreciate if someone else could step in and finish that paragraph! For further reference, see #12273 (comment)

@wouterj wouterj changed the title Fix 12273 Fix 12273 Added new ErrorController Nov 24, 2019
wouterj
wouterj requested changes Nov 24, 2019
View reviewed changes
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Thanks a lot for trying to find out how this feature works! I don't know much about the error handling internals, so I hope someone else can help us with that.

yceruto
yceruto suggested changes Nov 25, 2019
View reviewed changes
Copy link
Member

@yceruto yceruto left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@dbrekelmans thank you very much for this contribution! It looks almost fine to me, we just need to change a few things to be more accurate.

We should also replace Component\ErrorRenderer by Component\ErrorHandler everywhere. Thanks!

@yceruto
Copy link
Member

yceruto commented Nov 25, 2019 •
edited
Loading

All my previous comments explain how things work, but please, let's focus this document on how to achieve things rather than explaining how they work. Thanks!

nusje2000
nusje2000 suggested changes Nov 25, 2019
View reviewed changes
@yceruto yceruto mentioned this pull request Nov 25, 2019
Closed
@dbrekelmans
Copy link
Contributor Author

We should also replace Component\ErrorRenderer by Component\ErrorHandler everywhere. Thanks!

Thanks to this comment I also noticed there still exists an error_renderer.rst. Since the component was merged into ErrorHandler, I suppose the docs should too.

For me it's a bit too in-depth and time consuming to consider that within the scope of this PR. I do however think that this should be documented in an issue so it's not forgotten.

@yceruto
Copy link
Member

yceruto commented Nov 25, 2019

We should also replace Component\ErrorRenderer by Component\ErrorHandler everywhere. Thanks!

Thanks to this comment I also noticed there still exists an error_renderer.rst. Since the component was merged into ErrorHandler, I suppose the docs should too.

Agreed! I meant in this page only.

@javiereguiluz javiereguiluz mentioned this pull request Nov 26, 2019
Closed
@yceruto yceruto mentioned this pull request Nov 26, 2019
Closed
@dbrekelmans
Copy link
Contributor Author

@yceruto I resolved all PR comments. I'm not sure why the linter is complaining about a .. code-block:: php though as I've just copied how it's used elsewhere and it looks good on https://pr-12687-pvgcroy-nq3xa5jtywvyc.eu.s5y.io/controller/error_pages.html#overriding-error-output-for-non-html-formats.

@dbrekelmans
Copy link
Contributor Author

Hi @wouterj, it still says you have requested changes. Could you confirm I resolved your feedback?

@yceruto
Copy link
Member

yceruto commented Dec 4, 2019

Hi @dbrekelmans I'll take a look again tomorrow, thank you for your patience.

yceruto
yceruto approved these changes Dec 4, 2019
View reviewed changes
Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

LGTM!

@dbrekelmans dbrekelmans requested a review from wouterj December 6, 2019 09:45
@javiereguiluz javiereguiluz added this to the 4.4 milestone Dec 9, 2019
javiereguiluz added a commit that referenced this pull request Dec 9, 2019
@javiereguiluz
minor #12687 Fix 12273 Added new ErrorController (dbrekelmans)
891b677 
This PR was squashed before being merged into the 4.4 branch (closes #12687).

Discussion
----------

Fix 12273 Added new ErrorController

Partially fixes #12273

The `Overriding the Default ErrorController` paragraph in `controller/error_pages.rst` still needs to be updated. I struggled with this so I would really appreciate if someone else could step in and finish that paragraph! For further reference, see #12273 (comment)

Commits
-------

4c867f9 Fix 12273 Added new ErrorController
@javiereguiluz javiereguiluz merged commit 4c867f9 into symfony:4.4 Dec 9, 2019
@javiereguiluz
Copy link
Member

Daniël, thanks a lot for this nice and big contribution! We made some minor tweaks while merging, but your pull request was great. Congrats on your first Symfony Docs contribution!

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

@OskarStark OskarStark OskarStark left review comments

@nusje2000 nusje2000 nusje2000 requested changes

@yceruto yceruto yceruto approved these changes

@wouterj wouterj Awaiting requested review from wouterj

Assignees
No one assigned
Labels
ErrorHandler Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
7 participants
@dbrekelmans @yceruto @javiereguiluz @wouterj @OskarStark @nusje2000 @carsonbot