Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
This is a bug report but I'm filing it against 8.4.x because it's not horribly urgent.
Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDispatcherTest looks like this:
[..]
use Symfony\Component\EventDispatcher\Tests\ContainerAwareEventDispatcherTest as SymfonyContainerAwareEventDispatcherTest;
use Symfony\Component\EventDispatcher\Tests\TestEventListener;
/**
* Unit tests for the ContainerAwareEventDispatcher.
*
* NOTE: 98% of this code is a literal copy of Symfony's emerging
* CompiledEventDispatcherTest.
*
* This file does NOT follow Drupal coding standards, so as to simplify future
* synchronizations.
*
* @see https://github.com/symfony/symfony/pull/12521
*
* @group EventDispatcher
*/
class ContainerAwareEventDispatcherTest extends SymfonyContainerAwareEventDispatcherTest
If we click through to that pull request: https://github.com/symfony/symfony/pull/12521 we see that it's been closed.
We should not depend on a test from a dependency, because it could move or change expectations.
A comment in #2874909-25: Update Symfony components to 3.3.* references this test.
Proposed resolution
Figure out what to do.
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 2895487-9.drupal.Symfony-4-ContainerAwareEventDispatcherTest-depends-on-Symfony-tests.patch | 3.73 KB | mikelutz |
Comments
Comment #3
andypostComment #4
andypostI don't think it's a bug, the
\Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDispatcherTesttests exactly core expectations from event listener.That looks too much binded but let's first resolve #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher
Comment #8
mikelutzComment #9
mikelutzAt a minimum we should base against symfony's AbstractEventDispatcherTest, rather than the ContainerAwareEventDispatcherTest, as ContainerAwareEventDispatcherTest has been removed in Symfony4.
I dug through the logs to get a bit more history here. It looks like we created our own ContainerAwareEventDispatcher from scratch, modeled on symfony's with some improvements, and then had our test class extend Symfony's ContainerAware test class. Half a dozen test function in this class actually test Symfony's ContainerAwareEventDispatcher instead of ours, so they are basically useless. in #2927746: Update Symfony components to 3.4.* We updated symfony to 3.4, and Symfony's ContainerAware dispatcher was deprecated. These half a dozen useless inherited test functions started throwing deprecation errors (because they were testing the wrong dispatcher), so we wrapped them in @expectedDeprecation wrappers to hide them. We really seem to want to use the tests in Symfony's AbstractEventDispatcherTest which does test against the dispatcher we set up in the setUp method, but there is nothing in Symfony's ContainerAwareEventDispatcherTest that we actually use.
So this patch switches from extending ContainerAware to extending the AbstractEventDispatcherTest, and removes the now unneeded Deprecation wrappers. I think this is a good compromise right now, as it will let us continue the Symfony 4 work while #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher is resolved, and remove useless tests. We still extend off a symfony test, but at least it's an abstract test class that's made to be extended, and is still available in symfony 4.
Comment #10
catchVery tidy patch and agreed this is a good interim step.
Do we want an additional follow-up other than #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher to remove the test subclass? That's the only thing I can think of here but it might be fine to just have the one issue open for now. RTBC.
Comment #11
mikelutzI haven't completely read through the other thread, but I assume if they deprecate the dispatcher, then they would be forced to mark the whole test subclass as @legacy in the same patch, and it would be removed with all the other deprecations when we open Drupal 9. I don't think we would want to remove the test subclass completely before then, as it has value in continuing to test the deprecated dispatcher for the remainder of Drupal 8.
Comment #12
catchCommitted e4d88be and pushed to 8.8.x. Thanks!
Comment #14
jibranGiven that this a test fix I'd vote for 8.7.x cherry-pick for the sake of keeping the tests in sync.
Comment #15
mikelutzI don't think there would be any issue with backporting. It should be valid in 8.7
Comment #17
mstrelan CreditAttribution: mstrelan commentedFrom what I can tell this works for symfony 4.2 but not 4.3 as AbstractEventDispatcherTest no longer exists.