Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require errorReportingLevel to be a subset of error_reporting #611

Merged
merged 9 commits into from
Oct 20, 2020

Conversation

imjoehaines
Copy link
Contributor

@imjoehaines imjoehaines commented Oct 19, 2020

Goal

PHP 8 has changed how the @ operator works:

The @ operator will no longer silence fatal errors (E_ERROR, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR, E_RECOVERABLE_ERROR, E_PARSE).

https://github.com/php/php-src/blob/7be61bec8037e601c989e5002198f77bf39e8d59/UPGRADING#L59-L82

Previously, error_reporting would return '0' if @ is used but in PHP 8 it can now return any combination of fatal error types, depending on which were enabled (see https://3v4l.org/Z2lfD)

This means that our current check for the @ operator will no longer work so we need an alternative in order to fully support PHP 8. This is made more complicated because we have a second, Bugsnag only error level (errorReportingLevel), otherwise this would be a simple change

Design

Quite a few options were explored, but eventually we decided that the best way to solve this is to require that Bugsnag's errorReportingLevel is a subset of error_reporting — i.e. every error level in errorReportingLevel must also be in error_reporting

This allows us to check if something has been silenced by checking against error_reporting first — if the error code is not in error_reporting then it has either been silenced or was never being reported to begin with

If errorReportingLevel is not a subset of error_reporting, a warning will be logged and all errors of the incorrect level(s) will be ignored

For example, this would be valid usage of errorReportingLevel because everything in errorReportingLevel is also in error_reporting:

error_reporting(E_ALL & ~E_WARNING & ~E_USER_DEPRECATED);
$client->setErrorReportingLevel(E_ALL & ~E_WARNING & ~E_USER_DEPRECATED & ~E_NOTICE & ~E_USER_NOTICE);

However, this would be invalid usage of errorReportingLevel because E_WARNING is not in error_reporting but is in errorReportingLevel:

error_reporting(E_ALL & ~E_WARNING);
$client->setErrorReportingLevel(E_ALL);

As before, setting errorReportingLevel to 0 will ignore all errors and setting it to null (or not setting it at all) will use error_reporting instead

We decided to implement this universally across PHP versions, so that there's not an unexpected change in Bugsnag's behaviour when going from PHP 7 → 8

Changeset

  • Configuration::setErrorReportingLevel now validates the given level is a subset of error_reporting
  • Configuration::shouldIgnoreErrorCode uses a PHP 8 compatible way to check for @, enabled by the above change
  • Additional helper methods added to ErrorTypes
  • Set error_reporting to include all error types in PHPUnit config (this previously varied based on PHP/PHPUnit versions)
  • Tests for all the above

These tests were setting error_reporting to 0 as a proxy for using
error suppression. Instead we should use '@' for real, which proves
that errors are ignored when suppression is actuall used and because
PHP 8 changes error_reporting's value when suppression is used
This is a workaround for a PHP 8 issue, where there is no longer
a reliable sentinel value for error_reporting when error suppression
is used

Previously error_reporting would equal '0', but in PHP 8 fatal errors
will not be suppressed (but if fatal errors were not in error_reporting
to begin with, they will not be added)

PHP's error_reporting continues to work with this change because its
value is automtatically changed at runtime when '@' is used. Our own
error reporting option doesn't have this luxury, so we need to try to
detect if error suppression is used, which can only be done via the
current error_reporting value

By ensuring that Bugsnag's error reporting level is always a subset
of error_reporting, we can check for error suppression by checking
against error_reporting — if something is not in error_reporting
then it cannot be in Bugsnag's error reporting level as then it
wouldn't be a subset
This was accidentally using a boolean and (&&) rather than a bitwise
and (&) but happened to work anyway :D

Now also tests for '0' (ignore all error codes) and 'null' (use the
global error_reporting)
@imjoehaines imjoehaines marked this pull request as ready for review October 19, 2020 14:45
@imjoehaines imjoehaines requested a review from kattrali October 19, 2020 14:45
@imjoehaines imjoehaines merged commit 30e396f into next Oct 20, 2020
@imjoehaines imjoehaines deleted the error-reporting-subset branch October 20, 2020 08:48
@imjoehaines imjoehaines mentioned this pull request Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants