Open marcovtwout opened 3 years ago
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
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?
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.
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.
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)
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)
));
}
}
Some thoughts about the possible alternative solutions:
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.
@marcovtwout your patch could be applied by a composer plugin running on composer install which checks which version of phpunit is installed...
@marcovtwout Any updates when Yii will support 8.1? Or even 8.2?
8.1 support is available in master, see https://github.com/yiisoft/yii/blob/master/CHANGELOG#L7
a little addition to @marcovtwout answer
if (!class_exists('PHPUnit_Framework_TestCase') && class_exists('PHPUnit\Framework\TestCase')) {
class_alias('\PHPUnit\Framework\TestCase', 'PHPUnit_Framework_TestCase');
}
A few relevant updates about extended support and keeping things backward compatible:
I will not state anything, but in my opinion Yii 1.1 projects that might be concerned with this change do not use composerised setup and probably will not upgrade anyway or they are like dead projects until their OS or yii projects will get hacked and they will begin to concern themselves with problems that they are facing and then start upgrading.
I think that best thing here would be to add void and create ruleset for tool like rectorphp to make conversion for legacy code if does not already and do the change. And write a blog post about it on how to migrate tests to latest version or composer upgrade could automatically trigger some script which would do that transformation for people concerned and having legacy code.
This issue has been open for a while, seems like a good time to revisit our stance. I would still propose to apply the solution suggested in https://github.com/yiisoft/yii/issues/4366#issuecomment-852960838 which raises the minimum PHP version to 7.0 for those running unit tests.
I would suggest upgrading to PHP 7.3
, so it could support PHP 8.4
and PHPUnit 9.6
.
@terabytesoftw What changes exactly are needed in yii framework code to support PHPUnit 9.6?
@terabytesoftw What changes exactly are needed in yii framework code to support PHPUnit 9.6?
Well, basically it is eliminating the depreciated methods with the new methods, and eliminating the patches, it is not complicated.
Since PHPUnit 8
PHPUnit\Framework\TestCase::setUp()
has avoid
return type added: https://phpunit.de/announcements/phpunit-8.htmlThis method is extended by
CDbTestCase
andCWebTestCase
: https://github.com/yiisoft/yii/blob/master/framework/test/CDbTestCase.php#L114 https://github.com/yiisoft/yii/blob/master/framework/test/CDbTestCase.phpRunning tests on PHP7.0+ PHPUnit 8+ throws a fatal error:
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.1and PHPUnit < 8.Any ideas on how to patch this in a straightforward but backward compatible manner?