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

Error handlers that are not callable outside the scope they were registered in are not handled correctly #5844

Closed
kkmuffme opened this issue May 28, 2024 · 4 comments
Labels
type/bug Something is broken

Comments

@kkmuffme
Copy link

kkmuffme commented May 28, 2024

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 php/doc-en#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

@kkmuffme kkmuffme added the type/bug Something is broken label May 28, 2024
@sebastianbergmann
Copy link
Owner

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.

@sebastianbergmann sebastianbergmann added the status/waiting-for-feedback Waiting for feedback from original reporter label May 29, 2024
@kkmuffme
Copy link
Author

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);
    }
}

@sebastianbergmann sebastianbergmann removed the status/waiting-for-feedback Waiting for feedback from original reporter label May 29, 2024
@mvorisek
Copy link
Contributor

mvorisek commented Jun 7, 2024

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.

php/php-src#14494 should be linked as IMO it is a little flaw in php design/callable validation on set_error_handler.

@sebastianbergmann sebastianbergmann changed the title Fatal with some error/exception handlers Error handlers that are not callable outside the scope they were registered in are not handled correctly Oct 19, 2024
@kkmuffme
Copy link
Author

This will completely remove it though?

Wouldn't the correct way to do it be to add it back with something like php/php-src#14494 (comment) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something is broken
Projects
None yet
Development

No branches or pull requests

3 participants