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_get_last() is not set when errors are logged #5587

Closed
mvorisek opened this issue Nov 30, 2023 · 4 comments · Fixed by #5592
Closed

error_get_last() is not set when errors are logged #5587

mvorisek opened this issue Nov 30, 2023 · 4 comments · Fixed by #5592
Labels
type/bug Something is broken version/10 Something affects PHPUnit 10

Comments

@mvorisek
Copy link
Contributor

mvorisek commented Nov 30, 2023

related with #5428

Q A
PHPUnit version 10.4.2
PHP version 8.2.12
Installation Method Composer

Summary

Current behavior

error_get_last() is not set when errors are logged (PHPUnit error handler is active). The errors are logged by default which is a good. But the current impl. of the error handler causes code that relies on error_get_last() set after a core php function has returned a failure like:

$dirHandle = opendir($dirPath);
if ($dirHandle === false) {
    $errorMessage = preg_replace('~^.+?:\s*~s', '', error_get_last()['message']);
}

to fail as error_get_last() is empty.

This is because the current impl. of the error handler returns true [1] when an error is handled. The php docs say [2]:

It is important to remember that the standard PHP error handler is completely bypassed for the error types specified by error_levels unless the callback function returns false.

[1] https://github.com/sebastianbergmann/phpunit/blob/10.4.2/src/Runner/ErrorHandler.php#L144
[2] https://www.php.net/manual/en/function.set-error-handler.php

The issue is present since PHPUnit 10, in PHPUnit 9 and lower error_get_last() was set correctly.

How to reproduce

$dirHandle = opendir('/--non-existing-path--');
self::assertNotNull(error_get_last());

Expected behavior

The PHPUnit handler should:

  1. log an error
  2. suppress its output
  3. not affect the error_get_last() result, the result must be the same /w and /wo PHPUnit error handler active

I tried to workaround about this issue https://3v4l.org/kS1vM/rfc#vgit.master but if the output is suppressed by lowering the reporting level when handling the error, it seems there is no way to restore the level back immediatelly after the error handling is finished.

I belive the solution is to modify the PHPUnit error handler:

  1. to return false when handling the error - it will let php to handle the error natively
  2. to backup the current error_reporting() level and set it to error_reporting(error_reporting() & (E_ERROR | E_PARSE | E_CORE_ERROR | E_CORE_WARNING | E_COMPILE_ERROR | E_COMPILE_WARNING)) when the handler activates (the listed error levels cannot be handled by an user handler [2])
  3. to restore the backed error level when the error handler deactivates (or better increase the current error level if lower than the backed one)
  4. e676667 original fix and c2c8dbb can be reverted then, no WithoutErrorHandler attribute needed
@sebastianbergmann
Copy link
Owner

If you rely on error_get_last(), want to test your own error handler, etc. then you need to use #[WithoutErrorHandler] to disable PHPUnit's error handler:

Issue5587Test.php

<?php declare(strict_types=1);
use PHPUnit\Framework\Attributes\WithoutErrorHandler;
use PHPUnit\Framework\TestCase;

final class Issue5587Test extends TestCase
{
    #[WithoutErrorHandler]
    public function testOne(): void
    {
        @opendir('/--non-existing-path--');

        $this->assertNotNull(error_get_last());
    }
}
$ phpunit Issue5587Test.php
PHPUnit 10.5-g9e39cfb8ed by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.0

.                                                                   1 / 1 (100%)

Time: 00:00.006, Memory: 4.00 MB

OK (1 test, 1 assertion)

@keradus
Copy link
Contributor

keradus commented Dec 25, 2023

I want to share another scenario I faced.
I was having src code with preg_* functions usage, with manual collecting of last preg error (to escalate to exception). For that, I was using error_get_last on regex failure.

I recognise WithoutErrorHandler attribute, yet I was surprised that my codebase behaves differently while SUT.
I acknowledge that dealing with error_get_last is almost like playing with php-internals, thus likely PHPUnit should not care about my scenario. I also extremely sad that whenever I want to deal with Preg_ or IO operations, native error handling is based on errors and not exceptions.

Error handler is global state. I decided to avoid relying on that global state (that could be overridden by PHPUnit, but also by whatever library I use) and put my logic to retrieve internal-error behind one-time-error-handler wrapper - thus also not using WithoutErrorHandler.

Sharing here, as maybe someone will like to get inspired by this:
https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/pull/7616/files#diff-7768e6241e8d6f50b5aabcc4e499bd261d717902ad614b0a179232e32f017bdaR35

@mvorisek
Copy link
Contributor Author

Hi @keradus, in phpunit 9.x this issue is not present as the errors are not handled, in phpunit 10.x, this will be fixed in the next minor 10.5.x release thanks to #5592. I recommend to wait for the release or you can use the 10.5 phpunit branch temporary.

@keradus
Copy link
Contributor

keradus commented Dec 26, 2023

interesting! thanks for good news!

Overall, i'm happy to wait for this to be released to upgrade Fixer to v10 afterwards.

[ Same time, I would love to get rid of any custom error handling in my codebase... and hope to see this set up in PHP itself properly ;) ]

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

Successfully merging a pull request may close this issue.

3 participants