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 https://github.com/symfony/symfony/pull/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 https://github.com/symfony/recipes-contrib/pull/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.

[Jean85]

Using a simpler approach with a Monolog handler is already what we would like to do (since it was already suggested by Nicolas and tracked in #337), but it's doable only here in the context of a Symfony app and not in the base SDK (sentry/sentry) because without our error handler we cannot be sure to hook into all the uncaught exceptions and errors; Symfony does all of that with its own error handler, so that's why we would like to switch over.

As for disabling the error handler, it's currently enabled by the ErrorListenerIntegration, so if you override the list of integrations that you want to enable, you would effectively be able to not have it hooked. We temporarily tried to do it in 2.x of this bundle (see #204) but it backfired for other reasons since we didn't leverage Monolog as we're planning to do now.

[ste93cry]

• \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)

This sounds a lot like the issue described in https://github.com/getsentry/sentry-symfony/issues/421#issuecomment-765530528

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.

Indeed, but looking again at the code of version 3.5 I think that the integrations that register the handler weren't added to the client. @Jean85 do you have any grasp of it?

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.

Agree. It has always been this way I believe, and we missed the opportunity to change this in time for the release of version 4.0 unfortunately

[Jean85]

Indeed, but looking again at the code of version 3.5 I think that the integrations that register the handler weren't added to the client. @Jean85 do you have any grasp of it?

Yeah that seems the code from #204. This factory was used to remove them: https://github.com/getsentry/sentry-symfony/blob/3.5.x/src/DependencyInjection/IntegrationFilterFactory.php The error handler was still active though because it was needed to capture fatals or OOM IIRC.

[ureimers]

Hi @Jean85 and @ste93cry, first thank you both for your replies! This all sounds very promising. As we already migrated to 4.x I think we'll just live with the errors not being logged to the non-Sentry-logs and wait for the new Monolog-based approach.

It all seems to be very complicated and following your referenced pull requests or issues is like going down a rabbit hole, because they all lead to other PRs or issues (I stopped when I somehow reached the HttpPlug documentation and the different HTTP PSRs which, honestly, I don't want to bother with, but that's a whole other story). https://github.com/B-Galati/monolog-sentry-handler looked like a promising Monolog-based approach, but it doesn't support Sentry SDK 3 yet and is "only" a Monolog Handler without any integration (it has a documenation for a Symfony integration, though).

I'm a little flooded with information and right now, as I need to accept the fact that I don't have any free time, I think I'll just wait this out and don't try to solve it on my own. There seem to be a lot of bright people on this, so I don't think I would be of any help anyway.

[annuh]

• \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)

Is there maybe a work-around for this? 🙂 We are still using Sentry v2 and we would like to update our Sentry integration to Sentry v3 for ie. PHP8.1 support. However, not having all \Errors in our other logging/monitoring systems is a problem for us.

[ste93cry]

Since you're likely using Monolog in a Symfony app, to workaround the issue you can activate the Monolog handler we provide and then configure the integrations option to filter out the (FatalError|Error|Exception)ListenerIntegration integration

[mfb]

For reference, on the Drupal side of things, which also has its own error and exception handlers, I disable ExceptionListenerIntegration and ErrorListenerIntegration, but keep FatalErrorListenerIntegration (as catching fatals didn't seem to conflict)

[github-actions[bot]]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[simPod]

🦚

[jorrit]

Updating to sentry-symfony 4 caused problems for me which seem to be related to error handling, which is why I post it here:

My application is still on Symfony 4 with several components from the 5.x range. This gives several deprecation warnings on startup:

2022-07-27T12:43:49+02:00 [info] User Deprecated: Since symfony/property-access 5.2: Passing a boolean as the first argument to "Symfony\Component\PropertyAccess\PropertyAccessor::__construct()" is deprecated. Pass a combination of bitwise flags instead (i.e an integer).
2022-07-27T12:43:49+02:00 [info] User Deprecated: Since symfony/property-access 5.3: Passing a boolean as the second argument to "Symfony\Component\PropertyAccess\PropertyAccessor::__construct()" is deprecated. Pass a combination of bitwise flags instead (i.e an integer).
2022-07-27T12:43:49+02:00 [info] User Deprecated: Since symfony/property-access 5.3: Passing a boolean as the fourth argument to "Symfony\Component\PropertyAccess\PropertyAccessor::__construct()" is deprecated. Pass a combination of bitwise flags as the second argument instead (i.e an integer).
2022-07-27T12:43:49+02:00 [info] User Deprecated: The SwiftMailerHandler is deprecated since Monolog 2.6. Use SymfonyMailerHandler instead.
2022-07-27T12:43:49+02:00 [info] User Deprecated: Since symfony/validator 5.2: Passing an instance of "Doctrine\Common\Annotations\PsrCachedReader" as first argument to "Symfony\Component\Validator\ValidatorBuilder::enableAnnotationMapping" is deprecated. Pass true instead and call setDoctrineAnnotationReader() if you want to enable annotation mapping with Doctrine Annotations.
2022-07-27T12:43:49+02:00 [info] User Deprecated: The SwiftMailerHandler is deprecated since Monolog 2.6. Use SymfonyMailerHandler instead.

These warnings are caught before the error handling is fully set up, and they are stored in the $bootstrappingLogger logger of the Symfony\Component\ErrorHandler\ErrorHandler class. Without Sentry or with Sentry 2, these errors were passed to the default logger once it was set up. However, because Sentry sets itself as exception handler, DebugHandlersListener doesn't find it and doesn't executesetDefaultLogger. This causes the bootstrap errors to be logged to the browser screen in BufferingLogger::__destruct().

DebugHandlersListener is supposed to work around this by checking the exception handler in its constructor (see https://github.com/symfony/symfony/pull/39944), but this doesn't work on my installation as Sentry is already the exception handler at that point. When building the kernel, Sentry\Monolog\Handler is loaded before Symfony\Component\HttpKernel\EventListener\DebugHandlersListener.

My work around it to disable the standard integrations and load all non-handling integrations myself:

        default_integrations: false
        integrations:
            # Defaults integrations except the error and exception handlers because we use Monolog for that.
            - 'Sentry\Integration\RequestIntegration'
            - 'Sentry\Integration\TransactionIntegration'
            - 'Sentry\Integration\FrameContextifierIntegration'
            - 'Sentry\Integration\EnvironmentIntegration'

[Jean85]

@jorrit did you check the bundle loading order? Maybe you can load the Sentry bundle later (after Monolog?) to fix this issue?