PHPLIB-462: ensure compatibility with PHP 7.4 #665
Conversation
alcaeus
force-pushed
the
php-7.4-compat
branch
from
1453ef9 to
e0c6531
Compare
alcaeus
changed the title
There was a problem hiding this comment.
symfony/symfony#33078 appears to have been merged already.
Was that related to using ^4.4@dev in composer.json, or do we still need that until a tagged release is made upstream?
A couple of other questions, but changes LGTM.
| backupStaticAttributes="false" | ||
| xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/8.3/phpunit.xsd" | ||
| beStrictAboutOutputDuringTests="true" | ||
| beStrictAboutChangesToGlobalState="true" |
There was a problem hiding this comment.
Am I correct that this option was introduced in 4.6 (see: blog post), even though it was never documented? I'm not sure about beStrictAboutOutputDuringTests, but there are some online references to that dating back to 2015 so I assume that's fine.
Just wanted to confirm that these will apply to all PHPUnit versions we use for testing.
There was a problem hiding this comment.
The XSD option was added in sebastianbergmann/phpunit@9101f51#diff-365e6053dce410a51198fc699549f0ae, which was first included in 4.0.0. The comments in the code indicate that the functionality itself was added in 3.8.0, so we should be safe here.
The reason this was added is that I regenerated a new configuration file using phpunit --generate-configuration to account for changes in the default configuration.
| */ | ||
| trait PolyfillAssertTrait | ||
| { | ||
| use SymfonyPolyfillAssertTrait; |
There was a problem hiding this comment.
Is there a particular reason that PolyfillAssertTrait doesn't have a compat shim, like SetUpTearDownTrait?
Is that because it really is only relevant to older PHPUnit versions, so Symfony is careful not to use it in newer branches?
There was a problem hiding this comment.
I talked to @nicolas-grekas about this. The assertion polypill is marked as @internal because they only use it to patch PHPUnit classes in their simple-phpunit binary: https://github.com/symfony/symfony/blob/a9ace363899373f4fed96f32cd38fc94187666d2/src/Symfony/Bridge/PhpUnit/bin/simple-phpunit.php#L154..L157. With the myriad of PHPUnit versions Symfony needs to support, it's too risky for them to have a single trait and let people use that in classes extending from PHPUnit\Framework\Assert, as it would then override the methods already shipped by PHPUnit. Patching the original class works around this issue as the code in PHPUnit\Framework\Assert then overrides the trait code.
In our case, I didn't want to patch PHPUnit code and with the PHPUnit versions we currently support, the single trait is good enough for us as all new assertion methods were added in or after PHPUnit 7. That's why we declare our own trait, which is empty if the PHPUnit version is new enough, as it allows us to always include our polyfill trait in the test classes.
There is still a certain risk using internal code from Symfony, but given that they need to keep the trait around as long as they support PHPUnit < 7 I feel rather confident there: dropping older PHPUnit versions for them won't be possible until Symfony 4.4 is EOL, which will be in November 2023. I hope we'll be able to get rid of support for PHP 5 before then. However, if you'd rather not use an internal class we can always duplicate the polyfill to our library, of course keeping the original copyright intact.
There was a problem hiding this comment.
Or you could use simple-phpunit :)
But I agree, the risk is low.
There was a problem hiding this comment.
Thanks for the deep explanation! I'm content with this.
@nicolas-grekas: Thanks for chiming in. simple-phpunit looks interesting (first I've seen of it) and is probably something (along with the bridge) that I could have benefited from a good while back 😄. I'll defer to @alcaeus about whether it's worth migrating over to that at some later point.
| private $expectedCollection; | ||
|
|
||
| public function setUp() | ||
| private function doSetUp() |
There was a problem hiding this comment.
Just to confirm, can we still chain to parent::setUp() below despite this method being renamed to doSetUp()?
There was a problem hiding this comment.
Yes, that is still possible. The parent::setUp() call in doSetUp will always go to the parent class. In this case, the parent (FunctionalTestCase) also includes the trait, so the call will again land in the shim and call the doSetUp method that is written in FunctionalTestCase. Chaining the setup/teardown methods is still possible as before, we just have to remember to use the polyfill trait and write doSetUp methods until we can get rid of PHPUnit < 8.
There was a problem hiding this comment.
I confirm: the trait has been designed so that only the signature of the method is changed, and not its body.
Yes, I originally included the code I PR'ed to Symfony in this library, but now that it has been merged we no longer need to include it ourselves.
Symfony 4.4 will be released in November, so we have to use an |
Please make a PHPLIB ticket as a reminder to do so (if you haven't already). |
|
Created PHPLIB-465 to track changing the dev requirement after the release of Symfony 4.4. |
While we're not going to be running any PHP 7.4 features for a while, we at least want to make sure that we test the library on PHP 7.4. This requires upgrading to newer PHPUnit releases to avoid errors due to the deprecation of the
__toStringmethods in reflection classes.Upgrading to PHPUnit 7 or newer is non-trivial, as support for PHP 5 was dropped and PHPUnit introduces type hints. To cope with that, we use
symfony/phpunit-bridgeto provide compatibility layers for theTestCaseclass. Once symfony/symfony#33078 is merged, we can also drop the constraint traits from this packages. However, since the traits are marked as@internalwe don't need to wait for a merge of that pull request and can drop them once they're available insymfony/phpunit-bridge.