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

[ErrorRenderer] Refactor and delegate serializing responsibility to the Serializer component #34288

Closed
wants to merge 1 commit into from

Conversation

@yceruto
Copy link
Member

yceruto commented Nov 8, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR TODO

By default, the ErrorRenderer component will host only the HTML+JS error page, delegating the serialization of other formats to the Serializer component (if installed & serializer config is enabled). This will simplify the usage and integration with other third-party packages like API-Platform and FOSRestBundle.

Main changes:

Related comment:

wdyt?

@yceruto yceruto added this to the 4.4 milestone Nov 8, 2019
@yceruto yceruto requested a review from dunglas as a code owner Nov 8, 2019
@yceruto yceruto force-pushed the yceruto:error_renderer_serializer branch 3 times, most recently from 4d3c31d to 99f2631 Nov 8, 2019
@@ -18,7 +18,6 @@ Console
Debug
-----

* Deprecated the `Debug` class, use the one from the `ErrorRenderer` component instead

This comment has been minimized.

Copy link
@yceruto

yceruto Nov 8, 2019

Author Member

[duplicated] the whole component is deprecated (mentioned two lines bellow)

@@ -57,7 +57,6 @@ Console
Debug
-----

* Removed the `Debug` class, use the one from the `ErrorRenderer` component instead

This comment has been minimized.

Copy link
@yceruto

yceruto Nov 8, 2019

Author Member

same here

@@ -26,14 +26,14 @@
*/
class FlattenException extends LegacyFlattenException
{
private $title;

This comment has been minimized.

Copy link
@yceruto

yceruto Nov 8, 2019

Author Member

minor refactor for better name (it was introduced in 4.4)

@yceruto yceruto force-pushed the yceruto:error_renderer_serializer branch 2 times, most recently from 1ae38b6 to a13b9de Nov 8, 2019
@yceruto yceruto force-pushed the yceruto:error_renderer_serializer branch 3 times, most recently from 5bfb7b6 to 05f4b08 Nov 8, 2019
@yceruto

This comment has been minimized.

Copy link
Member Author

yceruto commented Nov 8, 2019

Ready for review!

(Travis dep=high failures is normal)

Copy link
Member

fabpot left a comment

Great work!

public function normalize($exception, $format = null, array $context = [])
{
if (!$exception instanceof FlattenException) {
$exception = FlattenException::createFromThrowable($exception);

This comment has been minimized.

Copy link
@fabpot

fabpot Nov 9, 2019

Member

What if ErrorRenderer is not installed?

}
}
```

Configure your rendering service tagging it with `error_renderer.renderer`.

This comment has been minimized.

Copy link
@chalasr
@yceruto

This comment has been minimized.

Copy link
Member Author

yceruto commented Nov 10, 2019

closing in favor of #34312

@yceruto yceruto closed this Nov 10, 2019
@yceruto yceruto deleted the yceruto:error_renderer_serializer branch Nov 10, 2019
fabpot added a commit that referenced this pull request Nov 12, 2019
…onent (nicolas-grekas, yceruto)

This PR was merged into the 4.4 branch.

Discussion
----------

[ErrorHandler] merge and remove the ErrorRenderer component

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

This PR supersedes #34288.

Here is what it does:
- Merge the `ErrorRenderer` component into `ErrorHandler`
- Add `ErrorRendererInterface::render(\Throwable $e): FlattenException` and refactor error renderers around it.
- Add `FlattenException::setAsString()` to make the previous possible.
- Add `CliErrorRenderer` to render error on the CLI too. This means `VarDumper` is now a required dependency of `ErrorHandler`. This paves the way to use it also for rendering HTML - the logic there is much more advanced than what `HtmlErrorRenderer` provides and ever should provide.
- Make `BufferingLogger` map its collected logs to `error_log()` if they are not emptied before.
- Remove some classes that are not needed anymore (`ErrorRenderer`, `ErrorRendererPass`, `HtmlErrorRendererInterface`)
- Simplified the logic in `Debug::enable()` - nobody uses its arguments
- Fix a few issues found meanwhile.

With these changes, the component can be used standalone. One is now able to require only it, register it either with either `ErrorHandler::register()` or `Debug::enable()` and profit.

Commits
-------

d1bf1ca [ErrorHandler] help finish the PR
6c9157b [ErrorHandler] merge and remove the ErrorRenderer component
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.