\Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations() does not work in unit tests

Looks good to me and already was reviewed in part in #3029197: [symfony 5] Class implements "Symfony\Component\DependencyInjection\ResettableContainerInterface" that is deprecated since Symfony 4.2, use "Symfony\Contracts\Service\ResetInterface" instead. Adding some tags and removing needs tests since it has tests.

Fixed on commit

diff --git a/core/tests/Drupal/Tests/Listeners/AfterSymfonyListener.php b/core/tests/Drupal/Tests/Listeners/AfterSymfonyListener.php
index c374335ae9..121d0d2db2 100644
--- a/core/tests/Drupal/Tests/Listeners/AfterSymfonyListener.php
+++ b/core/tests/Drupal/Tests/Listeners/AfterSymfonyListener.php
@@ -8,7 +8,7 @@

 if (class_exists('PHPUnit_Runner_Version') && version_compare(\PHPUnit_Runner_Version::id(), '6.0.0', '<')) {
   class_alias('Drupal\Tests\Listeners\Legacy\AfterSymfonyListener', 'Drupal\Tests\Listeners\AfterSymfonyListener');
-  // Using an early return instead of a else does not work when using the
+  // Using an early return instead of an else does not work when using the
   // PHPUnit phar due to some weird PHP behavior (the class gets defined without
   // executing the code before it and so the definition is not properly
   // conditional).
diff --git a/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
index 37e08914be..1103450a9b 100644
--- a/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -165,8 +165,8 @@ protected function registerErrorHandler($test) {

     // Register another listener so that we can remove the error handler before
     // Symfony's DeprecationErrorHandler checks that it is the currently
-    // registered handler. Note this done like this to ensure the error hander
-    // is removed after SymfonyTestsListenerTrait::endTest() is called.
+    // registered handler. Note this is done like this to ensure the error
+    // handler is removed after SymfonyTestsListenerTrait::endTest() is called.
     // SymfonyTestsListenerTrait has its own error handler that needs to be
     // removed before this one.
     $test_result_object = $test->getTestResultObject();

Committed c52175d and pushed to 8.7.x. Thanks!

@catch just pointed me here from #3034015: Class to test deprecation error for deprecated trait triggers deprecation error itself, which may be a symptom of this.

Discussed this today, may help people working on this issue:

gaborhojtsy [2:00 PM]
I admit I did not understand what was the problem with the error handler swapping, is that too excessively happening where it should not? — I could work on the comment missing of course but that does not in itself help >D

alexpott [2:01 PM]
The symfony deprecation handler has a check in its shutdown function to ensure its expected error handler is still the error handler. I thought I’d managed to get this right by restoring the correct error handler before it runs but I missed something.

gaborhojtsy [2:02 PM]
ah, so we may be missing errors reported then(?)

alexpott [2:04 PM]
This is made way more complex by the fact we have quite a few layers to contend with. Isolated tests for example run in their own process. And all kernel / functional tests are isolated.
It looks like it’s the isolated tests where there is an issue.

Superb, thanks! Let's get this in for the 8.7 beta then :)

@alexpott Is it possible you were working on this patch in conjunction with symfony 4 testing, and were seeing the errors when you updated your dependencies to symfony 4.2? I am still seeing "THE ERROR HANDLER HAS CHANGED" locally with this patch, but only when using symfony 4 libraries. I'm seeing it in RecursiveContextualValidatorTest at least, at the moment.

I can reproduce #16 using the latest patch here and the latest patch on #2976394: Allow Symfony 4.4 to be installed in Drupal 8

This happens on all tests with multiple test methods. I think because the restore_error_handler() is done after each method, but we were only setting the new error handler on the start of the test run. Making sure that set_error_handler($deprecation_handler); is done for every test method seems to clear this up.

missing interdiff

Nice find with #18. Marking RTBC. Kicked off a couple of extra test runs.

Happy to commit this once #22 comes back green.

Committed 51cc668 and pushed to 8.8.x. Thanks!

Moving to 'to be ported' against 8.7.x for cherry-pick after the beta freeze.

Cherry-picked to 8.7.x