Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Revert ignoring fatal errors on PHP 7+ #571

Merged
merged 1 commit into from
Mar 20, 2018

Conversation

Jean85
Copy link
Collaborator

@Jean85 Jean85 commented Mar 19, 2018

This PR attempts to fix #552 allowing all fatals to be captured, with no exception anymore. This is because further investigations lead to discover that the issues in double or loop reporting of fatals where due to a bug on Symfony: see the issue symfony/symfony#26438 and the fix symfony/symfony#26568.

Eventual double reporting should be not fixed here in the client, but on the native integrations, like in the Symfony Bundle.

This PR also adds a regression test with a PHPT test, which could be run with PHPUnit with no issues.

@Jean85 Jean85 self-assigned this Mar 19, 2018
@dcramer
Copy link
Member

dcramer commented Mar 19, 2018

fwiw the core client should handle double reporting when it can (I think it does/did?). No reason to re-write the same thing.

@Jean85
Copy link
Collaborator Author

Jean85 commented Mar 19, 2018

@dcramer that kind of stuff is requested in #563. The kind of duplication that I'm talking about here is different, is due to third party integrations, and the scenario is like:

  • a fatal error is thrown
  • Sentry captures it correctly through the ErrorHandler::handleError function
  • Sentry the passes the error to the previous error handler
  • The previous error handler is Symfony's, which rethrows errors as FatalThrowableError exceptions
  • Sentry's exception handler kicks in...

In this way the error is reported twice, once as an error, once as a FatalThrowableError. Since those are different, the core cannot handle it in itself, but a simple ignore_fatal_throwables option on the Symfony bundle could fix this easily.

@dcramer
Copy link
Member

dcramer commented Mar 19, 2018

@Jean85 ah nevermind then - agreed.

Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

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

👍

@Jean85 Jean85 merged commit 047f0d3 into master Mar 20, 2018
@Jean85 Jean85 deleted the fix-regression-on-fatal-reporting branch March 20, 2018 12:18
@Jean85
Copy link
Collaborator Author

Jean85 commented Mar 20, 2018

With this I think it's time to release 1.8.4... @stayallive what do you think? We need to update the changelog too!

@stayallive
Copy link
Collaborator

@mfb
Copy link
Contributor

mfb commented Mar 20, 2018

Another bug I'd suggest looking at re: fatal errors is #391 - I pushed some code that can fix the bug, or perhaps some other refactoring is desired

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Capturing E_ERROR fatal errors broken on PHP 7
4 participants