Compatibility with PHPUnit 8+

[marcovtwout]

Since PHPUnit 8 PHPUnit\Framework\TestCase::setUp() has a void return type added: https://phpunit.de/announcements/phpunit-8.html

This method is extended by CDbTestCase and CWebTestCase:
https://github.com/yiisoft/yii/blob/master/framework/test/CDbTestCase.php#L114
https://github.com/yiisoft/yii/blob/master/framework/test/CDbTestCase.php

Running tests on PHP7.0+ PHPUnit 8+ throws a fatal error:

PHP Fatal error:  Declaration of CDbTestCase::setUp() must be compatible with PHPUnit\Framework\TestCase::setUp(): void in (..)

On PHP 7.x you can stay on PHPUnit version 7 as a workaround.
However PHP 8 requires a minimum of PHPUnit version 8.

The most straightforward fix is to add the void return type to CDbTestCase and CWebTestCase, but this breaks backward compatibility with PHP < 7.1 and PHPUnit < 8.

Any ideas on how to patch this in a straightforward but backward compatible manner?

[samdark]

We are running tests with older PHPUnit, patched version: https://github.com/yiisoft/yii/blob/master/composer.json#L101 PHP 8 is already checked and pass: https://github.com/yiisoft/yii/runs/2676270735

[marcovtwout]

Keeping PHPUnit 4 is acceptable for running framework unit tests, but for user projects I think it's better to support newer PHPUnit versions if possible. For example, anyone using Codeception with PHP 8 cannot stay on PHPUnit 4 and must upgrade to a much more recent version.

Backward compatibility is not as bad as I originally thought: adding void is backward compatible with older PHPUnit Versions. So the only BC break is with PHP 7.0 or below (EOL 01-2019). And this should not break production code as these two classes are only used in tests.

I think in this case supporting the newer versions might outweigh the BC break. What do you think?

[samdark]

And this should not break production code as these two classes are only used in tests.

Well, it is likely that it will affect someone's tests.

I think in this case supporting the newer versions might outweigh the BC break. What do you think?

It could be done with autoload map that is PHP-version specific instead.

[rob006]

W could use different name (namespaced maybe?) for new base classes for tests. Reusing the same FQCL for different implementations will confuse SCA tools and will create problems if someone is not using Yii autoloader.

[marcovtwout]

On a related note, running tests on PHPUnit 6+ also triggers an error where legacy autoloading code is performed when "PHPUnit_Runner_Version" doesn't exist.
https://github.com/yiisoft/yii/blob/master/framework/test/CTestCase.php#L11

That if-statement should probably read:
if(!class_exists('PHPUnit_Runner_Version') && !class_exists('PHPUnit\Runner\Version')) {

(I didn't notice this earlier since one of my common project dependencies already applied a workaround similar to https://github.com/yiisoft/yii/blob/master/tests/compatibility.php#L16-L19)

[marcovtwout]

If anyone is looking for a quick workaround, here's what I'm using in a patch file for the time being:

// Compatibility fix for CTestCase and PHPUnit 7+
if (!class_exists('PHPUnit_Runner_Version') && class_exists('PHPUnit\Runner\Version')) {
    class_alias('\PHPUnit\Runner\Version', 'PHPUnit_Runner_Version');
}

// Compatibility fix for CTestCase and PHPUnit 8+
// @see https://github.com/yiisoft/yii/issues/4366
$yiiFiles = [
    __DIR__ . '/vendor/yiisoft/yii/framework/test/CDbTestCase.php',
    __DIR__ . '/vendor/yiisoft/yii/framework/test/CWebTestCase.php',
];
foreach($yiiFiles as $file) {
    if (file_exists($file)) {
        file_put_contents($file, str_replace(
            "protected function setUp()",
            "protected function\n\tsetUp(): void",
            file_get_contents($file)
        ));
    }
}

[marcovtwout]

Some thoughts about the possible alternative solutions:

1. We could define a second set of classes:
   • They could use the same name and need overrides in $classMap (https://github.com/yiisoft/yii/blob/master/framework/YiiBase.php#L68). But having duplicate classes will probably hinder IDE autocompletion and other problems @rob006 describes
   • They could be in a separate namespace. That would require some changes in core code (calls to Yii::import('system.test.*'); in yiit.php and in framework/test/) and users will have to update their code to extend the new base classes.
   • In both cases we have duplicate code (and documentation) which doesn't feel very nice.
2. Alternatives with class_alias or dynamically generating code will have the same problems as above under nr 1.
3. Code could be patched like in the workaround above, but not all user situations will support writing to framework folder and IDE will still throw warnings.

Comparing pros and cons, I think the best way forward is to add : void to the base classes. Yes it will break BC for users running PHP 7.0 or below, but only when running tests. And it requires the least amount of changes for those users that keep PHP up to date.

This is not an easy issue to decide on so I suggest to keep it open for now to gather some more community response.

[cebe]

@marcovtwout your patch could be applied by a composer plugin running on composer install which checks which version of phpunit is installed...

[Justinas-Jurciukonis]

@marcovtwout Any updates when Yii will support 8.1? Or even 8.2?

[marcovtwout]

8.1 support is available in master, see https://github.com/yiisoft/yii/blob/master/CHANGELOG#L7