Allow the full list of deprecations from PHPUnit8Warning to respect the --suppress-deprecations behavior on DrupalCI

Adding a related issue #3027952: [Plan] Remove the usage of deprecated methods in tests which might get affected a little.

How about we put then in a new list of warnings and use the SYMFONY_DEPRECATIONS_HELPER flag and ignore them if it is equal to disabled or the test has the legacy group?

That way a project can move forward quickly without having to fix these things. And it'll make it easier to prep 8.9.x for PHPUnit 9.

+++ b/core/tests/Drupal/Tests/Traits/PHPUnit8Warnings.php
@@ -18,25 +18,65 @@
+      if (!('disabled' !== getenv('SYMFONY_DEPRECATIONS_HELPER'))) {

Double negative here is a bit hard to read, this can just be === I think?

Otherwise this seems like a good approach.

The proposed resolution seems good here, although definitely agreed this should only happen in 9.0.x so that they start getting surfaced again in 9.1.x

Re #18 yeah doing that feels wrong. We could also use reflection to inspect the warning private and then reset it which is also wrong. It is testable but we'd need to test in the same way that PHPUnit tests itself for end-to-end testing. To do this we'd need to support .phpt tests. Then we could make the test something like:

  /**
   * Tests not skipping PHPUnit deprecation warnings.
   *
   * @see Drupal\Tests\Traits\PHPUnit8Warnings
   */
  public function testNotSkippingPhpUnit8Warnings() {
    if (!getenv('SkippedDeprecationTestWarning')) {
      $this->markTestSkipped('This test is only triggered by core/tests/Drupal/Tests/PhpUnitWarnings.phpt as it tests PHPUnit itself.');
    }
    $original_setting = getenv('SYMFONY_DEPRECATIONS_HELPER');

    // Ensure the deprecation helper is enabled regardless of the configured
    // setting.
    putenv('SYMFONY_DEPRECATIONS_HELPER=');

    // The warning that core fails should still be silenced.
    $this->addWarning('Warning for \\Drupal\\Tests\\Traits\\PHPUnit8Warnings::$coreFailingWarnings');
    // The warning that core does not fail should be flagged.
    // @todo This doesn't work.
    $this->addWarning('Warning for \\Drupal\\Tests\\Traits\\PHPUnit8Warnings::$corePassingDeprecationsWarnings');

    putenv("SYMFONY_DEPRECATIONS_HELPER=$original_setting");
  }

And then add a file called core/tests/Drupal/Tests/SkippedDeprecationTest.php which contains something like:

--TEST--
phpunit warnings test
--FILE--
<?php
putenv('SkippedDeprecationTestWarning=1');
$_SERVER['argv'][1] = '--no-configuration';
$_SERVER['argv'][3] = __DIR__ . '/SkippedDeprecationTest.php';
$_SERVER['argv'][4] = '--filter';
$_SERVER['argv'][5] = 'testNotSkippingPhpUnit8Warnings';

require __DIR__ . '/../../bootstrap.php';
PHPUnit\TextUI\Command::main();
--EXPECTF--
PHPUnit %s by Sebastian Bergmann and contributors.

W                                                                   1 / 1 (100%)

Time: %s ms, Memory: %s MB

There was 1 warning:

1) Drupal\Tests\SkippedDeprecationTest::testNotSkippingPhpUnit8Warnings
Warning for \Drupal\Tests\Traits\PHPUnit8Warnings::$corePassingDeprecationsWarnings

%s
%s
%s
%s
%s

WARNINGS!
Tests: 1, Assertions: 0, Warnings: 1.

I'll open a follow-up to discuss adding support for this type of test. PHPunit will run .phpt tests but our \Drupal\Core\Test\TestDiscovery very much only works with classes and makes a series of assumptions.

This looks RTBC to me apart from a minor nit - and something that might need a followup issue with the coder module.

+++ b/core/tests/Drupal/Tests/Traits/PHPUnit8Warnings.php
@@ -18,25 +18,69 @@
+   *
+   * @return void

Unfortunately this fails our PHPCS check with

FILE: ...upal8alt.dev/core/tests/Drupal/Tests/Traits/PHPUnit8Warnings.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 66 | ERROR | If there is no return value for a function, there must
    |       | not be a @return tag.
    |       | (Drupal.Commenting.FunctionComment.VoidReturn)
----------------------------------------------------------------------

+++ b/core/tests/Drupal/Tests/Traits/PHPUnit8Warnings.php
@@ -18,25 +18,69 @@
+      // Return early only if deprecation errors are specifically suppressed.
+      if (getenv('SYMFONY_DEPRECATIONS_HELPER') === 'disabled') {

I considered whether we should add something like

      $method = $this->getName(FALSE);
      if (strpos($method, 'testLegacy') === 0
        || strpos($method, 'provideLegacy') === 0
        || strpos($method, 'getLegacy') === 0
        || strpos(get_class($this), '\Legacy')
        || in_array('legacy', Test::getGroups(get_class($this), $method), TRUE)) {
        // This is a legacy test...
      }

To detect whether the warning is occurring in a legacy test but I think only using the SYMFONY_DEPRECATIONS_HELPER is the way to go here. As I'm not sure what we should do in this case. Like should we be converting the warning into an @trigger_error? Feels like extra complexity. And this change has the desired effect. Contrib and custom can test without this tripping them up and the y can also test and see the warning - all with the convenience of using the same flag that we already use to surface deprecations in the test system.

Actually maybe that suggests a neater way of approaching the whole issue. We could convert the entire original list into deprecation errors (i.e. @trigger_error('blah', E_USER_DEPRECATED)) and then in \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations() skip the one core still triggers. That's still allow contrib/custom to ignore these with SYMFONY_DEPRECATIONS_HELPER but they'd be handled like all the other deprecations we have. It would also mean less code paths and simpler testing.

Well the advantage of using @trigger_error is that we have a single logic path so we don't have to add

+++ b/core/tests/Drupal/Tests/Traits/PHPUnit8Warnings.php
@@ -18,25 +18,67 @@
+    // Test assertion.
+    'Warning for \\Drupal\\Tests\\Traits\\PHPUnit8Warnings::$coreFailingWarnings',

For testing purposes because the list will never change as we won't need to change it. Also it means that all code paths are testable with our current infrastructure. I think less logic and more testable is a good reason to do something.

Another advantage of converting to @trigger_error() is support for ignoring the deprecations where you are doing deprecations in legacy tests.

+1 in the idea to convert the warning into a @trigger_error.

Also I think that we should not override addWarning in the trait, since it’s a PHPUnit internal: https://github.com/sebastianbergmann/phpunit/blob/4dc5717d74120d27d7dc37...

IMHO the correct way should be somehow to intercept the added warning in a test listener ... but I have not checked if possible :)

@mondrake I don't think

IMHO the correct way should be somehow to intercept the added warning in a test listener

is possible. The way warnings work in PHPUnit occurs outside anything that could catch the exception it results in. See \PHPUnit\Framework\TestCase::runBare() and once $listener->addWarning() is being called in \PHPUnit\Framework\TestResult::addWarning() the test is already marked as failing.

@alexpott thanks for feedback. It's a pity, though. Need to keep a watch on what PHPUnit 10 will bring in terms of replacing the listeners.

One thing I don't get is why these switches exist:

$ vendor/bin/phpunit --help|grep warning
  --stop-on-warning           Stop execution upon first warning
  --fail-on-warning           Treat tests with warnings as failures

It seems like fail-on-warning is happening by default, but I can't see where, or how we can turn it off again.

+++ b/core/tests/Drupal/Tests/Core/Test/PhpUnitBridgeTest.php
@@ -21,6 +21,18 @@ public function testDeprecatedClass() {
+   * @see Drupal\Tests\Traits\PHPUnit8Warnings

@see should be fully qualified, ie. with a leading backslash.

Just FYI, not sure if it's relevant here:

\Drupal\Tests\Listeners\DrupalListener implements a deprecated interface TestListener

And a link: Make TestHook interfaces as easy to use as TestListener #3390

# \PHPUnit\Framework\TestListener

/**
 * @deprecated Use the `TestHook` interfaces instead
 */
interface TestListener

Another advantage of changing to deprecations is that we can actually use the deprecated in a method in a test rather than adding

+++ b/core/tests/Drupal/Tests/Traits/PHPUnit8Warnings.php
@@ -18,25 +18,52 @@
+    // Warnings for testing.
+    'Skipped warning for \\Drupal\\Tests\\Traits\\PHPUnit8Warnings::$deprecationWarnings',
+    'Un-skipped warning for \\Drupal\\Tests\\Traits\\PHPUnit8Warnings::$deprecationWarnings',

We don't need to test skipping deprecations as that functionality is not part of this change. And if we actually call the deprecated methods then we don't need the other string.

The patch attached contains a test of the each deprecated method and shows that we're triggering the expected deprecations and more importantly that PHPUnit is not triggering other warnings. This means that if PHPUnit 8 adds any new deprecations in the meantime that effect these methods then this test will catch it. And on the issue that fixes the expectExceptionMessageRegExp() is deprecated in PHPUnit 8 and will be removed in PHPUnit 9. deprecation we can add another test to PHPUnit8WarningsTest.

It also means that if there other deprecated PHPUnit 8 methods that contrib is leveraging then we can add them to the list and test the deprecation in core too.

If we make Drupal compatible with PHPUnit 9 then we'll need to skip this test there but that's okay too.q

Oops forgot to revert the changes to PhpUnitBridgeTest.

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -142,6 +142,9 @@ public static function getSkippedDeprecations() {
       '\Drupal\Tests\SkippedDeprecationTest deprecation',
+      // The following are PHPUnit 8 deprecations that need to be resolved prior
+      // to PHPUnit 9 compatibility.
+      'expectExceptionMessageRegExp() is deprecated in PHPUnit 8 and will be removed in PHPUnit 9.',
       // The following Symfony deprecations are introduced in the Symfony 4

Can't we find a way not to have this in two places, but just in one, and have both DeprecationListenerTrait and PHPUnit8Warnings implementations access it? Reduce c/p carpal tunnel syndrome symptoms.

@mondrake I don't think so. The point is that we need to do the work to remove it from the skipped deprecations and then it remains in the PhpUnit8Warnings trait. The skipped deprecation list is a list of technical debt that we have. We can easily remove it from the list so we have it in one place - we do the work in core to stop calling the method. Created #3143235: Convert ::expectExceptionMessageRegExp to ::expectExceptionMessageMatches to handle this.

So if we land #3143235-4: Convert ::expectExceptionMessageRegExp to ::expectExceptionMessageMatches in 9.0.x and 9.1.x then we don't have to add this to the list of skipped deprecations and can add a test for expectExceptionMessageRegExp triggering a deprecation.

Now that #3143235: Convert ::expectExceptionMessageRegExp to ::expectExceptionMessageMatches has landed we don't need to skip it.

In #3127141: Use PHPUnit 9 for PHP 7.4+, while keeping support for PHPUnit 8.4 in PHP 7.3 we are suggesting to replace PHPUnit8Warnings with a more generic class name - since it's pretty clear this is there to stay in later PHPUnit versions. PHPUnit 9 already has warnings triggered for methods that will be remoed in PHPUnit 10.

I wonder whether this is the right time to do the renaming in this issue, since the other issue will take longer probably.

Re #44 because this does not add or change the logic for skipping deprecations that is tested elsewhere. PHPUnit adds deprecations mid cycle so if we want to support these calls we need to be sure that future versions of these asserts do not trigger other warnings. For example assertAttributeEquals currently triggers 3 warnings. If PHPUnit deprecates something else used in any of the calls then we need to know.

The point about the list changing and therefore we should add our own is a good one. Added that back in.

Renamed the class as #48.

> Still also dunno why we are adding deprecation tests of PHPUnit's own code

If PHPUnit changes any of the warning messages we will no longer convert them to deprecations, these tests will catch that.

Tried to update the documentation to account for #52.

I'm not sure it is triggering deprecations in a new way. It's using the same @trigger_error(message, E_USER_DEPRECATION) as everywhere else. And the skipping of such deprecations is tested already by \Drupal\Tests\SkippedDeprecationTest

Re #54.3 and #55 - #49 added a test that's independent of existing deprecation messages.

+++ b/core/tests/Drupal/Tests/PhpUnitWarningsTest.php
@@ -0,0 +1,91 @@
+  /**
+   * @expectedDeprecation Test warning for \Drupal\Tests\PhpUnitWarningsTest::testAddWarning()
+   */
+  public function testAddWarning() {
+    $this->addWarning('Test warning for \Drupal\Tests\PhpUnitWarningsTest::testAddWarning()');
+  }

[Edit: originally point to #53 as where the test was added but I was wrong]

I think this should go in 9.1.x first, without the hunks below, to allow consistently renaming of the trait across branches. We should not have the hunks below in 9.1.x because if at any point we will support PHPUnit 9, they will simply fail (unless we put BC shims around).

1.

+++ b/core/tests/Drupal/Tests/Traits/PhpUnitWarnings.php
@@ -0,0 +1,64 @@
+    'Using assertContains() with string haystacks is deprecated and will not be supported in PHPUnit 9. Refactor your test to use assertStringContainsString() or assertStringContainsStringIgnoringCase() instead.',
+    'Using assertNotContains() with string haystacks is deprecated and will not be supported in PHPUnit 9. Refactor your test to use assertStringNotContainsString() or assertStringNotContainsStringIgnoringCase() instead.',
+    'assertArraySubset() is deprecated and will be removed in PHPUnit 9.',
+    'assertInternalType() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertIsArray(), assertIsBool(), assertIsFloat(), assertIsInt(), assertIsNumeric(), assertIsObject(), assertIsResource(), assertIsString(), assertIsScalar(), assertIsCallable(), or assertIsIterable() instead.',
+    'readAttribute() is deprecated and will be removed in PHPUnit 9.',
+    'getObjectAttribute() is deprecated and will be removed in PHPUnit 9.',
+    'The optional $canonicalize parameter of assertEquals() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertEqualsCanonicalizing() instead.',
+    'assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.',
+    'assertAttributeSame() is deprecated and will be removed in PHPUnit 9.',
+    'assertAttributeInstanceOf() is deprecated and will be removed in PHPUnit 9.',
+    'assertAttributeEmpty() is deprecated and will be removed in PHPUnit 9.',
+    'The optional $ignoreCase parameter of assertContains() is deprecated and will be removed in PHPUnit 9.',
+    'The optional $ignoreCase parameter of assertNotContains() is deprecated and will be removed in PHPUnit 9.',

2.

+++ b/core/tests/Drupal/Tests/PhpUnitWarningsTest.php
@@ -0,0 +1,91 @@
+  /**
+   * @expectedDeprecation Using assertContains() with string haystacks is deprecated and will not be supported in PHPUnit 9. Refactor your test to use assertStringContainsString() or assertStringContainsStringIgnoringCase() instead.
+   * @expectedDeprecation The optional $ignoreCase parameter of assertContains() is deprecated and will be removed in PHPUnit 9.
+   */
+  public function testAssertContains() {
+    $this->assertContains('string', 'aaaastringaaa');
+    $this->assertContains('STRING', 'aaaastringaaa', '', TRUE);
+  }
+
+  /**
+   * @expectedDeprecation Using assertNotContains() with string haystacks is deprecated and will not be supported in PHPUnit 9. Refactor your test to use assertStringNotContainsString() or assertStringNotContainsStringIgnoringCase() instead.
+   * @expectedDeprecation The optional $ignoreCase parameter of assertNotContains() is deprecated and will be removed in PHPUnit 9.
+   */
+  public function testAssertNotContains() {
+    $this->assertNotContains('foo', 'bar');
+    $this->assertNotContains('FOO', 'bar', '', TRUE);
+  }
+
+  /**
+   * @expectedDeprecation assertArraySubset() is deprecated and will be removed in PHPUnit 9.
+   */
+  public function testAssertArraySubset() {
+    $this->assertArraySubset(['a'], ['a', 'b']);
+  }
+
+  /**
+   * @expectedDeprecation assertInternalType() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertIsArray(), assertIsBool(), assertIsFloat(), assertIsInt(), assertIsNumeric(), assertIsObject(), assertIsResource(), assertIsString(), assertIsScalar(), assertIsCallable(), or assertIsIterable() instead.
+   */
+  public function testAssertInternalType() {
+    $this->assertInternalType('string', 'string');
+  }
+
+  /**
+   * @expectedDeprecation assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
+   * @expectedDeprecation readAttribute() is deprecated and will be removed in PHPUnit 9.
+   * @expectedDeprecation getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
+   * @expectedDeprecation assertAttributeSame() is deprecated and will be removed in PHPUnit 9.
+   * @expectedDeprecation assertAttributeInstanceOf() is deprecated and will be removed in PHPUnit 9.
+   * @expectedDeprecation assertAttributeEmpty() is deprecated and will be removed in PHPUnit 9.
+   */
+  public function testAssertAttribute() {
+    $obj = new class() {
+      protected $attribute = 'value';
+      protected $class;
+      protected $empty;
+
+      public function __construct() {
+        $this->class = new \stdClass();
+      }
+
+    };
+    $this->assertAttributeEquals('value', 'attribute', $obj);
+    $this->assertAttributeSame('value', 'attribute', $obj);
+    $this->assertAttributeInstanceOf(\stdClass::class, 'class', $obj);
+    $this->assertAttributeEmpty('empty', $obj);
+  }
+
+  /**
+   * @expectedDeprecation The optional $canonicalize parameter of assertEquals() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertEqualsCanonicalizing() instead.
+   */
+  public function testAssertEquals() {
+    $this->assertEquals(['a', 'b'], ['b', 'a'], '', 0.0, 10, TRUE);
+  }
+

In order to allow PHPUnit 9 on Drupal 9 we're going to need the forward compatible shims so I disagree about removing the tests. I think that's one of the reasons why we need the tests - we need to ensure our forward compatible shims work as we want them to.

Then this one is good to go IMHO.

BTW, BC shims would be relatively easy for these methods that are no longer present in PHPUnit 9

+++ b/core/tests/Drupal/Tests/Traits/PhpUnitWarnings.php
@@ -0,0 +1,64 @@
+    'assertArraySubset() is deprecated and will be removed in PHPUnit 9.',
+    'assertInternalType() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertIsArray(), assertIsBool(), assertIsFloat(), assertIsInt(), assertIsNumeric(), assertIsObject(), assertIsResource(), assertIsString(), assertIsScalar(), assertIsCallable(), or assertIsIterable() instead.',
+    'readAttribute() is deprecated and will be removed in PHPUnit 9.',
+    'getObjectAttribute() is deprecated and will be removed in PHPUnit 9.',
...
+    'assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.',
+    'assertAttributeSame() is deprecated and will be removed in PHPUnit 9.',
+    'assertAttributeInstanceOf() is deprecated and will be removed in PHPUnit 9.',
+    'assertAttributeEmpty() is deprecated and will be removed in PHPUnit 9.',
...
+    'expectExceptionMessageRegExp() is deprecated in PHPUnit 8 and will be removed in PHPUnit 9.',

but where PHPUnit 9 keeps the method but changes the signature, things will be more complicated. assertContains on strings will probably require rewriting PHPUnit's Assert class.

+++ b/core/tests/Drupal/Tests/Traits/PhpUnitWarnings.php
@@ -0,0 +1,64 @@
+    'Using assertContains() with string haystacks is deprecated and will not be supported in PHPUnit 9. Refactor your test to use assertStringContainsString() or assertStringContainsStringIgnoringCase() instead.',
+    'Using assertNotContains() with string haystacks is deprecated and will not be supported in PHPUnit 9. Refactor your test to use assertStringNotContainsString() or assertStringNotContainsStringIgnoringCase() instead.',
...
+    'The optional $canonicalize parameter of assertEquals() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertEqualsCanonicalizing() instead.',
...
+    'The optional $ignoreCase parameter of assertContains() is deprecated and will be removed in PHPUnit 9.',
+    'The optional $ignoreCase parameter of assertNotContains() is deprecated and will be removed in PHPUnit 9.',

@mondrake that's a good point. I think we're in the "we'll do our best" with respect to forward compatibility here. But at least this test keeps us honest and if we have to skip the one where the method if changed on PHPUnit9 then so be it - but at least we'll have it documented and continued to be tested on PHPUnit8. There, is of course an on the other hand, if we leave the warnings as they - they'll get surfaced in contrib / custom projects quicker and therefore resolved quicker and therefore it'll be easier for these projects to support both PHPUnit 8 and 9 when core does.

Rerolled #53

Tests are failing.

Fixed previous CI failure.

@mohrerao please post interdiffs when you make a change to a patch. Here's one for #66.

@alexpott, #67 was a reroll #53 which failed in #64. Thats why i didnt add interdiff.

Looking into failed tests now.

Tried running tests locally after applying patch and tests are passing. Can some one explain why these failures are coming with Drupal CI?

@mohrerao take a look at the console log of the DrupalCI test run: https://dispatcher.drupalci.org/job/drupal_patches/56228/consoleFull

the error is in the build tests phase: Fatal error: Trait 'Drupal\Tests\Traits\PHPUnitWarnings' not found in /var/www/html/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php on line 52

probably an use statement is missing somewhere

Re #71, Drupal\Tests\Traits\PHPUnitWarning is there. But I can not figure it out why #66 did not pass. So rerolled from #53. But attached a raw-interdiff from #66 as well.

Strange, it looks like a case sensitivity issue, given interdiff vs #66

Drupal\Tests\media_library\FunctionalJavascript\WidgetUploadTest strikes again. :(

Bumping to Critical since this is in the critical path to PHP 8.

it looks like a case sensitivity issue

< +use Drupal\Tests\Traits\PHPUnitWarnings;
...
> +use Drupal\Tests\Traits\PhpUnitWarnings;

Yes, you're right. PHPUnitWarnings vs PhpUnitWarnings

Was there an issue opened with phpunit to split addWarning into addDeprecation/addWarning as that seems like a longer-term solution here? The use of one method for two concepts is how we got here.

Looking at these two approaches again, I think the trigger_error approach is more consistent with how we handle deprecation elsewhere.

It's blocker for #3127141: Use PHPUnit 9 for PHP 7.4+, while keeping support for PHPUnit 8.4 in PHP 7.3

Re #78 idea with addDeprecation vs addWarning interesting but looks huge api change, it could need to rename trait too, but I lack of reason to introduce this change... re-using deprecation listener for that also could have wrapper

@larowlan I know there has been some discussion between the Symfony project and PHPUnit project about functionality in PHPUnit bridge. However I couldn't find anything in the PHPUnit issue tracker so I created https://github.com/sebastianbergmann/phpunit/issues/4461

CSPELL: checking all files
core/tests/Drupal/Tests/PhpUnitWarningsTest.php:23:38 - Unknown word (aaaastringaaa)
core/tests/Drupal/Tests/PhpUnitWarningsTest.php:24:38 - Unknown word (aaaastringaaa)
CSpell: Files checked: 10, Issues found: 2 in 1 files

Patch attached fixes this.

OK I still don't have a preference between the approaches, but @larowlan in #78 slightly tips it in favour of the current patch, so...

Committed ab58ca4 and pushed to 9.1.x. Thanks!