Skip to content

[ErrorHandler] Rework fatal errors #33053

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
Oct 11, 2019
Merged

[ErrorHandler] Rework fatal errors #33053

merged 1 commit into from
Oct 11, 2019

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Aug 8, 2019 •
edited
Loading

Q A
Branch? 4.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #32605
License MIT
Doc PR -

Built on top of #33038 so review only the second commit : d5c3f7e

The goals of this PR is to replace current "fatal error handlers" with "error enhancers" since all our current fatal error handlers works on \Error since PHP7.

That means we won't use the FatalErrorException anymore, so we will be able to remove it (once we don't need it in the rest of the codebase).

The final goal btw is to handle \Throwable everywhere in the code so we can remove FatalThrowableError & FatalErrorException classes.

@fancyweb fancyweb force-pushed the error-handler-rework-fatal-error branch from 0a610ae to fa9c9cb Compare August 8, 2019 13:34
@nicolas-grekas nicolas-grekas mentioned this pull request Aug 8, 2019
Closed
5 tasks
@yceruto yceruto added this to the next milestone Aug 8, 2019
@fancyweb fancyweb force-pushed the error-handler-rework-fatal-error branch from fa9c9cb to 5eb1bc5 Compare August 9, 2019 06:56
@yceruto
Copy link
Member

yceruto commented Aug 9, 2019

(rebase needed)

@fancyweb
Copy link
Contributor Author

fancyweb commented Aug 9, 2019

Impossible to resolve conflicts, because of 1st commit.

@fancyweb fancyweb force-pushed the error-handler-rework-fatal-error branch from 5eb1bc5 to b44845f Compare August 12, 2019 08:58
@fancyweb fancyweb force-pushed the error-handler-rework-fatal-error branch 2 times, most recently from d8cc931 to b3c53c4 Compare September 4, 2019 15:24
@yceruto
Copy link
Member

yceruto commented Sep 4, 2019

@fancyweb Thanks a lot for this PR, great job so far!

There are still some pending failures, let me know when this is ready for you and I will take a deeper look.

@yceruto yceruto self-requested a review September 4, 2019 16:30
@fancyweb fancyweb force-pushed the error-handler-rework-fatal-error branch 3 times, most recently from 468e40a to 62f60ef Compare September 10, 2019 12:08
stof
stof reviewed Sep 10, 2019
View reviewed changes
src/Symfony/Component/ErrorHandler/ErrorEnhancer/ClassNotFoundErrorEnhancer.php Outdated
}
}

if ($function[0] instanceof ComposerClassLoader || $function[0] instanceof SymfonyClassLoader) {
Copy link
Member

Choose a reason for hiding this comment

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

SymfonyClassLoader is a dead component in 4.x. Do we still need to account for it in the new ErrorHandler component ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked about this with @nicolas-grekas a while ago, and he told me we can eventually stop supporting it on 5.0. But I didn't do it yet.

@fancyweb fancyweb force-pushed the error-handler-rework-fatal-error branch 2 times, most recently from 764e6ac to d5c3f7e Compare September 10, 2019 13:56
yceruto
yceruto approved these changes Sep 10, 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

@fabpot
Copy link
Member

fabpot commented Sep 27, 2019

#33038 has been merged now. @fancyweb Can you rebase this one?

@fancyweb fancyweb force-pushed the error-handler-rework-fatal-error branch 5 times, most recently from 9d4bf35 to 56295e0 Compare September 27, 2019 08:59
@fancyweb
Copy link
Contributor Author

@fabpot Rebase is done and CI is green.

@fancyweb fancyweb force-pushed the error-handler-rework-fatal-error branch 3 times, most recently from 0dd5e9f to a4c93a6 Compare September 27, 2019 14:31
@fancyweb
Copy link
Contributor Author

Just repushed to take comments into account:

  1. Removed FatalErrorHandlerInterface, everything is done by ErrorEnhancerInterface.
  2. Removed FatalErrorInterface and everything related that was only necessary to know if a class not found error was "fatal" or not. We handle it but end user just can't know if the error he gets was fatal or not (well not easily).
  3. Removed FatalErrorException and FatalThrowableError in favor of ErrorException.

@fancyweb fancyweb force-pushed the error-handler-rework-fatal-error branch from a4c93a6 to aaa0cdf Compare September 30, 2019 12:01
@Tobion
Copy link
Contributor

Tobion commented Oct 11, 2019

Thanks @fancyweb for working on this feature, this is much appreciated.

Tobion added a commit that referenced this pull request Oct 11, 2019
@Tobion
feature #33053 [ErrorHandler] Rework fatal errors (fancyweb)
38b9a27 
This PR was merged into the 4.4 branch.

Discussion
----------

[ErrorHandler] Rework fatal errors

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32605
| License       | MIT
| Doc PR        | -

Built on top of #33038 so review only the second commit : d5c3f7e

The goals of this PR is to replace current "fatal error handlers" with "error enhancers" since all our current fatal error handlers works on \Error since PHP7.

That means we won't use the FatalErrorException anymore, so we will be able to remove it (once we don't need it in the rest of the codebase).

The final goal btw is to handle \Throwable everywhere in the code so we can remove FatalThrowableError & FatalErrorException classes.

Commits
-------

aaa0cdf [ErrorHandler] Rework fatal error handlers
@Tobion Tobion merged commit aaa0cdf into symfony:4.4 Oct 11, 2019
@fancyweb fancyweb deleted the error-handler-rework-fatal-error branch October 11, 2019 14:32
yceruto
yceruto reviewed Oct 23, 2019
View reviewed changes
src/Symfony/Component/ErrorHandler/ErrorHandler.php
use Symfony\Component\ErrorHandler\FatalErrorHandler\UndefinedFunctionFatalErrorHandler;
use Symfony\Component\ErrorHandler\FatalErrorHandler\UndefinedMethodFatalErrorHandler;
use Symfony\Component\ErrorRenderer\ErrorRenderer\HtmlErrorRenderer;
use Symfony\Component\ErrorRenderer\Exception\FlattenException;
Copy link
Member

Choose a reason for hiding this comment

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

Unintentionally deleted, fixed in #34075

@yceruto yceruto mentioned this pull request Oct 23, 2019
Merged
chalasr added a commit that referenced this pull request Oct 23, 2019
@chalasr
minor #34075 [ErrorHandler] Import missing classes (yceruto)
48282ce 
This PR was merged into the 4.4 branch.

Discussion
----------

[ErrorHandler] Import missing classes

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

Fix #33053 (comment)

Commits
-------

e07a3d7 Import missing classes
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
@fancyweb fancyweb mentioned this pull request Nov 7, 2019
Merged
nicolas-grekas added a commit that referenced this pull request Nov 8, 2019
@nicolas-grekas
minor #34284 [ErrorHandler] Remove Symfony ClassLoader support in an …
0628892 
…error enhancer (fancyweb)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[ErrorHandler] Remove Symfony ClassLoader support in an error enhancer

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #33053 (comment)
| License       | MIT
| Doc PR        | -

Throwing a deprecation for this on 4.4 looks useless to me as the component was deprecated 2 years ago and stopping its support will not break anything.

Commits
-------

e745654 [ErrorHandler] Remove Symfony ClassLoader support in an error enhancer
This was referenced Nov 12, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@yceruto yceruto yceruto approved these changes

@Tobion Tobion Tobion approved these changes

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
@fancyweb @yceruto @fabpot @Tobion @nicolas-grekas @stof @carsonbot