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

Error handler as singleton and with listeners #762

Merged
merged 24 commits into from
Feb 7, 2019

Conversation

Jean85
Copy link
Collaborator

@Jean85 Jean85 commented Feb 4, 2019

Sorry for opening a new PR, but this is a new approach to solve #746, different from #759.

In here, as discussed together, we use the integrations to attach two listeners to the error handler, moving away the issue of error level filtering from that class.

I like this approach very much:

  • it's cleaner
  • tests are better and cleaner
  • the end user can use the listeners to attach custom reporting
  • the end user can even use the default integrations to decide which auto-reporting to enable (errors or exceptions)

@Jean85 Jean85 added this to the Release 2.0 milestone Feb 4, 2019
@Jean85 Jean85 self-assigned this Feb 4, 2019
@Jean85 Jean85 requested review from HazAT and ste93cry February 4, 2019 12:09
@Jean85 Jean85 changed the title [WIP] Error handler as singleton and with listeners Error handler as singleton and with listeners Feb 4, 2019
@HazAT
Copy link
Member

HazAT commented Feb 4, 2019

src/ErrorHandler.php Outdated Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
@HazAT
Copy link
Member

HazAT commented Feb 4, 2019

Otherwise, this PR really looks good, like the approach. I will close my PR :)
Close #759

UPGRADE-2.0.md Outdated Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
tests/Fixtures/classes/StubTransport.php Outdated Show resolved Hide resolved
tests/Fixtures/classes/StubTransport.php Outdated Show resolved Hide resolved
tests/phpt/send_only_all_except_notice.phpt Outdated Show resolved Hide resolved
tests/phpt/send_only_all_except_notice.phpt Show resolved Hide resolved
tests/phpt/send_only_all_except_notice.phpt Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
src/Integration/ErrorListenerIntegration.php Outdated Show resolved Hide resolved
src/ErrorHandler.php Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
@ste93cry
Copy link
Collaborator

ste93cry commented Feb 6, 2019

Can you please also add an entry to the CHANGELOG.md file?

src/ErrorHandler.php Outdated Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
tests/phpt/error_handler_refuses_negative_value.phpt Outdated Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
src/ErrorHandler.php Outdated Show resolved Hide resolved
tests/ErrorHandlerTest.php Show resolved Hide resolved
tests/ErrorHandlerTest.php Outdated Show resolved Hide resolved
tests/ErrorHandlerTest.php Outdated Show resolved Hide resolved
tests/ErrorHandlerTest.php Outdated Show resolved Hide resolved
tests/ErrorHandlerTest.php Outdated Show resolved Hide resolved
@ste93cry ste93cry merged commit 9028676 into master Feb 7, 2019
@ste93cry ste93cry deleted the error-handler-as-singleton branch February 7, 2019 10:08
Jean85 added a commit that referenced this pull request Feb 7, 2019
@Jean85 Jean85 mentioned this pull request Feb 7, 2019
@RobinHoutevelts
Copy link

Was just about to complain that \Sentry\ErrorHandler::handleError also called the previousExceptionHandler with the handmade \ErrorException.

I'm not sure this PR also fixes that but I'll check it out when I find the time.

Either way this is a nice improvement 👍

@Jean85
Copy link
Collaborator Author

Jean85 commented Feb 8, 2019

We completely changed the approach there, splitting in two separate listeners, so that should be fixed!

@ste93cry
Copy link
Collaborator

ste93cry commented Feb 8, 2019

It's not, @RobinHoutevelts is right in saying that we're still calling the $previousExceptionHandler in the handleException method regardless of the type of exception. We should investigate if this is an unintended behavior and in case fix it

@Jean85
Copy link
Collaborator Author

Jean85 commented Feb 8, 2019

But that's no longer a problem, because we're no longer invoking the handleException from handleError nor handleFatalError methods, thanks to the listeners.

I broke that (not ideal) chain with this PR.

@ste93cry
Copy link
Collaborator

ste93cry commented Feb 8, 2019

You're right, sorry for the mistake then

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

4 participants