Do not register the error handler if the DSN is null

[VincentLanglet]

Related to getsentry/sentry-symfony#46 (comment)

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

• sentry-php/src/ErrorHandler.php

  Line 194 in 51af981

  public static function registerOnceExceptionHandler(): self

• sentry-php/src/ErrorHandler.php

  Line 131 in 51af981

  public static function registerOnceErrorHandler(): self

• sentry-php/src/ErrorHandler.php

  Line 167 in 51af981

  public static function registerOnceFatalErrorHandler(int $reservedMemorySize = self::DEFAULT_RESERVED_MEMORY_SIZE): self

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

• sentry-php/src/Integration/ExceptionListenerIntegration.php

  Line 21 in 51af981

  $errorHandler = ErrorHandler::registerOnceExceptionHandler();

• sentry-php/src/Integration/ErrorListenerIntegration.php

  Line 22 in 51af981

  $errorHandler = ErrorHandler::registerOnceErrorHandler();

• sentry-php/src/Integration/FatalErrorListenerIntegration.php

  Line 23 in 51af981

  $errorHandler = ErrorHandler::registerOnceFatalErrorHandler();

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 Overriding error handler changes symfony error handler sentry-symfony#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# 

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