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

Rework to use only listeners #204

Merged
merged 7 commits into from Mar 22, 2019
Merged

Rework to use only listeners #204

merged 7 commits into from Mar 22, 2019

Conversation

Jean85
Copy link
Collaborator

@Jean85 Jean85 commented Mar 19, 2019

While testing the beta1, I discovered that Sentry's error handler wasn't triggered, if not only on the most fatal errors. Any other intermediate approach lead to double-reporting of errors (like #156), so I have to fall back to using the framework events, and disable Sentry's error handler.

@Jean85 Jean85 added this to the 3.0 (Sentry client 2.0) milestone Mar 19, 2019
@Jean85 Jean85 self-assigned this Mar 19, 2019
@Jean85 Jean85 marked this pull request as ready for review March 20, 2019 08:29
@Jean85 Jean85 requested review from HazAT and stayallive March 20, 2019 08:29
@Jean85
Copy link
Collaborator Author

Jean85 commented Mar 22, 2019

Note to self: is getsentry/sentry-php#788 is merged/tagged, it will break this one. We need to wait that before releasing a stable version.

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.

Looks good to me 👍

@ste93cry
Copy link
Collaborator

ste93cry commented Mar 22, 2019

@Jean85 if we want to strictly follow semver then that PR will be merged only in sentry-php 2.1 which will probably not see the light in the near future. I suggest to just merge this PR and rework it later when it will be time. Also I should have implemented everything in a BC-compatible manner, so you shouldn't have the need of adapting your changes to mine... Can you explain to me what is going to break?

@Jean85
Copy link
Collaborator Author

Jean85 commented Mar 22, 2019

It would break in the fact that you will add a new handler, that I would have to remove to avoid double-reporting of fatal errors.

@Jean85 Jean85 merged commit 2845df5 into master Mar 22, 2019
@Jean85 Jean85 deleted the rework-to-use-only-listeners branch March 22, 2019 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants