sebastianbergmann / phpunit

The PHP Unit Testing framework.
https://phpunit.de/
BSD 3-Clause "New" or "Revised" License
19.61k stars 2.19k forks source link

Fatal with some error/exception handlers #5844

Open kkmuffme opened 1 month ago

kkmuffme commented 1 month ago
Q A
PHPUnit version 11.1.3
PHP version 8.2.x
Installation Method both

Summary

set_error_handler and set_exception_handler do not necessarily return a callable - the PHP docs are wrong. See https://github.com/php/doc-en/issues/3421

Since phpunit uses both to store and then set them again in e.g. https://github.com/sebastianbergmann/phpunit/blob/main/src/Framework/TestCase.php#L1938 in conjunction with https://github.com/sebastianbergmann/phpunit/blob/main/src/Framework/TestCase.php#L1950 this leads to phpunit not working

In case this error handler originates from a dependency/outside of the project, that can't be changed and phpunit cannot be used at all

Current behavior

phpunit doesn't work/fatal

How to reproduce

See the php-doc issue for examples

Expected behavior

No fatal

EDIT: as a possible solution checking

if (!is_callable($previousHandler) {
    // @todo report risky or something?
    break;
}

before the assignment https://github.com/sebastianbergmann/phpunit/blob/main/src/Framework/TestCase.php#L1944C17-L1944C39

sebastianbergmann commented 1 month ago

Thank you for your report.

Please provide a minimal, self-contained, reproducing test case that shows the problem you are reporting.

Without such a minimal, self-contained, reproducing test case I will not be able to investigate this issue.

kkmuffme commented 1 month ago

Here's a really basic one (of course not very realistic, but it's minimal and self-contained). Same thing also (more realistic) is possible with exception handler

<?php declare(strict_types=1);
/*
 * This file is part of PHPUnit.
 *
 * (c) Sebastian Bergmann <sebastian@phpunit.de>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */
namespace PHPUnit\TestFixture;

use function set_error_handler;
use function trigger_error;
use PHPUnit\Framework\TestCase;

class SomeExternalDependency {
    public function __construct()
    {
        set_error_handler([$this, 'logError']);
    }

    private function logError(): bool
    {
        return false;
    }
}

class Issue5844Test extends TestCase
{

    public function testSetErrorHandlerNonCallable(): void
    {
        new SomeExternalDependency();

        trigger_error('error', E_USER_WARNING);

        $this->assertTrue(true);
    }
}
mvorisek commented 1 month ago

Here is an repro: https://3v4l.org/GFd18

The issue is the callback must be restored in rebound context. I belive reflection can be used to detect the target context.

Here is docs https://www.php.net/manual/en/language.types.callable.php for all supportted callable types. All possibly private/not-callable-from-non-this must be tested.

https://github.com/php/php-src/issues/14494 should be linked as IMO it is a little flaw in php design/callable validation on set_error_handler.