Do not register the error handler if the DSN is null

[VincentLanglet]

Related to https://github.com/getsentry/sentry-symfony/issues/46#issuecomment-780003387

it should be possible to change the way the client is built during the container compilation so that if the DSN is null then we skip adding those integrations to the client.

[Jean85]

Currently, there are 3 methods that register an handler of some kind: exceptions, errors, fatals

• https://github.com/getsentry/sentry-php/blob/51af981636bbe97fe9271dab76819d02593c3191/src/ErrorHandler.php#L194
• https://github.com/getsentry/sentry-php/blob/51af981636bbe97fe9271dab76819d02593c3191/src/ErrorHandler.php#L131
• https://github.com/getsentry/sentry-php/blob/51af981636bbe97fe9271dab76819d02593c3191/src/ErrorHandler.php#L167

All 3 of them are used ONLY in the relative integration:

• https://github.com/getsentry/sentry-php/blob/51af981636bbe97fe9271dab76819d02593c3191/src/Integration/ExceptionListenerIntegration.php#L21
• https://github.com/getsentry/sentry-php/blob/51af981636bbe97fe9271dab76819d02593c3191/src/Integration/ErrorListenerIntegration.php#L22
• https://github.com/getsentry/sentry-php/blob/51af981636bbe97fe9271dab76819d02593c3191/src/Integration/FatalErrorListenerIntegration.php#L23

So, removing those 3 integrations from the building of the init, it should be enough to avoid the error handler active at all. Or am I missing something?

[ste93cry]

Looking at the code I think you're right, if the DSN is null we can safely avoid instantiating those integrations in IntegrationRegistry::getDefaultIntegrations(). Since you cannot change the DSN for a client after its instantiation, we are safe that that client will never need the integration. Of course it's still possible for people to enable the integration regardless by setting it in the integrations option themselves. ~Are you open to submit the change?~

Edit: Since I had a bit of time this evening, I coded the change. The only thing I'm not sure before opening the PR is if we should treat this as an improvement or a bug. Technically speaking there is nothing broken, but even though since version 2.0 it worked like this I'm more on the side that it should not. Any opinion on this?

[Jean85]

I have two opposing views on this:

• on one hand, it would mitigate https://github.com/getsentry/sentry-symfony/issues/421 by avoiding interfering with the error handler
• OTOH, the fact that you avoid registering the handler at all could exposes us to subtle changes between the local environment and prod, and this could potentially lead to nasty and hard-to-reproduce bugs..

[ste93cry]

on one hand, it would mitigate getsentry/sentry-symfony#421 by avoiding interfering with the error handler

Yes, that's the main reason to release this as a bugfix rather than an improvement so that that issue does not have to wait too long yet to be fixed

OTOH, the fact that you avoid registering the handler at all could exposes us to subtle changes between the local environment and prod, and this could potentially lead to nasty and hard-to-reproduce bugs..

I don't see a real problem with this, this is how a lot of things work: they are disabled in development and are enabled only in production

[bitclaw]

Any news regarding this?

Just setup a new Symfony 5.3 project for some legacy code and ran into this when running some integration tests I wrote:

3974d77b1b62:/var/www/symfony# php ./vendor/bin/phpunit

PHPUnit 9.5.10 by Sebastian Bergmann and contributors.

...                                                                 3 / 3 (100%)

Time: 00:01.697, Memory: 52.50 MB

OK (3 tests, 7 assertions)

THE ERROR HANDLER HAS CHANGED!
3974d77b1b62:/var/www/symfony# 

[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 🥀

[nocive]

bump

[cleptric]

@ste93cry @Jean85 Is this something we still want to pursue?

[ste93cry]

From my side, yes, absolutely this is something it's time to work on. I know @Jean85 had a different opinion on the solution, but the way it works now is just causing so many headaches and troubles that it's not worth it in my opinion.