Use PHPUnit 9 for PHP 7.4+, while keeping support for PHPUnit 8.4 in PHP 7.3

Nice work!

+++ b/core/tests/Drupal/Tests/Traits/PHPUnit8Warnings.php
@@ -26,6 +26,14 @@
+    // PHPUnit 9.

We probably want to rename this trait to just e.g. PhpUnitWarnings now?

Changed as per #9

Instead of sorting in #18 can we improve the assertions? We know the array key in at least some of the cases, I think?

We don't need to sort the arrays or change MessengerTest - we can just use assertContainsEquals() which is less strict.

Upstream issue to follow for phpunit-bridge is https://github.com/symfony/symfony/issues/36499

Symfony fixed the issue on their side so I think we have to wait for new Symfony releases and then bump our dependency on the phpunit-bridge.

Symfony already added a conflict with PHPUnit 9.1.2 and kept a BC layer for <9.1.2 so I think we just have to bump the version of phpunit-bridge and we are all OK.

Works for me, let's see what core committers think.

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -317,7 +317,7 @@ public static function upgradePHPUnit(Event $event) {
-    // If the PHP version is 7.3 or above and PHPUnit is less than version 7
+    // If the PHP version is 7.4 or above and PHPUnit is less than version 9

@@ -339,7 +339,7 @@ public static function upgradePHPUnit(Event $event) {
-    return !(version_compare(PHP_MAJOR_VERSION . '.' . PHP_MINOR_VERSION, '7.3') >= 0 && version_compare($phpunit_version, '7.0') < 0);
+    return !(version_compare(PHP_MAJOR_VERSION . '.' . PHP_MINOR_VERSION, '7.4') >= 0 && version_compare($phpunit_version, '9.0') < 0);

+++ b/core/scripts/run-tests.sh
@@ -149,7 +149,7 @@
-  simpletest_script_print_error("PHPUnit testing framework version 7 or greater is required when running on PHP 7.3 or greater. Run the command 'composer run-script drupal-phpunit-upgrade' in order to fix this.");
+  simpletest_script_print_error("PHPUnit testing framework version 9 or greater is required when running on PHP 7.4 or greater. Run the command 'composer run-script drupal-phpunit-upgrade' in order to fix this.");

I'm not a huge fan of this. PHPUnit 8 supports PHP 7.4 so we're enforcing an upgrade that's not necessary.

+++ b/composer.json
@@ -21,7 +21,7 @@
-        "phpunit/phpunit": "^8.4.1",
+        "phpunit/phpunit": "^8.4.1 || <9.1.2",

+++ b/composer/Metapackage/DevDependencies/composer.json
@@ -16,7 +16,7 @@
-        "phpunit/phpunit": "^8.4.1",
+        "phpunit/phpunit": "^8.4.1 || ^9",

This doesn't look right.

I think I agree that until we have a PHP 8 to test on, testing PHP 7.3 on PHPUnit 8 and PHP 7.4 on PHPUnit 9 seems like a good compromise. Therefore the code in Drupal\Core\Composer\Composer::upgradePHPUnit does the right thing to get both of these tested on the testbot. However, the message in run-tests.sh is not strictly true - PHPUnit 9 is not strictly required on PHP 7.4, maybe we need to reword this?

The other option is just postpone this entirely until PHP 8 forces us to upgrade, I guess. We have still fixed all the deprecations to date - having said that there may be more to come, and it would be good to keep on top of them.

This is a critical tasks because it blocks PHP 8 adoption.

Rerolled... git took care of it with a 3 way merge.

Re-rolled patch please review!

Warning: Your XML configuration validates against a deprecated schema.
Suggestion: Migrate your XML configuration using "--migrate-configuration"!

Should we look to fix this here?

Ah yeah, it will still give the warning if the XML doesn't validate, and we need to keep the old elements for BC, so we might as well wait until we have a minimum of PHPUnit 9.

I tried this locally anyway and ran into https://github.com/sebastianbergmann/phpunit/issues/4419

In which case back to RTBC on this, and core committers can decide whether we only require PHPUnit 9 in PHP 8 or allow it in 7.4.

@catch brought this up to the committer team -- I'd like to have a think about it, so tagging for RM review.

The test fail on #66 seems to show we have a problem on PHP 7.3?

Also I think we should block this on #3142267: Allow the full list of deprecations from PHPUnit8Warning to respect the --suppress-deprecations behavior on DrupalCI - that changes the PHPUnit8Warnings class to trigger deprecation messages so we then only have to maintain a single list of technical debt in \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations() - at the very least this patch should make the same change.

Patch #66 didn't apply anyomore on Drupal 9.1.x, so here I have added reroll of patch #66.

Wow, this resolves half of the 29 dependency issues when attempting to make Drupal 9 core compatible with PHP 8. (#3109885: [meta] Ensure compatibility of Drupal 9 with PHP 8.0 (as it evolves))

Blocker commited

To update the hash you can run composer update --lock and it will update the hash but leave the packages alone.

+++ b/core/modules/migrate/tests/src/Unit/MigrateStubTest.php
@@ -22,6 +23,8 @@
 class MigrateStubTest extends TestCase {

This should be extending use Drupal\Tests\UnitTestCase; and not the Phpunit class. It's a test in a module and there's no reason to not extend the Drupal base class.

I think it is in scope - the changes necessary to the test system show that that test is not inheriting from the correct class.

I've reviewed all the component test changes are it looks like a few changes can be made.

1. +++ b/core/tests/Drupal/Tests/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumperTest.php
   @@ -8,6 +8,8 @@
   +  use Drupal\Tests\Traits\PhpUnitWarnings;
   
   @@ -24,6 +26,9 @@
   +    use PhpUnitWarnings;
   

   This is not necessary here.

2. +++ b/core/tests/Drupal/Tests/Component/Gettext/PoStreamWriterTest.php
   @@ -4,6 +4,7 @@
   +use Drupal\Tests\Traits\PhpUnitWarnings;
   
   @@ -14,6 +15,8 @@
   +  use PhpUnitWarnings;
   
   +++ b/core/tests/Drupal/Tests/Component/Plugin/PluginManagerBaseTest.php
   @@ -5,6 +5,7 @@
   +use Drupal\Tests\Traits\PhpUnitWarnings;
   
   @@ -13,6 +14,8 @@
   +  use PhpUnitWarnings;
   

   Should be use PhpUnitCompatibilityTrait; instead.

3. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
   @@ -133,6 +133,18 @@ public static function getSkippedDeprecations() {
   +      'PHPUnit\\Framework\\TestCase::prophesize() is deprecated and will be removed in PHPUnit 10. Please use the trait provided by phpspec/prophecy-phpunit.',
   
   +++ b/core/tests/Drupal/Tests/Traits/PhpUnitWarnings.php
   @@ -37,6 +37,17 @@
   +    'PHPUnit\\Framework\\TestCase::prophesize() is deprecated and will be removed in PHPUnit 10. Please use the trait provided by phpspec/prophecy-phpunit.',
   

   Now we're using the trait I don't think we need this message here anymore.

For #94

we can remove from the deprecation silencers, but not from the PhpUnitWarnings trait unless we accept contrib to fail with PHPUnit warnings.

First thought was good point. But then I realised that our base test classes are all using the compatibility trait and therefore we should be able to remove it. We don't support contrib / custom not extending from our base classes.

It looks like all review points have been addressed.

Re #102 I don't think that is clear actually. PHPUnit 9 is not necessary for testing on PHP 7.4 - it is for PHP 8.0. We don't require PHPUnit 9 to run tests on PHP 7.4 - if you use the phpunit runner.

Looks like a random JS fail

+++ b/core/modules/system/tests/src/Kernel/Entity/EntityReferenceSelectionReferenceableTest.php
+++ b/core/modules/system/tests/src/Kernel/Entity/EntityReferenceSelectionReferenceableTest.php
@@ -126,7 +126,7 @@ public function testReferenceablesWithNoLabelKey($match, $match_operator, $limit

@@ -126,7 +126,7 @@ public function testReferenceablesWithNoLabelKey($match, $match_operator, $limit
       // entity labels.
       // @see \Drupal\Core\Entity\EntityReferenceSelection\SelectionInterface::getReferenceableEntities()
       $item = is_string($item) ? Html::escape($item) : $item;
-      $this->assertContains($item, $referenceables[$this->bundle]);
+      $this->assertContainsEquals($item, $referenceables[$this->bundle]);
     }

We probably could have done this in a spin-off issue to make the diff here smaller, but it doesn't really hurt. The method is already available to contrib modules to do the same thing so not much we can do about it ourselves.

This is fine. That issue changed the path reference hash - it's not the same thing.

#3110972: Update easyrdf library to 1.0.0 has now changed the content hash :)

Went over this one more time with a release manager eye, and decided we need a change record, which I've added here: https://www.drupal.org/node/3176567

I think this is in good shape and should not disrupt many contrib tests, if at all, as well as us having no choice due to PHPUnit and PHP's release cycle. So... committed 74fbb0a and pushed to 9.1.x. Thanks!

Forgot to say there are a few changes for @expectedDeprecation vs. ::expectDeprecation() here, however I left them out of the change record because we're already addressing those changes in #3172438: Since symfony/phpunit-bridge 5.1: Using "@expectedDeprecation" annotations in tests is deprecated, use the "ExpectDeprecationTrait::expectDeprecation()" method instead..

hmm I was going off #3127141-18: Use PHPUnit 9 for PHP 7.4+, while keeping support for PHPUnit 8.4 in PHP 7.3, maybe #3127141-19: Use PHPUnit 9 for PHP 7.4+, while keeping support for PHPUnit 8.4 in PHP 7.3 is the better one?

I'm not clear from reading this issue why we had to keep using PHPUnit 8 on PHP 7.3, but whatever the reason was, I wonder if it's been addressed since then in later releases of PHPUnit 9, so I opened #3217451: [TESTING ONLY; DO NOT COMMIT] Update to PHPUnit 9.