@karser On DeprecationErrorHandler:L111 there is also a ::handleError-call

@karser On DeprecationErrorHandler:L111 there is also a ::handleError-call

True. Do we need this autoload juggling? Didn't we already define which ErrorHandler class to use on line 76?

static $autoload = true;

$ErrorHandler = class_exists('PHPUnit_Util_ErrorHandler', $autoload) ? 'PHPUnit_Util_ErrorHandler' : 'PHPUnit\Util\ErrorHandler';
$autoload = false;

@smoench Fixed. callPhpUnitErrorHandler is now static so it can be called from set_error_handler closure.

Not sure how to deal with these booleans. In TestResult class they are all true by default

this is a critical blocker :(

we need to completely redesign the way we hook there to collect deprecations.
until 8.2, phpunit used global state and it was easy to hook, albeit not than clean I realize

now we need to do this in our listener: when TestResult calls startTest in its run method, we need to register phpunit's error handler (duplicating what it does just after the call to startTest) and then add our collector. Then disable at endTest.

Willing to dive there @karser? Maybe with help from @greg0ire too?

Note that we need to do this for 3.4 and for 4.3 (since our code changed so much between the two)

@nicolas-grekas I'm not fully following what happens in phpunit bridge, so I'll ask some stupid questions:

• Does DeprecationErrorHandler do anything other than collecting deprecations? So it means that DeprecationErrorHandler will be entirely moved to the phpunit listener?
• There is Symfony\Bridge\PhpUnit\SymfonyTestsListener, so should we use this listener?
• TestListener interface is deprecated (still works though) in phpunit 8. Should we consider TestHook interface?

Does DeprecationErrorHandler do anything other than collecting deprecations?

it copes only with that responsbility yes

So it means that DeprecationErrorHandler will be entirely moved to the phpunit listener?

It looks like so. Currently, it doesn't need to be registered as a listener, but it looks like phpunit 8 changes that.

There is Symfony\Bridge\PhpUnit\SymfonyTestsListener, so should we use this listener?

I'd say yes, one listener is easier to setup for users

TestListener interface is deprecated (still works though) in phpunit 8. Should we consider TestHook interface?

yes, although it could be done in another PR if that's easier?

@nicolas-grekas Ok, I'll try to do what you described, let's see how far I can get.
Sure, I'll do that in another PR, because this one already solves the problem, though not prefect.

Another alternative might be to check how $convertDeprecationsToExceptions is defined, and maybe duplicate the logic that turns it to true/false. That would be the easiest, if doable.

As of now, I wouldn't recommend merging the PR, as putting all these settings at true is a bug that someone will report sooner than later.

I'll do the research first. Looks like these variables were introduced only in 8.3 and didn't exist in 8.2

Maybe we can have a look at the stack trace and read the properties from there? It looks like we might find the TestResult instance there, with all the state we need?

TestResult is created by TestRunner which passes $arguments which contain the neccessary parameters.

$arguments are created by TestRunner::handleConfiguration. Here is simpllified function which does exactly what we need:

    private function handleConfiguration(array &$arguments): void
    {
        if (isset($arguments['configuration']) &&
            !$arguments['configuration'] instanceof Configuration) {
            $arguments['configuration'] = Configuration::getInstance(
                $arguments['configuration']
            );
        }

        if (isset($arguments['configuration'])) {
            $arguments['configuration']->handlePHPConfiguration();

            $phpunitConfiguration = $arguments['configuration']->getPHPUnitConfiguration();

            if (isset($phpunitConfiguration['convertDeprecationsToExceptions']) && !isset($arguments['convertDeprecationsToExceptions'])) {
                $arguments['convertDeprecationsToExceptions'] = $phpunitConfiguration['convertDeprecationsToExceptions'];
            }

            if (isset($phpunitConfiguration['convertErrorsToExceptions']) && !isset($arguments['convertErrorsToExceptions'])) {
                $arguments['convertErrorsToExceptions'] = $phpunitConfiguration['convertErrorsToExceptions'];
            }

            if (isset($phpunitConfiguration['convertNoticesToExceptions']) && !isset($arguments['convertNoticesToExceptions'])) {
                $arguments['convertNoticesToExceptions'] = $phpunitConfiguration['convertNoticesToExceptions'];
            }

            if (isset($phpunitConfiguration['convertWarningsToExceptions']) && !isset($arguments['convertWarningsToExceptions'])) {
                $arguments['convertWarningsToExceptions'] = $phpunitConfiguration['convertWarningsToExceptions'];
            }
        }
        $arguments['convertDeprecationsToExceptions']                     = $arguments['convertDeprecationsToExceptions'] ?? true;
        $arguments['convertErrorsToExceptions']                           = $arguments['convertErrorsToExceptions'] ?? true;
        $arguments['convertNoticesToExceptions']                          = $arguments['convertNoticesToExceptions'] ?? true;
        $arguments['convertWarningsToExceptions']                         = $arguments['convertWarningsToExceptions'] ?? true;
    }

I extracted this logic into ErrorHandlerCallerV83. It obtains the necessary variables the way PHPUnit does and instantiates ErrorHandler. I made sure that these variables are changed when I modify them in phpunit.xml. it works with both v8.2 and v8.3.
@nicolas-grekas WDYT?

Maybe in another PR to keep this one as is: could you please give a try to the stacktrace-based approach? I feel like it could be simpler and more robust.

Indeed, using debug_backtrace is MUCH easier approach. I didn't know that it's possible to get to TestResult object this way. The only issue - what to do if TestResult instance is missing in the stacktrace? Throw an exception then?

We should then call the previous handler if one was set, or return false

Here is stacktrace-based PR #32933