Skip to content

[Framework bundle] Cleanup error and exception handlers on Kernel shutdown #53812

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

Closed
simPod opened this issue Feb 6, 2024 · 46 comments · Fixed by #58372
Closed

[Framework bundle] Cleanup error and exception handlers on Kernel shutdown #53812

simPod opened this issue Feb 6, 2024 · 46 comments · Fixed by #58372
Labels
Bug FrameworkBundle Help wanted Issues and PRs which are looking for volunteers to complete them. Status: Reviewed

Comments

@simPod
Copy link
Contributor

simPod commented Feb 6, 2024

Symfony version(s) affected

7.0.0

Description

PHPUnit 11 checks for any leftovers in error handlers sebastianbergmann/phpunit#5619

How to reproduce

Run FrameworkBundle->boot() in phpunit test.

$handler = ErrorHandler::register(null, false);

It results in Test code or tested code did not remove its own exception handlers thrown by phpunit.

Possible Solution

Introduce FrameworkBundle::shutdown() that would cleanup error handler set in boot(). IMO it's reasonable to have some cleanup in shutdown() of things initiated in boot().

Additional Context

No response

@derrabus
Copy link
Member

derrabus commented Feb 6, 2024

Isn't this something that WebTestCase could handle?

@simPod
Copy link
Contributor Author

simPod commented Feb 6, 2024

IMO it should not be bound to a sf test case. Users usually extend their testcase and cannot extend another one.

It should be something that can be triggered via composition, not inheritance.

@simPod simPod changed the title [Framework bundle] Cleanup error handler on Kernel shutdown [Framework bundle] Cleanup error and exception handlers on Kernel shutdown Feb 7, 2024
@derrabus
Copy link
Member

derrabus commented Feb 9, 2024

This issue is a blocker for PHPUnit 11 in any Symfony project that uses KernelTestCase or WebTestCase. Given that PHPUnit comes with its own error handler, maybe the solution would be to opt out of Symfony's own error handler when booting the kernel for functional tests?

@derrabus derrabus added Status: Reviewed Help wanted Issues and PRs which are looking for volunteers to complete them. and removed Status: Needs Review labels Feb 9, 2024
@simPod
Copy link
Contributor Author

simPod commented Feb 9, 2024

that uses KernelTestCase or WebTestCase

this is not a root cause. I'd correct that to:

This issue is a blocker for PHPUnit 11 in any Symfony project that uses Kernel in tests.

E.g. for me the test maintenance is much easier when I'm not relying on SF testcases so I can extend my own leaner testcase. I use Kernel via composition in tests.

So the root cause is that there's no way to cleanup after Kernel, not some test case.

@olifrieda
Copy link

Same issue "Test code or tested code did not remove its own exception handlers" here with

  • PHP 8.2.15
  • Symfony 6.4.3
  • phpunit 11

@rubenrubiob
Copy link

I had the same problem with Symfony 6.4 and PHPUnit 11. I temporarily solved it by calling this method in the tearDown of my base test case (which extends from KernelTestCase or WebTestCase):

protected function restoreExceptionHandler(): void
{
    while (true) {
        $previousHandler = set_exception_handler(static fn() => null);

        restore_exception_handler();

        if ($previousHandler === null) {
            break;
        }

        restore_exception_handler();
    }
}

The tearDown looks like this:

protected function tearDown(): void
{
    parent::tearDown();

    $this->restoreExceptionHandler();
}

It is based on what Laravel did on their testsuite: https://github.com/laravel/framework/pull/49622/files

Maybe the solution is to call a similar method in the tearDown of KernelTestCase and WebTestCase?

@simPod
Copy link
Contributor Author

simPod commented Feb 22, 2024

Currently, I cleanup exceptions like this:

        $res = [];

        while (true) {
            $previousHandler = set_exception_handler(static fn () => null);
            restore_exception_handler();

            if (
                is_array($previousHandler)
                && $previousHandler[0] instanceof \Symfony\Component\ErrorHandler\ErrorHandler
                && $previousHandler[1] === 'handleException'
            ) {
                restore_exception_handler();

                continue;
            }

            if ($previousHandler === null) {
                break;
            }

            $res[] = $previousHandler;
            restore_exception_handler();
        }

        $res = array_reverse($res);

        foreach ($res as $handler) {
            set_exception_handler($handler);
        }

The same for errors

@frankdekker
Copy link

A different workaround, seeing as the Symfony/Errorhandler is a singleton and will only register once, is to register the error handler before phpunit memorizes all the existing error handlers. This way the error handler Symfony register's is not new.

Create (or modify) /tests/bootstrap.php:

<?php
declare(strict_types=1);

use Symfony\Component\ErrorHandler\ErrorHandler;

ErrorHandler::register(null, false);

and in phpunit.xml

<?xml version="1.0" encoding="UTF-8"?>
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/11.0/phpunit.xsd"
         bootstrap="tests/bootstrap.php">
...
</phpunit>

@derrabus
Copy link
Member

derrabus commented Feb 24, 2024

@frankdekker is right, the error handler registered by FrameworkBundle would not replace the active error handler. PHPUnit already registers its own error handler and that one is still active at the end of the test. So we're good here.

The problem which PHPUnit reports is about the exception handler. Yes, error handler and exception handler are two different things. And yes, it's super confusing that Symfony has a method called ErrorHandler::register() that registers an error handler and an exception handler. 😓

PHPUnit on the other hand does not register an exception handler and even if it did, that handler would be wrapped by ErrorHandler::register() which PHPUnit would still rightfully complain about.

But. ErrorHandler won't replace the exception handler if ErrorHandleralready is the current exception handler. This means, we can simplify @frankdekker's bootstrap script like this:

<?php

declare(strict_types=1);

use Symfony\Component\ErrorHandler\ErrorHandler;

require dirname(__DIR__).'/vendor/autoload.php';

set_exception_handler([new ErrorHandler(), 'handleException']);

That being said, this feels like a hack. Maybe we should add a way to opt out of the exception handler. In situations where the kernel is executed within a huge try/catch block (which a PHPUnit test basically is), that exception handler does not serve any purpose anyway.

@arderyp
Copy link
Contributor

arderyp commented Mar 4, 2024

thanks very much @derrabus, you're very simple addition to my pre-existing tests/bootstrap.php resolved the issue:

use Symfony\Component\ErrorHandler\ErrorHandler;
set_exception_handler([new ErrorHandler(), 'handleException']);

I agree with you that, while simple, it does seem somewhat like a hack. I am wondering if the symfony maintainers plan to take on your suggestion, perhaps by offering a new config option to opt out of the error handler that defaults to true in the test environment.

@JE4GLE
Copy link

JE4GLE commented Mar 5, 2024

Will this issue be resolved by symfony or is it expected to add @derrabus solution to every project?

@derrabus
Copy link
Member

derrabus commented Mar 5, 2024

Will this issue be resolved by symfony or is it expected to add @derrabus solution to every project?

This will be resolved by whomever resolves it. Could be you?

xarem added a commit to whatwedo/MonitorBundle that referenced this issue Mar 10, 2024
otherwise risky checks are warned

symfony/symfony#53812
@derrabus
Copy link
Member

What if we stopped registering error handlers, exception handlers or shutdown functions of any kind in FrameworkBundle?

We do need this stuff way before the framework is booted, so whatever FrameworkBundle registers here, it does it way too late. This applies especially to exception handlers: They are meant to handle uncaught exceptions. The kernel however runs in a big try/catch, so at the time FrameworkBundle is booted, the need for an exception handler is more or less over.

In modern Symfony applications, the error handling should be set up by the runtime already. All that FrameworkBundle does is connecting the logger properly. The logic that registers an error handler or exception handler is usually dormant and causes trouble because PHPUnit tests run outside of the runtime.

@stof
Copy link
Member

stof commented Mar 13, 2024

This would require deprecating the FrameworkBundle configuration settings that allow configuring the error handler though

@derrabus
Copy link
Member

This would require deprecating the FrameworkBundle configuration settings that allow configuring the error handler though

Would it? I mean, FWB can still connect to the error handler registered by the runtime and reconfigure it.

@flashx4u

This comment has been minimized.

@nickjbedford
Copy link

Yes, the issue still exists. And the workaround is documented in this thread.

I tried these fixes and they didn't fix the issue for me. I'm not sure why. Our codebase/test suite originates all the way from Laravel 5.8 so perhaps there's something there (though we've done our best to continuously modernise the codebase).

webignition added a commit to smartassert/sources that referenced this issue Sep 27, 2024
webignition added a commit to smartassert/sources that referenced this issue Sep 27, 2024
webignition added a commit to smartassert/job-coordinator that referenced this issue Oct 1, 2024
webignition added a commit to smartassert/job-coordinator that referenced this issue Oct 1, 2024
Kocal added a commit to Kocal/hugo.alliau.me that referenced this issue Oct 9, 2024
@arderyp
Copy link
Contributor

arderyp commented Oct 9, 2024

@nicolas-grekas thanks for the merge, but I am somewhat confused. The commit mentions it was merged into 5.4 branch 2 weeks ago, but the latest 5.4 release is 3 weeks old (5.4.44). And no indication as to whether the fix will also be merged into 6.4 and 7.x. Can you clarify, or am I by chance missing something? The latest framework-bundle 6.4.12 did not resolve this for me...

@xabbuh
Copy link
Member

xabbuh commented Oct 9, 2024

We regularly merge bugfixes up into all maintained branches which means that the fix will ship with the next patch releases of 5.4, 6.4 and 7.1.

@arderyp
Copy link
Contributor

arderyp commented Oct 9, 2024

Ahh okay, thanks!!

@alexislefebvre
Copy link
Contributor

alexislefebvre commented Oct 10, 2024

As xabbuh said, it has been merged to the 6.4 branch: 33050e3

And the last release on 6.4.* is older than this merge: https://github.com/symfony/symfony/releases?q=v6.4&expanded=true

So it's not yet available but it will come to 6.4 at some point, but I don't know the release process of the minor versions so I can't tell when.

@arderyp
Copy link
Contributor

arderyp commented Oct 11, 2024

Thank you both for your patience with my silly question. I should have know, and think I did, but just forgot how the versioned branches work. Thanks again :)

webignition added a commit to smartassert/worker-manager that referenced this issue Oct 15, 2024
webignition added a commit to smartassert/worker-manager that referenced this issue Oct 15, 2024
@garak
Copy link
Contributor

garak commented Oct 27, 2024

The patch is in the 6.4 branch now, but the problem remains.

@xabbuh
Copy link
Member

xabbuh commented Oct 27, 2024

@garak Can you then please create a small example application that allows to reproduce?

@mariusjp
Copy link

mariusjp commented Oct 28, 2024

Can confirm @garak 's statement, just updated my project to Symfony 7.1.6;

After removing the hotfix:

require \dirname(__DIR__) . '/vendor/autoload.php';

// @todo: Remove temporary fix for the error handler for Symfony 7 in combination with PHPUnit 11
/* @see https://github.com/symfony/symfony/issues/53812 */
\set_exception_handler([new ErrorHandler(), 'handleException']);

I'm getting the same error again:
"Test code or tested code did not remove its own exception handlers"

I'm running PHPUnit 11.4.2.

I believe my statement here is not fair, it's very specific to my project, I can't seem to reproduce the problem, see:
#53812 (comment)

@xabbuh
Copy link
Member

xabbuh commented Oct 28, 2024

@mariusjp see #53812 (comment)

@mariusjp
Copy link

@mariusjp see #53812 (comment)

Fair point, I've tried a few different scenarios. All are working fine, so can only conclude it's a library that we are using that is interfering. Already tried a few, still can't reproduce. I'll let you know when I find something!

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Oct 28, 2024

I did a short test by rebasing the @sulu branch for PHPUnit 11 interesting is that currently in case of Sulu only the @dev versions seems to work so maybe something is not yet release on PHPUnit or may Symfony side:

Update Something seems to block the framework bundle upgrade. Intersting that I get on @dev still the 7.2.0-dev:

Bildschirmfoto 2024-10-28 um 10 36 38

symfony/framework-bundle v7.1.6 conflicts symfony/runtime (<6.4.13|>=7.0,<7.1.6)

Found the issue in my case a update of matthiasnoback/symfony-dependency-injection-test was also needed it was downgrading some depndencies. composer outdated did help here. Maybe this helps others also.

@mmarton
Copy link

mmarton commented Oct 28, 2024

I've had similar issue that was caused by a workaround in zenstruck/foundry. Opened an issue there:
zenstruck/foundry#711

BacLuc added a commit to BacLuc/ecamp3 that referenced this issue Nov 23, 2024
As suggested in symfony/symfony#53812 (comment)

This seems like a temporary fix, until symfony has figured out
what to do with sebastianbergmann/phpunit#5619

Issue: ecamp#6407
@dozer111
Copy link

interesting.

this problem is absent on symfony 6.4, but exists in latest 7.2

gbirke added a commit to wmde/fundraising-application that referenced this issue Jan 9, 2025
We were held back by symfony/symfony#53812
The issue is that `FrameworkBundle` registers an exception handler (if
`ErrorHandler` was not registered before as an exception handler) and
never removes it when the Kernel shuts down. PHPUnit checks for newly
registered exception handlers at the end of each test, so whenever the
kernel boots, PHPUnit sees a "dangling" exception handlers and issues a
warning.

Now that the issue is closed with a documented workaround (but not a
proper solution in `FrameworkBundle`) the only way forward seems to be
the workaround. "Workaround" means registering the Symfony error handler
*before* starting PHPUnit, which will skip the additional registration
in `FrameworkBundle`.

Ticket: https://phabricator.wikimedia.org/T359971
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug FrameworkBundle Help wanted Issues and PRs which are looking for volunteers to complete them. Status: Reviewed
Projects
None yet