Overriding error handler changes symfony error handler

[simPod]

When using older version of Sentry (v2) with Symfony (v5.2) the handler in DebugHandlersListener::configure() is

$handler = {array} [2]
 0 = {Symfony\Component\ErrorHandler\ErrorHandler} [18]
 1 = "handleException"

in Sentry v3 the ExceptionListenerIntegration overrides exception handler in registerOnceExceptionHandler() via line

self::$handlerInstance->previousExceptionHandler = set_exception_handler(\Closure::fromCallable([self::$handlerInstance, 'handleException']));

therefore the handler is now of type Closure, not array

And the condition in Symfony's DebugHandlersListener is not satisfied

        $handler = set_exception_handler('var_dump');
        $handler = \is_array($handler) ? $handler[0] : null;
        restore_exception_handler();

$handler is now null.

That kind of significantly influences the rest of app behaviour.

TBH I don't know what is the proper solution to the problem. Is it intended? I wish to keep Symfony error handler as I hook into its events.

Setting $handler to null forces symfony/debug's ErrorHandler to be used instead and I don't want that, it does not use onKernelException() listeners for example.

(

  - Upgrading sentry/sentry (2.5.0 => 3.1.2): Extracting archive
  - Upgrading sentry/sdk (2.2.0 => 3.1.0)
  - Upgrading sentry/sentry-symfony (3.5.2 => dev-master f34ebf2): Extracting archive

)

[Jean85]

Not sure about this... @nicolas-grekas, could you help us out here? Does the Symfony error handler NOT support closures as handlers?

[nicolas-grekas]

I experienced the same issue on our Symfony apps.

There is nothing special to do to reproduce: the issue is that when Sentry\Client is instantiated, it mutates the global state by setting its own error/exception handler.

When running tests, this leads to a red warning that says THE ERROR HANDLER HAS CHANGED!, because the phpunit-bridge detects that the tests end with a different error handler than the one they started with.

This could be fixed by making Sentry not set its error handler when no DSN is defined. Do you think that's doable?

Then there is this in DebugHandlersListener: we read the error handler stack to get access to the current error handler. If we find Symfony's there, then we configure it. But since Sentry replaces it with its own, the listener does nothing, and the app is not fully configured.

Honestly, I don't get why Sentry would hook as an error handler: in the context of Symfony, we already have logs to report PHP warnings/etc. Can't this be removed?

I might also try to find another way to get access to our error handler so that we can still configure it when another piece of code replaces it.

[nicolas-grekas]

See symfony/symfony#39944 for the issue related to DebugHandlersListener.

For tests, it would be great if this bundle could provide a way to not instantiate Sentry\Client in the test env - either via explicit configuration or implicitly when the DSN is empty.

[Jean85]

Thanks a lot @nicolas-grekas for the fast PR on your side.

As for not injecting our handler, it's doable and we already discussed it in #337, but we didn't find the time to do it; it's a big change for us and it's a lot less "plug & play" for our users, since it would require writing down an handler inside the Monolog config, so it doesn't seems solvable with Flex either.

[nicolas-grekas]

I propose to fix this at the recipe level, see symfony/recipes-contrib#1080

[ureimers]

Even with the handler being skipped, if no DSN is provided, the custom error handler still interferes with our Symfony configuration:

• \Errors aren't logged by monolog any longer (Not even by Apache/PHP. This is especially tricky, because \Exceptions are logged. It seems that only PHP \Error's are affected. If we disable Sentry everything works fine)
• Symfony's framework.error_controller isn't invoked anymore (also only for \Errors, as it seems)

I thought, as a workaround, I could just use the Sentry PHP SDK and write a small Monolog-Handler to log to Sentry but as the SDK itself (Sentry\Client::__construct()) is the culprit here, right now the two options for us is to write a Sentry API-Interface ourselves or stop using Sentry at all.

At first I thought that sentry.register_error_listener: false would help out here, but it turns out that this does exactly what it states: Disable the listener, not the handler.

As much as I like Sentry, I don't understand why it needs to register its own error handler. Especially in the context of Symfony or other frameworks that support Monolog.

Maybe I don't grasp all the features and what they need to be working but I would think that an simple Monolog handler that I could put into my monolog chain would be enough to send my errors to Sentry.