sentry-php icon indicating copy to clipboard operation
sentry-php copied to clipboard

Add a way to cleanup error and exception handlers

Open simPod opened this issue 1 year ago • 5 comments

Problem Statement

PHPUnit 11 checks for any leftovers in error handlers https://github.com/sebastianbergmann/phpunit/pull/5619

Solution Brainstorm

When \Sentry\ErrorHandler::registerOnceErrorHandler() is called, error handler is registered.

It would be nice to add degisterOnceErrorHandler(). Since the error handler is kept in ErrorHandler's state, it would be easy to deregister it.

simPod avatar Feb 07 '24 08:02 simPod

That makes sense, let me try to use PHPUnit 11 in the SDK tests and see what happens 😄

cleptric avatar Feb 07 '24 12:02 cleptric

Can I ask why you would want this?

Sentry shouldn't be active when running tests ideally and our own test suite is not going to be ready for PHPUnit 11 for some time probably. If you do load Sentry in your tests isn't it better to prevent loading the (fatal) error handler integrations so they don't register in the first place instead of de-registering them afterwards which I'm assuming needs to be done manually by the user in their own code anyway?

There is no method to de-register a specific error handler so by the time a degisterOnceErrorHandler is called other handler could have been registered causing us to remove other handlers in the process.

stayallive avatar Feb 20 '24 09:02 stayallive

When doing full app integration tests I want to simulate the same environment as is in prod in order to test exactly the same thing that runs there.

simPod avatar Feb 20 '24 09:02 simPod

And how would you have setup the degisterOnceErrorHandler in your test suite if it existed?

Is not registering the error handler integrations not an good compromise here? I'm assuming your integration tests don't test for unhandled or fatal errors caught by Sentry? Or do they?

stayallive avatar Feb 20 '24 09:02 stayallive

Is not registering the error handler integrations not an good compromise here? I'm assuming your integration tests don't test for unhandled or fatal errors caught by Sentry? Or do they?

Sure but then my tests skip some part of code that get's executed in prod and therefore do not cover it.


Currently my workaround in teardown looks similar to this:

        $res = [];

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

            if ($previousHandler instanceof Closure) {
                $reflection = new ReflectionFunction($previousHandler);
                if (
                    $reflection->getClosureThis() instanceof \Sentry\ErrorHandler
                    && $reflection->getName() === '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);
        }

as imperfect as it may be, it allowed me to run my tests with phpunit 11 without compromising coverage.

simPod avatar Feb 22 '24 09:02 simPod

@cleptric what is the sentry's statement on this?

simPod avatar May 15 '24 07:05 simPod

What @stayallive said.

cleptric avatar May 15 '24 08:05 cleptric