Problem/Motivation

In #2895487: [Symfony 4] ContainerAwareEventDispatcherTest depends on Symfony tests We adjusted Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDispatcherTest to extend Symfony's AbstractEventDispatcherTest for a few reasons, including being compatible with Symfony 4. Now, in Symfony 4 Beta 1, AbstractEventDispatcherTest has gone away and been rolled into Symfony's EventDispatcherTest. We should roll AbstractEventDispatcherTest into our event dispatcher test so we don't depend on Symfony anymore.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#9 interdiff.3055040.6-9.txt738 bytesmikelutz
#9 3055040-9.drupal.Roll-AbstractEventDispatcherTest-into-ContainerAwareEventDispatcherTest.patch18.33 KBmikelutz
#6 interdiff.3055040.3-6.txt701 bytesmikelutz
#6 3055040-6.drupal.Roll-AbstractEventDispatcherTest-into-ContainerAwareEventDispatcherTest.patch18.33 KBmikelutz
#3 3055040-3.drupal.Roll-AbstractEventDispatcherTest-into-ContainerAwareEventDispatcherTest.patch17.64 KBmikelutz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikelutz created an issue. See original summary.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
Issue summary: View changes
Related issues: -#3029540: [Symfony 4] Sub class \Symfony\Component\Validator\ConstraintViolation and use that in \Drupal\Core\TypedData\Validation\ExecutionContext::addViolation() +#2895487: [Symfony 4] ContainerAwareEventDispatcherTest depends on Symfony tests

https://www.drupal.org/project/drupal/issues/2895487

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
Status: Active » Needs review
FileSize
3055040-3.drupal.Roll-AbstractEventDispatcherTest-into-ContainerAwareEventDispatcherTest.patch17.64 KB
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: +Symfony 4, +Symfony 5
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

This can uncomment and remove the @see in \Drupal\Core\Composer\Composer::$packageToCleanup

    // @see \Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDispatcherTest
    // 'symfony/event-dispatcher' => ['Tests'],
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
FileSize
3055040-6.drupal.Roll-AbstractEventDispatcherTest-into-ContainerAwareEventDispatcherTest.patch18.33 KB
interdiff.3055040.3-6.txt701 bytes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented 
+++ b/core/tests/Drupal/Tests/Component/EventDispatcher/ContainerAwareEventDispatcherTest.php
@@ -4,29 +4,54 @@
+ * NOTE: 72% of this code is a literal copy of Symfony 3.4's

72% => most :)

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented

For the record, I counted lines and actually did the math. :-p

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
FileSize
3055040-9.drupal.Roll-AbstractEventDispatcherTest-into-ContainerAwareEventDispatcherTest.patch18.33 KB
interdiff.3055040.6-9.txt738 bytes
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer and at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

Seems like @alexpott is on board with this change. This test has been problematic to fix in the past. This RTBC for now but I hope one day we'll find a way to remove this test.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

For the record, I counted lines and actually did the math

I totally believe that - but also I didn't want to try to maintain that number as further changes might be made to the test.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

I think given that \Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher is 100% Drupal code having the test be all Drupal makes sense. Can we open a follow-up to make the test confirm to Drupal's standards - there's no point maintaining something in the Drupal code base in the Symfony style that has no corollary in Symfony 4.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 3055040-9.drupal.Roll-AbstractEventDispatcherTest-into-ContainerAwareEventDispatcherTest.patch, failed testing. View results

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer and at PreviousNext commented
Status: Needs work » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

The fail in #13 is due to #3059022: If Vimeo is down our tests break.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed 0356445 and pushed to 8.8.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Component/EventDispatcher/ContainerAwareEventDispatcherTest.php b/core/tests/Drupal/Tests/Component/EventDispatcher/ContainerAwareEventDispatcherTest.php
index bb7727f057..8909c8533a 100644
--- a/core/tests/Drupal/Tests/Component/EventDispatcher/ContainerAwareEventDispatcherTest.php
+++ b/core/tests/Drupal/Tests/Component/EventDispatcher/ContainerAwareEventDispatcherTest.php
@@ -11,13 +11,12 @@
 use Symfony\Component\EventDispatcher\Event;
 use Symfony\Component\EventDispatcher\EventDispatcher;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
-use Symfony\Component\EventDispatcher\Tests\AbstractEventDispatcherTest;
 
 /**
  * Unit tests for the ContainerAwareEventDispatcher.
  *
  * NOTE: Most of this code is a literal copy of Symfony 3.4's
- * AbstractEventDispatcherTest.
+ * Symfony\Component\EventDispatcher\Tests\AbstractEventDispatcherTest.
  *
  * This file does NOT follow Drupal coding standards, so as to simplify future
  * synchronizations.

Fixed unused use and use a FQCN for the reference.

  • alexpott committed 0356445 on 8.8.x 
    Issue #3055040 by mikelutz, alexpott: Roll AbstractEventDispatcherTest...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

Created the follow-up - #3059079: Make ContainerAwareEventDispatcherTest use Drupal coding standards

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented

FYI, the only reason I didn't switch to Drupal standards In this issue was because we are likely to need to make changes one way or another in the 8.8 cycle for the event dispatcher, to prep for Drupal 9 and Symfony 5. Symfony 4 has a newer version of the test classes, which might work for us if we end up chosing to adapt ours in a similar way to the way Symfony did in 4.3, in which case I didn't want to reformat twice, I was hoping to have a plan for the dispatcher in place first.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.