Make it possible to register fatal error listeners separately from the error listeners

[ste93cry]

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
License	MIT

This PR should implement the feature requested in #751, which basically is making possible to register the listeners that will handle the fatal errors separately from the ones that will handle the errors. This also means that even though the handler will always be registered regardless of what it catches (exceptions, errors or fatal errors) the user can unset the corresponding integration and so the client will not capture anything for that case. I also refactored all the tests of the error handler by converting them to PHPT to get one step closer to not register it even once in the unit tests. The problem I found out while working on this is that such feature is a BC because we should not call both the error and fatal error listeners while handling a fatal error or we will capture it twice, but at the same time if (like the code does at the time of writing this) we do not call the error listeners then someone that has registered a custom listener will not receive anymore the fatals. I still have to find a way to workaround this, so this PR is in draft for this reason. Feel free to review it by the way

EDIT: I found a way to not (I hope) break the compatibility so that this feature can be merged into the next minor release. PR is ready for review, please kindly reason about what I did to not break the BC and if it makes sense in all cases

[Jean85]

I do like very much how you handled BC and the deprecation. I still have to review thoroughly the tests.

[ste93cry]

Unfortunately the deprecation won't be catched by the error handler because at that point the client is not already binded to the hub 😞 We should think about moving the deprecation somewhere else (I've thrown it in the constructor because it's the earliest point where you are starting using a deprecated feature) or just leave it as is and hope that the user sees it in the logs rather than in Sentry

[ste93cry]

I've fixed the build, now it's passing even with XDebug enabled (e.g. for code coverage). The problem was that PHPUnit to run the tests with code coverage enabled creates PHP files on the file and then require them after running a bunch of custom code, so the stacktrace is different. I really liked the fact that before I was able to test that even the stacktrace was what I expected, but unfortunately without using regexes it's not possible to differentiate between tests running with code coverage or not so I simply decided to not match strictly that part of the output

[Jean85]

Looks great! I would just change a small thing in the deprecation handling.

[mfb]

I tested this out and for some reason, I'm not seeing useful error reporting for fatal errors - Sentry doesn't show me where in the code the error happened.

For example, for an out-of-memory fatal error, in the list of issues, I see

Sentry\Exception\FatalErrorException [internal] in ?
in [internal]
Error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 134217736 bytes)

and when I click on the issue:
[internal]
No additional details are available for this frame.

However, I'm not sure this bug is related to this branch; it also seems to happen on master branch. The only difference on master branch is that it says ErrorException rather than Sentry\Exception\FatalErrorException

[mfb]

It looks like the stacktrace is not being handled correctly - it's showing up with function = Sentry\ErrorHandler::handleFatalError but the function of the next frame should be used, given how PHP stacktraces work - see https://github.com/getsentry/sentry-php/blob/1.x/lib/Raven/Stacktrace.php#L30

And then it looks like if the function starts with 'Sentry\' the frame has setIsInApp(false)

[Jean85]

And then it looks like if the function starts with 'Sentry' the frame has setIsInApp(false)

This is due to #786

[mfb]

I see there is a method called cleanBacktraceFromErrorHandlerFrames() so maybe this is needed for fatal errors.

[mfb]

When I register just the FatalErrorListener, I have the exact problem I'm trying to avoid: registerOnce() is calling set_error_handler() which is injecting Sentry into the stacktraces being processed by the app.

It seems like calling the addListener methods should call the appropriate registerOnce method, not the generic registerOnce() method?

[ste93cry]

It seems like calling the addListener methods should call the appropriate registerOnce method, not the generic registerOnce() method?

Indeed it should, but we cannot change this in the existing integrations (ErrorHandlerIntegration and ExceptionHandlerIntegration) because it would be a BC. For the newer FatalErrorHandlerIntegration there are no problems instead since it didn't exist before this PR. You can however create your own integrations based on the code of the first two and use them in place of the default ones to avoid calling the registerOnce method

[mfb]

ok, looking better :) I wonder if some users will be confused by PHP fatal errors being recorded as "Sentry\Exception\FatalErrorException" - implying it's an error related to Sentry itself. Maybe needs some documentation.

[ste93cry]

I wouldn't take actions on this without any evidence that it's a problem for users. I'm not sure instead of the approach I took to rework the API of the error handler, essentially creating new non-static functions to add the listeners and stopping auto registering implicitly the handler, so if you have any opinion to share it would be great

[mfb]

@ste93cry I was thinking that a changelog note about the new type of exception could be helpful.

[ste93cry]

Done! Did you have the chance to test out if this change solves your use case or at least helps you in solving it?

[mfb]

It seems to work great as far as the use case of only enabling the fatal error handler for apps that already have an error handler and exception handler.

I didn't try to enable the exception handler without the error handler, as I guess I'd have to create an integration, and it's lower priority, but fingers crossed that works too.

[HazAT]

Great change, makes it overall more configurable.
On thing that would be awesome is before merging open a PR in the Docs adding this new integration
https://github.com/getsentry/sentry-docs/

[stayallive]

This all looks really sensible to me 👍