ExpectDeprecationTrait is not compatible with PHPUnit 8

I have tried to fix this issue, here is a patch please review it.

PHPUnit 8 has expectDeprecationMessage(), should we use the same method name?

+++ b/core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php
@@ -25,7 +25,7 @@ trait ExpectDeprecationTrait {
-  protected function expectDeprecation($message) {
+  protected function addExpectedDeprecationMessage($message) {

This will break existing code. I think we need to do something more BC friendly here. I think we could make our method conform to the parent and offer a BC layer.

@mondrake you can add extra args... the problem is the slightly different functionality.

Well at some point PHPUnit is going to take on more of what Symfony's simple phpunit is doing wrt to deprecation testing - hence this method so we might want to give ourselves some wiggle room. I think it might be okay to trigger an unsilenced deprecation and have a new non-clashing method name. So we can move to the new method.

@mondrake what about https://3v4l.org/TAEae?

1. +++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit6/ExpectDeprecationTrait.php
   @@ -0,0 +1,34 @@
   +      @trigger_error('ExpectDeprecationTrait::expectDeprecation is deprecated in drupal:8.9.0 and is removed from drupal:9.0.0. Use ::addExpectedDeprecationMessage() instead. See https://www.drupal.org/node/3106024', E_USER_DEPRECATED);
   
   +++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit7/ExpectDeprecationTrait.php
   @@ -0,0 +1,34 @@
   +      @trigger_error('ExpectDeprecationTrait::expectDeprecation is deprecated in drupal:8.9.0 and is removed from drupal:9.0.0. Use ::addExpectedDeprecationMessage() instead. See https://www.drupal.org/node/3106024', E_USER_DEPRECATED);
   

   I don't this should be silenced. Also I think the fact that we've not changed any calls to expectDeprecation shows that this is not quite working as expected.

2. +++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/ExpectDeprecationTrait.php
   @@ -0,0 +1,18 @@
   +trait ExpectDeprecationTrait {
   +
   +    // @todo remove in Drupal 9.
   +
   +}
   

   This needs to do forwards-compatible logic

+++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/ExpectDeprecationTrait.php
@@ -0,0 +1,18 @@
+<?php
+
+namespace Drupal\TestTools\PhpUnitCompatibility\PhpUnit8;
+
+/**
+ * Adds the ability to dynamically set expected deprecation messages in tests.
+ *
+ * @internal
+ *   This class should only be used by Drupal core and will be removed once
+ *   https://github.com/symfony/symfony/pull/25757 is resolved.
+ *
+ * @todo Remove once https://github.com/symfony/symfony/pull/25757 is resolved.
+ */
+trait ExpectDeprecationTrait {
+
+    // @todo remove in Drupal 9.
+
+}

Without a forwards compatibility layer tests that use expectDeprecation in Drupal are not testing what they think they are testing.

1. +++ b/core/tests/Drupal/Tests/ExpectDeprecationTest.php
   @@ -14,23 +15,41 @@ class ExpectDeprecationTest extends UnitTestCase {
   +      $this->addExpectedDeprecationMessage('ExpectDeprecationTrait::expectDeprecation is deprecated in drupal:8.9.0 and is removed from drupal:9.0.0. Use ::addExpectedDeprecationMessage() instead. See https://www.drupal.org/node/3106024');
   

   I think we should add this deprecation to 8.8.x - but worth getting a +1 from a release manager.

2. +++ b/core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php
   @@ -2,12 +2,20 @@
   +// In order to manage different method signatures between PHPUnit versions, we
   +// dynamically load a compatibility trait dependent on the PHPUnit runner
   +// version.
   +if (!trait_exists(PhpunitVersionDependentExpectDeprecationTrait::class, FALSE)) {
   +  class_alias("Drupal\TestTools\PhpUnitCompatibility\PhpUnit" . RunnerVersion::getMajor() . "\ExpectDeprecationTrait", PhpunitVersionDependentExpectDeprecationTrait::class);
   +}
   
   @@ -19,13 +27,15 @@
   -  protected function expectDeprecation($message) {
   +  protected function addExpectedDeprecationMessage($message) {
   

   I don't think we need to the do the different version dance here - we can just trigger the deprecation from the method and be done - less complexity.

I think adding the deprecation is OK in the test system in 8.8.x, we're just communicating something that has to happen.

I have to say that I'm entirely unconvinced by the need for a BC layer, as pointed out in #9

 * @internal
 *   This class should only be used by Drupal core and will be removed once
 *   https://github.com/symfony/symfony/pull/25757 is resolved.

Honestly, if that doesn't signal "DON'T USE THIS", what point is there to marking anything @internal.

But we have one now, so ¯\_(ツ)_/¯
Can we get a quick update to mark it deprecated in 8.8.x per #26.1?

Here is just updated deprecated in 8.8.x per #26.1.

Updated deprecated message in test files wrt to patch #31, kindly review

+++ b/core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php
@@ -25,10 +25,28 @@
+      @trigger_error('ExpectDeprecationTrait::expectDeprecation is deprecated in drupal:8.8.x and is removed from drupal:9.0.0. Use ::addExpectedDeprecationMessage() instead. See https://www.drupal.org/node/3106024', E_USER_DEPRECATED);

8.8.x needs to be the exact version this was added - so this should be 8.8.4 as that will be the next release.

@longwave i have updated version to 8.8.4, kindly review new patch

1. +++ b/core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php
   @@ -25,10 +25,28 @@
   +   * @deprecated in drupal:8.9.0 and is removed from drupal:9.0.0. Use
   +   *   ::addExpectedDeprecationMessage() instead.
   

   This also needs to say 8.8.4 instead of 8.9.0.

2. +++ b/core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php
   @@ -25,10 +25,28 @@
   +    if (strpos($message, 'ExpectDeprecationTrait::expectDeprecation') === FALSE) {
   

   Not sure what this is guarding against, as I don't see how it can be called this way, but I also guess it can't hurt.

OK I see - so let's just fix #39.1 and this is ready to go in I think.

Updated patchh wrt comment #39.1, kindly review

Looks good, let's try to get this in before commit freeze!

AS #42 is only changing comments I think this is safe to commit before it comes back green and is worth getting into 8.8.x asap.

Committed and pushed 71a80e785a to 8.9.x and 9c0bf60beb to 8.8.x. Thanks!