Enable error handlers back

[Jean85]

While discussing with a colleague, he made me notice that the current setup of the bundle doesn't report everything. Due to the disabled error handler, all trigger_error are ignored if they do not raise an exception; so deprecations or notices cannot be collected like this. This was also reported as with #318 by @ruudk.

With this PR I'm adding new, improved E2E tests to try and get back the handles, so we can collect all errors, without the old issue of duplicated events (see #156 as an example).

Please, test this out because I'm not 100% sure that my E2E tests are reproducing the real behavior correctly.

[ste93cry]

My fear is that by having two error handlers they could conflict or the errors could be handled twice or handled differently from what would happen without Sentry. What about investigating whether it's possible to attach to the Symfony error handler instead?

[Jean85]

The new end to end tests should keep us safe in regards of double reporting, and in fact I've spotted just one case, fatals, and I've safely ignored them.

As for attach points, there are only two and those are not sufficient: a PSR-3 logger and the possibility to attach an exception handler, nothing more.

[ste93cry]

Why it's the PSR-3 logger not sufficient?

[Jean85]

Because it's very limited in information, only level & message; no stack trace, for example.

[ste93cry]

To me it looks like the exception is passed in the context 🤔 Did you look at how Monolog logs errors in a Symfony application?

[Jean85]

Monolog handler is a completely separate option in this bundle: #247

[ste93cry]

Sure, what I meant was to look at how Monolog was integrated in Symfony out-of-the-box (e.g. when it logs to var/logs/*) to understand if they use a custom error handler or not

[Jean85]

Monolog is PSR-3 compliant, so that interface is surely used as a junction. I can't do that without losing information, as I said.

Also, it seems that Symfony simply injects the logger service wherever is needed:

• https://github.com/symfony/http-kernel/blob/a3b12e2925bc9f042b92e1ead400aab9ad408c76/EventListener/ErrorListener.php#L39
• https://github.com/symfony/http-kernel/blob/a3b12e2925bc9f042b92e1ead400aab9ad408c76/EventListener/RouterListener.php#L72
• https://github.com/symfony/http-kernel/blob/a3b12e2925bc9f042b92e1ead400aab9ad408c76/EventListener/DebugHandlersListener.php#L54

[ste93cry]

I can't do that without losing information, as I said

We discussed this privately, but I'm gonna leave here some notes for future reference: as far as I remember from our conversation, you have at disposal everything you need from the Monolog context (the exception is always present under the context.exception field, and if it isn't present then the client will use the backtrace instead. Also, what I was talking about was to create a little PSR-3 implementation to attach to the Symfony ErrorHandler component. The issue however is that there can be only one logger at once registered with the handler, so we cannot go on with this proposal.

Said this, I see a big issue with this change, if I'm not missing something: it looks to me that now all integrations are enabled by default regardless of whether Monolog is used, so either you have to update the README to explain that you manually need to disable the (FatalError|Error|Exception)ListenerIntegration integrations or you need to handle this case while configuring the DI container

[Jean85]

Said this, I see a big issue with this change, if I'm not missing something: it looks to me that now all integrations are enabled by default regardless of whether Monolog is used, so either you have to update the README to explain that you manually need to disable the (FatalError|Error|Exception)ListenerIntegration integrations or you need to handle this case while configuring the DI container

Nope, that's not the case. The monolog-sentry integration is NOT enabled by default, as before; and there's already an option to shut off the listeners, useful if you enable the Monolog integration.

[ste93cry]

I know that the Monolog integration isn't enabled by default, but as soon as I enable it then those integrations should be disabled without having to do it manually, do you agree? Even if the listener gets disabled, this does not prevent the error handler from being registered as soon as one of those integrations run, so it's better if they don't even get set up

[Jean85]

So you would bind the enabling of the default integrations to the register_error_listener option? That seems feasibile... I'll dig into it.

[ste93cry]

So you would bind the enabling of the default integrations to the register_error_listener option

Yes, also because not doing this means breaking the compatibility with the current applications that have Monolog enabled: they will suddendly start having one new error handler registered, which could cause issues in the long run

[Jean85]

@ste93cry can I get an approval here? I would like to merge this and use it as a fallback after I implement integrating with Monolog as the first choice as you and Nicolas suggested.

[ste93cry]

I still think that even if this is a fallback it's not the best nor appropriate solution that we should provide, but ok, LGTM

[Jean85]

Due to #337 + 02e6902, master is now 4.x. Fixing conflicts and merging now.