Problem/Motivation

With #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher being committed to 10.3, any module that implements the event dispatcher will break. This is because the standard practice for returning the dispatcher service requires ContainerAwareEventDispatcher to be returned, which has been moved to the EventDispatcher.

Steps to reproduce

Enable and try to use any module using an event dispatcher. IE: search_api_solr, acquia_connector, views_json_source, etc

Proposed resolution

Add a BC layer that aliases ContainerAwareEventDispatcher to EventDispatcherInterface so typehints in contrib and custom continue to work.

Issue fork drupal-3445525

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

japerry created an issue. See original summary.

japerry’s picture

japerry
Location KOMK
commented
Related issues: +#2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher
andypost’s picture

andypost
he/him
Primary language Russian
commented

It means modules should be fixed to require EventDispatcherInterface instead of specific class

japerry’s picture

japerry
Location KOMK
commented
Title: Issue 2909185 Breaks any module implementing event_dispatchers » Issue 2909185 Breaks any module maintaining event_dispatcher backward compatiblity

Its been 4 years since I revisited the event dispatcher deprecations, but I vaguely remember the need to directly call ContainerAwareEventDispatcher because it would inherit the correct EventDispatcherInterface depending on what version of core (8.9, 9.0, 9.3, etc) you were using.

At this point, I think its safe to say we no longer need (or should) support Drupal 8 or early version of 9 (9.5 would be nice since 40% of sites are still on it), meaning any module using this workaround can update their calls to use Symfony\Contracts\EventDispatcher\EventDispatcherInterface.

However, with 10.3 only a month away, and the real likelihood of modules not being updated before this, sites will end up randomly breaking live. So instead of changing the service definition, is there some way to throw a deprecated notice if you're calling the class directly from your code? The Change Record should also be updated to tell maintainers they need to update their module if they used it as a type hint for the event dispatcher.

If anything, maybe move the change to 10.4, allowing modules more than a month to update?

alexpott’s picture

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

How about we provide a shim - i.e. bring back ContainerAwareEventDispatcher but as an alias for Symfony\Component\EventDispatcher\EventDispatcher - that way things will still work.

alexpott opened merge request !7931

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review

I've created an MR that adds a BC layer that allows me to install views_json_source

japerry’s picture

japerry
Location KOMK
commented
Status: Needs review » Reviewed & tested by the community

Yup! MR7931 works, tested with both Acquia Connector and Search API Solr. I think thats a good workaround, and should throw the deprecation notice as we're updating to Drupal 11.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Needs some phpstan tweaking, but class alias seems like a good plan to me.

alexpott’s picture

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

Hmmmm.... so we still have ContainerAwareEventDispatcher in core... so I think the fix has to be a little different... I think we need to only alias the class when the event dispatcher uses the Symfony\Component\EventDispatcher\EventDispatcher... so that's going to interesting.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

Could the original class be made to allow nullable parameters in its constructor with this fix? It feels dirty. But I guess it's possible.

mstrelan’s picture

mstrelan commented
Related issues: +#3440958: Deprecate \Drupal\Component\EventDispatcher\Event

Possibly should be considered together with #3440958: Deprecate \Drupal\Component\EventDispatcher\Event.

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +beta target
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
alexpott’s picture

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

So for some reason \Drupal\KernelTests\Core\Routing\ExceptionHandlingTest::testExceptionEscaping and \Drupal\KernelTests\Core\Routing\ExceptionHandlingTest::testBacktraceEscaping are breaking with this change.

It looks like the kernel tests go into a loop on gitlab ci - locally they just say "Test was run in child process and ended unexpectedly" :D

alexpott’s picture

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

Lol #15 was totes a silly mistake... causing loops.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Issue 2909185 Breaks any module maintaining event_dispatcher backward compatiblity » Add BC layer for contrib modules using ContainerAwareEventDispatcher as a typehint (BC fix for #2909185)
Issue summary: View changes

Fixing the title

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented 

  event_dispatcher.legacy:
    decorates: event_dispatcher
    class: Drupal\Core\LegacyEventDispatcher
    arguments: ['@event_dispatcher.legacy.inner']

👏 this is great

alexpott opened merge request !8063

alexpott changed the visibility of the branch 3445525-issue-2909185-breaks to hidden.

alexpott changed the visibility of the branch 3445525-event-dispatcher-bc to hidden.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

As the practical thing to do for now let's revert this in 10.3/10.4, keep it as-is in 11, and try to revisit the BC layer in 10.4.

alexpott’s picture

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

Committed and pushed aeb50aea4c to 10.4.x and afd3e94a44 to 10.3.x. Thanks!

  • alexpott committed afd3e94a on 10.3.x 
    Issue #3445525 by alexpott, japerry, catch, mglaman, longwave: Add BC...

  • alexpott committed aeb50aea on 10.4.x 
    Issue #3445525 by alexpott, japerry, catch, mglaman, longwave: Add BC...

Status: Fixed » Closed (fixed)

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

japerry’s picture

japerry
Location KOMK
commented

So there might be a new problem this issue has surfaced -- The deprecation warning appears to be gone in 10.3/4 but gone in Drupal 11. Doesn't that mean when you're running the deprecation checks, you won't see this warning, until the module WSODs with Drupal 11?

https://git.drupalcode.org/project/drupal/-/blob/10.3.x/core/lib/Drupal/...
vs
https://git.drupalcode.org/project/drupal/-/blob/11.0.x/core/lib/Drupal/...