Make registration of PHPUnit's error handler configurable

[sebastianbergmann]

PHPUnit’s test runner uses set_error_handler() to register an error handler for handling E_* issues before each test and unregisters it after each test.

When PHPUnit’s test runner becomes aware (after it called set_error_handler() to register its error handler) that another error handler was registered then it immediately unregisters its error handler so that the previously registered error handler remains active.

I would like to keep the current behaviour (see above) as default (at least for PHPUnit 11), but explore the following configurable options:

• Always register PHPUnit's error handler (and always overwrite any error handler that may have been registered before)
• Have PHPUnit's error handler call the error handler that has been registered before (in a test suite bootstrap script, for instance)

[mvorisek]

@sebastianbergmann IMO we should always register/override the error handler by default, because:

• when we do not, we cannot detect any warnings even if PHPUnit is configured so
• the user error handler is very often added by a mistake, ie. user is very often not aware of it, also, very often it is called in middle of some test becauose of some "once-per-app bootstrapping", leading to a situation when warnings prior are detected, but later silently handled not by PHPUnit
• there are quite not many situations when user error handler is wanted, thus WithoutErrorHandler attribute is good solution already

Have PHPUnit's error handler call the error handler that has been registered before (in a test suite bootstrap script, for instance)

I have thought about this and it is quite challenging to be meaningful this way. Do you want user handler to handle deprecations or any warnings in general when they have been successfully detected by PHPStan?

I belive the solution is simply to always regesister the PHPUnit handler and when unit test does not need it, WithoutErrorHandler attribute can be used. When a test does need a custom error handler, the test itself should add the error handler prior the test and remove it afterwards.

[ben-challis]

I thought in modern PHP it was a pretty common practice within a project to be converting E_NOTICE, E_WARNING, E_ERROR etc to \Throwable as they typically can and should be reacted to at the call site? In such situations, you are dealing with a stable environment where by you are in control of your bootstrap and error reporting / handling methods, the unit tests are operating within that base bootstrap. You can safely rely on this as you are a project. Having to opt out specific tests with WithoutErrorHandler here just feels very verbose, as well as being difficult to reason about.

For example, we had to migrate the following code for 9.x -> 10.x. I believe this can possibly now be undone but I think it's a good example scenario) which works with ext-decimal, which will trigger E_WARNING on truncation of precision, and our project's error handler converts to a Throwable.

Previously it was just:

    private static function decimalFromString(string $value): Decimal
    {
        try {
            return new Decimal($value, self::PRECISION);
        } catch (\Throwable $exception) {
            throw new \InvalidArgumentException(
                \sprintf(
                    'Failed to construct decimal with value "%s" and precision %d: %s',
                    $value,
                    self::PRECISION,
                    $exception->getMessage(),
                ),
                previous: $exception,
            );
        }
    }

To support PHPUnit's error handler replacing ours, we now have:

    private static function decimalFromString(string $value): Decimal
    {
        \set_error_handler(
            function (int $errorNumber, string $errorString, string $errorFile, int $errorLine) use ($value): bool {
                if ($errorNumber !== \E_WARNING) {
                    return false;
                }

                throw new \InvalidArgumentException(
                    \sprintf(
                        'Failed to construct decimal with value "%s" and precision %d: %s',
                        $value,
                        self::PRECISION,
                        $errorString,
                    ),
                );
            },
        );

        try {
            return new Decimal($value, self::PRECISION);
        } catch (\DomainException $exception) {
            throw new \InvalidArgumentException(
                \sprintf(
                    'Failed to construct decimal with value "%s" and precision %d: %s',
                    $value,
                    self::PRECISION,
                    $exception->getMessage(),
                ),
            );
        } finally {
            \restore_error_handler();
        }
    }

(Constructing a Decimal can throw a DomainException for certain issues, hence the catch remaining but being narrowed)

Which actually is quite a bit slower at runtime, as this now needs error handler juggling every time a decimal value is constructed from a string. We cannot simply use WithoutErrorHandler on a specific unit test that's testing truncation, as due to this just being a value object, it's used throughout the tested code. Continuing execution on the E_WARNING by removing the applications error handler results in incorrect behaviour of the project during tests compared to runtime.

You of course have the other case where you're testing a library and you aren't in control of runtime error handling setup. However even in this case it's difficult as you really want to be running your test suite twice, once with no error handler set up, and once with an error handler that converts warnings etc to exceptions and ensures you are handling both situations as consistently as possible.