Skip to content

[Proposal][WIP][TwigBundle] Extract error handling into separate ErrorPageBundle #22450

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

Conversation

sustmi
Copy link
Contributor

@sustmi sustmi commented Apr 15, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no?
Tests pass? yes
Fixed tickets -
License MIT
Doc PR symfony/symfony-docs#...
  • update CHANGELOG.md files
  • update UPGRADE.md files
  • update symfony/symfony-docs
  • fix composer.json in ErrorPageBundle (it was copy-pasted from TwigBundle)
  • fix composer.json in TwigBundle (it may contain dependencies that are no longer needed)
  • create new symfony/error-page-bundle package on Packagist
  • update symfony/symfony-standard to use the new bundle
  • add backward compatibility with deprecations?

I suggest moving error handling (ie. exception listener service, exception controller, error templates and error preview controller) from TwigBundle to new ErrorPageBundle.

Error handling was moved to TwigBundle from FrameworkBundle back in Symfony 2.0 (see 3749ad4) but in my opinion it does not belong there.

This change is BC-breaking so I guess it can be done only in next major release (Symfony 4.0).
What is the correct way to do such BC breaks. For example should there be a deprecated twig.exception_listener.controller service that is an alias pointing to the new error_page.exception_listener.controller service?
And how should I specify that

What do you think about this PR?

@sustmi sustmi force-pushed the extract-error-page-bundle branch from 91b394e to e803a54 Compare April 15, 2017 22:59
@sstok
Copy link
Contributor

sstok commented Apr 16, 2017

This bundle still requires the TwigBundle to able to render the template, so why should this be extracted to a separate bundle?

@sustmi
Copy link
Contributor Author

sustmi commented Apr 16, 2017

@sstok I think that the error page functionality simply does not belong into TwigBundle. If we separate the bundles, TwigBundle will remain with just one responsibility - to "glue" Twig into Symfony Framework. Similarly ErrorPageBundle will have one responsibility - to bring fancy customizable error pages into Symfony Framework.
Right now, TwigBundle has both of these responsibilities. I can imagine that someone wanted to use TwigBundle without the exception handling.

This bundle still requires the TwigBundle to able to render the template, so why should this be extracted to a separate bundle?

In my opinion, "X requires Y" relation does not imply "Y contains X" relation.

@fabpot
Copy link
Member

fabpot commented Apr 17, 2017

I don't see the point. What value does it bring?

@sustmi
Copy link
Contributor Author

sustmi commented Apr 18, 2017

Recently, PR #20951 that redesigned the exception page was merged. I noticed that the new design looks different from the exception page that Debug component provides. Also, the highlighted code in TwigBundle's error page uses different colour schema than for example WebProfilerBundle's _profiler_open_file action. Lastly, the debug output from VarDumper component uses another (dark) colour schema. I thought that it would be nice to make them all share the same design (and ideally not violate DRY).
However, it took me a while to figure out what is part of the ErrorPage functionality (eg. which resources are relevant) and what is the other stuff in TwigBundle. So I thought that I could save other people's time in future by extracting it to a separate bundle to make it clearer what is what.

TL;DR: The point is less confusing code. However I would understand if the necessary changes and BC breaks were not worth it. :|

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Apr 20, 2017
@nicolas-grekas
Copy link
Member

I don't see the need also, I'd be -1 here

@stof
Copy link
Member

stof commented Apr 20, 2017

The reason why the WebProfilerBundle duplicates the exception rendering instead of reusing the TwigBundle one directly is because we want to make the WebProfilerBundle UI available for Silex too for instance. So splitting ErrorPageBundle would not really allow sharing the code

@fabpot fabpot closed this Apr 20, 2017
@sustmi
Copy link
Contributor Author

sustmi commented Apr 21, 2017

Ok, I respect your opinions.
Just to clarify my intention: I wanted to make some component on which both ErrorPageBundle and WebProfilerBundle (and maybe DebugBundle) would be dependent. This new component would provide ways to implement the unified look.
The first step in doing this was to understand how all the different error page renderers work.
I had difficulties to understand which parts of TwigBundle are responsible for error pages and which not. Now that I discovered the details I thought that it may be nice of me to make it clear for others too by refactoring the code.
But I get that if the error pages generation code is touched very rarely it may not be worth refactoring it.

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