Fix incorrect detection of silenced errors (by the @ operator)

[stayallive]

As can be found here the @ operator no longer silences all error types starting with PHP 8: https://php.watch/versions/8.0/fatal-error-suppression.

Initially I thought the following was happening but that is not true: So when capturing a silenced error the error_reporting() will not report 0 on PHP 8 but 4437 (which is E_ERROR | E_PARSE | E_CORE_ERROR | E_COMPILE_ERROR | E_USER_ERROR | E_RECOVERABLE_ERROR, check for yourself: https://maximivanov.github.io/php-error-reporting-calculator/).

When capturing a silenced error the error_reporting() will not report 0 on PHP 8 but E_ERROR | E_PARSE | E_CORE_ERROR | E_COMPILE_ERROR | E_USER_ERROR | E_RECOVERABLE_ERROR without the ones you excluded in an earlier error_reporting() call yourself (for example error_reporting(E_ALL & ~E_USER_ERROR) would result in error_reporting() being: E_ERROR | E_PARSE | E_CORE_ERROR | E_COMPILE_ERROR | E_RECOVERABLE_ERROR when handling a silenced error on PHP 8 (missing the E_USER_ERROR)).

The current behaviour makes it so that error_reporting() is leading in determining what is a "silenced error" which is simply not true under PHP 8 (or 7 for that matter).

Currently when you set error_reporting(E_ALL & ~E_NOTICE) and 'error_types' => E_ALL it will currently report all E_NOTICE as silenced errors (thus not report them at all) which is incorrect and something I consider to be a bug and the incorrect behaviour.

This PR aims to resolve the above.

There is 1 edge case in this "detection" of silenced errors. If the user set's their error_reporting() to 0 on PHP 7 and below or anything only containing the error types that cannot be silenced on PHP 8 and up we detect all errors as silenced, but I consider this acceptable since this was always the way it was before the broken code was introduced. If we find this unacceptable we need to remove the silenced errors detection all together.

[Jean85]

Thank you for picking this really nasty issue!

[ste93cry]

I have to admit I'm having a really hard time understanding the issue without a real code example at disposal, but (correct me if I'm wrong) what you are saying is that we are reporting some errors as silenced when they are not, right?

[stayallive]

It looks like as @ste93cry mentioned I overlooked that the error_reporting() set when reporting on silenced errors is not fixed as I thought on PHP 8 (that's why we review), so I created a workaround for that too (the joys of error handling code).

Also to show both the difference between what PHP 7 and 8 reports for error_reporting() when handling silenced errors but also showing our current code being broken I have this example (adapted from @ste93cry): https://3v4l.org/nct1E (I would expect it to report the silent status correctly which is does not).

I have also tried to add more tests and better wording what I am doing and why I am dong it hopefully explaining what is going on and why the code does what it does.

[Jean85]

We should backport those tests in the 2.x branch and apply fixes if needed.

[stayallive]

There is no need to backport because this was introduced in #1087 and that change only went in 3.x so we're good!

[ste93cry]

After talking on Discord with @stayallive, I'm gonna approve this PR to not block it further, however I totally disagree with what has been done. Taking as example the following code, what would happen is that since the error_reporting() is configured to not report the E_USER_NOTICE errors, the error generated by the undefined $helloworld variable will not be shown to the user, therefore acting as if it was silenced using the @ operator

error_reporting(E_ALL & ~E_NOTICE);

echo $helloworld;

What's happening right now with Sentry is that the error will be reported as a SilencedErrorException, which I agree it may be misleading in the strict terms as it hasn't been silenced manually but rather using error_reporting(). Obviously, apart from an incorrect labeling of the error type, the real issue is that users may lose the error if the don't have capture_silenced_errors = true. However, with this patch, what will happen is that the error will be reported as a ErrorException, leading users into thinking that this error is not suppressed at all and consequently has been displayed to their customers. According to the documentation and also to the behaviour that SDKs have had until now, this is expected. This is where I disagree: for me, the behaviour has always been wrong, we changed it starting from version 3.0 (as a consequence of another change, but it is so) and now we want to go back to that old behaviour.

What I would do instead is fixing the documentation, and in case investigate if it's possible to distinguish between errors silenced using the @ operator and errors suppressed by the error_reporting setting in order to make the capture_silenced_errors option not affecting this last case. But even if that's not possible, I cannot think of an error that has not been shown as an error that is not suppressed 👎

[ste93cry]

FYI: this is a bugfix and as such should have been merged into the master branch, not the develop branch. I cherry-picked the patch into the master branch with 2c59c6c