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

[TwigBundle] Deprecating error templates for non-html formats and using ErrorRenderer as fallback #31398

Open
wants to merge 11 commits into
base: 4.4
from

Conversation

Projects
None yet
7 participants
@yceruto
Copy link
Member

commented May 6, 2019

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 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.

@yceruto yceruto force-pushed the yceruto:twig_exception_controller branch 4 times, most recently from 752e607 to efaf91e May 6, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

Hi @OskarStark thanks for your review, but they aren't part of this PR (see description), the last commit should be reviewed for now.

Please, move them to #31065 to track them in the right place ;)

@yceruto yceruto force-pushed the yceruto:twig_exception_controller branch 2 times, most recently from dafcc21 to eea898f May 7, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

eea898f#diff-21899d8070b2bcedfb8ab7f6993331deR73
https://ci.appveyor.com/project/fabpot/symfony/builds/24407001#L1264

I have a problem with the ErrorHandler in functional tests, it does not work because the DeprecationErrorHandler is used instead, hence $kernel->terminateWithException() method is never called and we can't get the json response. Thoughts?

@yceruto yceruto force-pushed the yceruto:twig_exception_controller branch from eea898f to 61d2b9a May 8, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Status: Needs work

@nicolas-grekas nicolas-grekas added this to the next milestone May 9, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Check later if #31868 solve this issue #31398 (comment).

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@yceruto Yes, it should fix the issue. I found it while reviewing your PR and Nicolas kindly fixed it. It will soon be on all branches so that you can rebase and see if that works (I tested locally and it worked well).

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

@fabpot perfect!! thanks.

@yceruto yceruto force-pushed the yceruto:twig_exception_controller branch from 61d2b9a to c2f4fc2 Jun 5, 2019

@yceruto yceruto changed the base branch from master to 4.4 Jun 5, 2019

@yceruto yceruto force-pushed the yceruto:twig_exception_controller branch 4 times, most recently from 35755ef to 46f9183 Jun 5, 2019

Show resolved Hide resolved UPGRADE-4.4.md
Show resolved Hide resolved UPGRADE-4.4.md Outdated
@Tobion

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

LGTM when my comments are adressed. Let' do it.

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2019

@tabion comments addressed, thank you!

Show resolved Hide resolved UPGRADE-4.4.md Outdated
@Tobion

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

#31398 (comment) is still left.

@yceruto yceruto force-pushed the yceruto:twig_exception_controller branch 2 times, most recently from a8e488d to f1f4b2a Jul 15, 2019

@Tobion

Tobion approved these changes Jul 15, 2019

Copy link
Member

left a comment

Thanks for taking all my suggestions into account.

@yceruto yceruto force-pushed the yceruto:twig_exception_controller branch from cac6bc8 to ca046b2 Jul 18, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2019

Rebased and updated accordingly, this is ready.

'02-14' => self::GHOST_HEART,
'02-29' => self::GHOST_PLUS,
'10-18' => self::GHOST_GIFT,
];

This comment has been minimized.

Copy link
@fabpot

fabpot Jul 19, 2019

Member

This is a removed "feature", right? As the PR is huge, can you tell us which other "features" have been removed?

This comment has been minimized.

Copy link
@yceruto

yceruto Jul 19, 2019

Author Member

Opps! sorry, that wasn't intentional. (re-added).

I've moved all the features from exception_full.html.twig to here, in practice, this has more features than the current one, as it was more advanced.

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

Summary

  • Added new TwigHtmlErrorRenderer (replaces and wraps the HtmlErrorRenderer) allowing to load custom Twig-based HTML error pages in non-debug mode, otherwise it falls back on HtmlErrorRenderer.
  • The ErrorRenderer mechanism is not enabled by default when TwigBundle is installed, that's why we're deprecating the ExceptionController and the default value that enable it (twig.exception_controller) which would be null in 5.0, thus enabling the ErrorRenderer mechanism by default. (we should think about whether the twig.exception_controller config option is still relevant or not for other use cases, because in 5.0 it's useless for TwigBundle)
  • Deprecating the ExceptionController we would also be deprecating the support to load non-HTML error page (e.g. error.json.twig, error.xml.twig, etc). because the new ErrorRenderer mechanism is in charge of that (people will have to migrate here).
  • Last, the ExceptionController provides an advanced full exception page which is also being deprecated, in favor of the HtmlErrorRenderer which now contains a PHP version of it.

I know, that's too much changes for one PR, and still there are more in the next iteration:

  • Update WebProfilerBundle to decouple it from TwigBundle templates and better ErrorRenderer integration (already WIP)
  • Refactor the HtmlErrorRenderer templates (grouping of PHP templates and other improvements)

Let me know if a split of this PR is necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.