Fix #1000: Respect error_reporting

[PatchRanger]

Fix #1000

[PatchRanger]

Travis tests failed not by my fault) I guess, the branch master became "red" after that: https://travis-ci.org/github/getsentry/sentry-php/builds/672025512 .

[Jean85]

This is in fact a BC, we cannot accept it as a patch. Sentry has its own error reporting level (Options::getErrorTypes()), which uses the same constants of error_reporting, and is E_ALL by default, which is applied in two points:

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

  Lines 77 to 79 in e0b6e8c

  	if (!($options->getErrorTypes() & $exception->getSeverity())) {
  	return;
  	}

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

  Lines 57 to 59 in e0b6e8c

  	if (!($options->getErrorTypes() & $exception->getSeverity())) {
  	return;
  	}

Applying this change would mean intersecting the two settings.

[Jean85]

This is something different. A SilencedErrorException is something like @foo(), related to the @ silencing operator.

[PatchRanger]

I see that it was designed as related to @. I see that Sentry has its own reporting level. My point is that the case of intentionally silenced by tweaking error_reporting warnings should be handled the same way.

Let's review how it should work in such particular case:

• I have error_reporting=E_ALL.
• My Sentry error reporting is set to WARNING: I want to see in my Sentry dashboard everything that equals or above WARNING.
• Symfony decides to workaround missed cache issue by tweaking error_reporting level. The idea is to intentionally "skip" particular warning. I don't want to see it in my Sentry dashboard, it was silenced - just by another way than @, see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Kernel.php#L431 .

In my view tweaking error_reporting level should be considered as another way of intentionally silencing @.

This is in fact a BC, we cannot accept it as a patch.

Ok, I understand. I am going to rebase it to another branch once we reconcile the most appropriate way of dealing with this issue.

[PatchRanger]

Applying this change would mean intersecting the two settings.

You made me think, thank you :-)
I think there is no intersection:

• Sentry Options::getErrorTypes() controls whether to report or not. It uses the same constants and defaults to E_ALL - but is totally independent of error_reporting setting: it is not controlled by tweaking error_reporting.
• Globally defined error_reporting impacts how to report the error: either silenced or usual way. It doesn't matter how error_reporting was tweaked: either by @ or manually - does it?
• There is another option (setCaptureSilencedErrors), which makes to report even silenced errors.

In my view it works almost the same way as we already have: just the case of 0 === error_reporting() was expanded to $level & error_reporting(). Isn't it?:)

[nicolas-grekas]

I think this patch is correct and fixes an issue. error_reporting() returns a bitmask and should be used as such.

[Jean85]

I think this patch is correct and fixes an issue. error_reporting() returns a bitmask and should be used as such.

That is technically correct; but we already handle the bitmask elsewhere, which I linked in my review message #1001 (review). Those two pieces of code interact with this change, in a way that IMO is not correct.

With this patch, we would have this situations:

• error is silenced with @: this patch doesn't change anything, because it's converted to a SilencedErrorException in both cases, and it would be reported depending on Sentry's capture_silenced_errors option.
• error matches both bitmasks (PHP's and Sentry's): no changes in behavior here too, error is reported fully
• error matches only PHP's bitmask: no changes here too, error is NOT reported to Sentry
• error matches only Sentry's bitmask: here we have a change
  • Before: error is wrapped in an ErrorException anyway, and reported
  • After: error is wrapped in a SilencedErrorException, and gets reported depending on the capture_silenced_errors option.

So, bottom line, I don't understand what you want to achieve with this patch. If you do not want some error reported, you should simply change your Sentry error_types options, that's all. This patch wouldn't give you anything more.

[PatchRanger]

So, bottom line, I don't understand what you want to achieve with this patch. If you do not want some error reported, you should simply change your Sentry error_types options, that's all. This patch wouldn't give you anything more.

I want )) I want all E_WARNING to be reported - except those ones that were intentionally silenced (either using @ or via error_reporting tweak). So I left my Sentry settings untouched - and applied this patch, which changes the logic of determining which errors are silenced.

[Jean85]

Understood. Silencing errors through hot-swapping error_reporting values seems a bit strange to me, but it's probably performance-related if it's done in Symfony like this.

Nicolas suggested to avoid the error handler approach at all and I would like to evaluate his suggestion, but that's a long-run fix, since it would probably require a new major version for this bundle. In the meantime, you can work around your issue by ignoring the events server side, or using a before_send filter to avoid sending those alltogether.

[ste93cry]

it's probably performance-related if it's done in Symfony like this

The real reason, as it's written in a comment just before the offending line, is that silencing errors using the @ operator would silence all errors regardless of the level. Instead, Symfony wants to silence only warnings. I'm not sure about this, but to me the way it's done is similar to setting a custom error handler and then reverting it later, with the added bonus that it should not trigger the famous bug we all know about when the code is being run inside an error handler.

Nicolas suggested to avoid the error handler approach at all and I would like to evaluate his suggestion

I don't think that this impacts this package as it may be used without the bundle part, but it's related to getsentry/sentry-symfony#322 for sure

I want all E_WARNING to be reported - except those ones that were intentionally silenced (either using @ or via error_reporting tweak)

I think I get what you want and you're absolutely correct in saying that it's not possible right now. According to the capture_silenced_errors option, we skip errors silenced using the @ operator but we indeed don't skip errors that don't match error_reporting() because the bitmask comparison is done against the SDK error_types option, which is of course something out of PHP control. Even if error_types defaulted to error_reporting(), which btw is something that we plan to do in 3.0, the issue would not be solved if the INI setting changes later. Now, we have one constraint to avoid breaking BC: change the aforementioned default value. In the future, it may make sense to drop our option and rely only on error_reporting() as it probably doesn't really make sense to say to PHP to report certain errors and then say Sentry to report something totally different. For now, I honestly cannot think of anything to introduce the support of error_reporting() without breaking the behavior of the current applications that expect to not be affected by such setting. Of course I'm open to suggestions, and if the suggestions do not involve adding one more option to the SDK it would be even cooler

[nicolas-grekas]

TBH I don't think there is any BC break to consider here. That's a bug. Fixing any bug can be considered a BC break. Here, I wouldn't expect any app to require that an error level excluded by the bitmask would still be reported. If any app does so, it's one that is tightly coupled to this specific error handler, with a behavior that diverges from what another app relying on standard PHP behavior would expect.
That's just a bug...

[ste93cry]

Unfortunately the error_types option has been inherited from 1.x, thus the behavior we're talking about is well known and even if we decide that we don't care about BC I fear that we will impact a lot of people relying on it without considering it an issue. However, I see that in 1.x a null value was not only allowed but also was the default, and in that case error_reporting was used instead. We could do something similar here until 3.0, WDYT?

[Jean85]

Yes we could, and I would make it the default value, but only in 3.0 due to BC, again.

[PatchRanger]

Yes we could, and I would make it the default value, but only in 3.0 due to BC, again.

@Jean85 If we extend this PR with switching default value of capture_silenced_errors to true, would it be less destructive to be considered as no-BC-break change?
Pros:

• Still fixes the bug.
• No new option.
• All silenced by tweaking error_reporting would be reported by default. Could be turned off by aforementioned capture_silenced_errors setting.

Cons:

• All @-silenced would also start to be reported for those, who doesn't override this default setting. Not sure how critical it is, in my view could be considered as "feature"))

[ste93cry]

would it be less destructive to be considered as no-BC-break change?

No, every big or small change to the defaults is still a BC and we cannot do it, also in consideration of what I said previously about the fact that people never reported this before and thus I assume that they don't consider this a bug but rather a feature or something that still works fine for them as-is. What should be done is make the error_types option support null, and if the option is set to that value then we should compare against error_reporting(). I would also like to deprecate entirely the option, but I know @Jean85 disagrees so I would like to discuss about this and understand if there is a real use case that prevent us from doing this. In the assumption we always made that our error handler should be as transparent as possible I absolutely agree with the thinking of @nicolas-grekas that everyone relying on this option is coupling the behavior of error handling of its app with this package and this is incorrect and should be discouraged

[ste93cry]

I'm closing this because it's pretty old, and because I think we fixed it in #1196