[Symfony 5] Symfony/Component/EventDispatcher/Event is deprecated in Symfony 4.3 use Symfony/Contracts/EventDispatcher/Event instead

Moreover SF 4.3 changed the way events registered https://symfony.com/blog/new-in-symfony-4-3-simpler-event-dispatching

Mostly straightforward find-and-replace.

Heh, well, it worked on the handful of tests I did try :)

I don't see how we can fix this until Symfony resolves similar issues with their own event classes; FilterResponseEvent extends the deprecated Event class and doesn't use the new one yet, but our dispatcher needs to be able to handle FilterResponseEvent and the new ones at the same time:

"Argument 2 passed to Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch() must be an instance of Symfony\Contracts\EventDispatcher\Event or null, instance of Symfony\Component\HttpKernel\Event\FilterResponseEvent given"

Symfony\Component\HttpKernel\Event\FilterResponseEvent is completely gone in Symfony 5, replaced with Symfony\Component\HttpKernel\Event\ResponseEvent. ResponseEvent implements the Symfony\Contracts interface in Symfony 5, but the component interface in Symfony 4.

So they're not going to update those classes in Symfony 4, instead they're completely deprecating them.

If we need to change the type hint and also provide for bc, we may need to do something like the following:

1. Define a new, core Event class. In Drupal 9, this would inherit from Component\Event, in Drupal 10, it inherits from Contracts\Event

2. Change all core subclasses to extend the new Drupal Event class (and trigger a deprecation error for contrib/custom when an Event class is not a subclass of the core one, or from Symfony itself).

Then in Drupal 10, when we update the Drupal event class, all the existing inheritance and type hints stay intact - we're essentially only changing the use statement for type hints at that point.

No interdiff since it's a new approach.

If this passes, for every Event class that core defines, we need to update the use statement to subclass from core.

#3055194: [Symfony 5] The signature of the "Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch()" method should be updated to "dispatch($event, string $eventName = null)", not doing so is deprecated since Symfony 4.3. is related since this also changes the signature of ::dispatch() - we should be able to do some custom deprecation handling there to handle this too.

Probably we won't be able to remove the actual Symfony 5 deprecation message suppression, but if a forward compatibility layer exists and we have a way to tell Drupal 9 modules they should use it via our own custom deprecation message, that should be enough to resolve this issue.

Alright taking things a bit further, there are now two main changes in the patch:

1. Events need to extend from the new Drupal\Component\EventDispatcher\Event class.

2. Event listeners need to stop type hinting their ::onFoo() methods (or at least ones that reference the raw Event class. There is no requirement for them to type hint.

If an event listener knows it's going to get a Drupal Event class or a subclass of Drupal Event, it could type hint on that - but I did not implement things like this since it makes everything inconsistent. Could also technically type hint on a Symfony event subclass like KernelEvent since that can also be updated from component to contracts without affecting the use statement/typehint in the event listener.

When modules drop Drupal 9 support they could type hint on the Symfony contracts version if they wanted to.

Still not sure what or even if we can do about triggering deprecations here, trying to take this a step at a time.

Next step:

Remove the Event type hint in ContainerAwareDispatcher::dispatch() - this is consistent with the change we have to make in #3055194: [Symfony 5] The signature of the "Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch()" method should be updated to "dispatch($event, string $eventName = null)", not doing so is deprecated since Symfony 4.3., and means that someone could pass a Symfony\Contracts\EventDispatcher\Event class if they wanted to.

Also when creating a default Event class, use the Drupal subclass.

OK well that step exploded.

Removing the type hint on ::dispatch() results in an illegal offset error, didn't look into why that's happening yet.

Just for kicks I tried changing the typehint locally to the Drupal Event class. This immediately ran into a problem with core events that inherit from Symfony's GenericEvent (which is still in Symfony master but now inherits from the contracts class).

I am starting to think about the following course of action:

1. Commit more or less what is in #3055198-15: [Symfony 5] Symfony/Component/EventDispatcher/Event is deprecated in Symfony 4.3 use Symfony/Contracts/EventDispatcher/Event instead - to make the new class available to contributed modules asap. We may even want to backport just the class to 9.0 or even 8.9

2. Proceed with #3055194: [Symfony 5] The signature of the "Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch()" method should be updated to "dispatch($event, string $eventName = null)", not doing so is deprecated since Symfony 4.3. - this will remove the type hint from ::dispatch and handle both versions of the Event class.

3. Open follow-ups to figure out how to notify people of the change - although it may have to be a drupal check/rector rule in the end.

> Removing the type hint on ::dispatch() results in an illegal offset error

This is because of the LegacyEventDispatcherProxy::decorate() method. It uses reflection to check the second argument to ::dispatch() and only adds the BC decorator if the argument is typed as an object - removing the typehint makes Symfony think we have done #3055194: [Symfony 5] The signature of the "Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch()" method should be updated to "dispatch($event, string $eventName = null)", not doing so is deprecated since Symfony 4.3. already.

OK I think that's another reason to go with #17, I've opened #3153803: [Symfony 5] Update EventDispatcher::dispatch() to make it forward-compatible with Symfony 5.

Re-uploading #15 since #16 is out of scope here.

I think the course of action described in #17 is sensible and the patch here does as much as we can without stepping on #3055194: [Symfony 5] The signature of the "Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch()" method should be updated to "dispatch($event, string $eventName = null)", not doing so is deprecated since Symfony 4.3., so as far as I can see this is ready to go.

FILE: /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Routing/RoutePreloader.php
------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------
 9 | WARNING | [x] Unused use statement
------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------

+++ b/core/lib/Drupal/Core/Routing/RoutePreloader.php
@@ -68,7 +67,7 @@ public function __construct(RouteProviderInterface $route_provider, StateInterfa
-  public function onRequest(KernelEvent $event) {
+  public function onRequest($event) {

@@ -96,7 +95,7 @@ public function onRequest(KernelEvent $event) {
-  public function onAlterRoutes(RouteBuildEvent $event) {
+  public function onAlterRoutes($event) {

Are we sure we want to remove these typehints? At least one is results in an unused use. I can see why we're removing the generic event typehints but these seem unnecessary.

Are we sure we want to remove these typehints? At least one is results in an unused use. I can see why we're removing the generic event typehints but these seem unnecessary.

So it's literally only this class that has this change. The reason I did it is because it's handling multiple events in one class, and at the moment some type hint on Event and some type hint on KernelEvent. While the type hint on KernelEvent doesn't have to be removed, if we don't, then we have a mixture of typehinted and non-typehinted listeners in the same class which looks very inconsistent - I think I'd be confused if I was looking at the class fresh without any background on this issue.

While it's not necessary to deal with the deprecation, I think it's worth doing. Having said that, this isn't a strong preference and I wouldn't object to a version without that change, but for now here's the unused use statement fixed.

1. +++ b/core/lib/Drupal/Core/Routing/RoutePreloader.php
   @@ -68,7 +66,7 @@ public function __construct(RouteProviderInterface $route_provider, StateInterfa
   -  public function onRequest(KernelEvent $event) {
   +  public function onRequest($event) {
        // Only preload on normal HTML pages, as they will display menu links.
        if ($this->routeProvider instanceof PreloadableRouteProviderInterface && $event->getRequest()->getRequestFormat() == 'html') {
   
   @@ -96,7 +94,7 @@ public function onRequest(KernelEvent $event) {
   -  public function onAlterRoutes(RouteBuildEvent $event) {
   +  public function onAlterRoutes($event) {
        $collection = $event->getRouteCollection();
   

   I think we should leave these typehints in - we're using methods on these events that are only present in these specific event classes. And therefore the typehint is valid.

2. +++ b/core/lib/Drupal/Core/Routing/RoutePreloader.php
   @@ -112,7 +110,7 @@ public function onAlterRoutes(RouteBuildEvent $event) {
   -  public function onFinishedRoutes(Event $event) {
   +  public function onFinishedRoutes($event) {
        $this->state->set('routing.non_admin_routes', $this->nonAdminRoutesOnRebuild);
        $this->nonAdminRoutesOnRebuild = [];
      }
   

   I think we should consider completely dropping the event argument when it's not used. We could handle that in a follow-up but maybe we'll do it on this one so we don't have the inconsistency of some typehinted methods and some not.

Feel free to re-rtbc once those typehints have been added back.

OK I've re-added the KernelEvent and RouteBuildEvent type hints, and dropped the param altogether for ::onFinishedRoutes(). This resolves my (mostly aesthetic) issue with the mis-match of type hinting and not type hinting on that class in a different way. Also I am pretty sure that #3153803: [Symfony 5] Update EventDispatcher::dispatch() to make it forward-compatible with Symfony 5 will still pass with the change, because we allow non-Drupal Event subclasses without triggering a deprecation error there.

1. +++ b/core/lib/Drupal/Core/Routing/RoutePreloader.php
   @@ -4,8 +4,8 @@
   +use Drupal\Core\Routing\RouteBuildEvent;
   

   I think this is an unused use (as the class is already in this namespace)

2. +++ b/core/modules/migrate/src/Event/EventBase.php
   @@ -4,9 +4,9 @@
   +use Drupal\Component\EventDispatcher\Event as Event;
   

   The "as Event" seems redundant.

Addressing #26.

Note I've added a change record and release notes snippet to #3153803: [Symfony 5] Update EventDispatcher::dispatch() to make it forward-compatible with Symfony 5 which is the first issue where contrib modules can reliably respond to the change.

Thanks, all looks good now, RTBC assuming tests are green.

Committed 3b8e432 and pushed to 9.1.x. Thanks!

The devel 9.1 tests are failing, and I think this commit is the cause. See https://gitlab.com/drupalforks/devel/-/jobs/638265843#L401.

How can devel remain compatible with Drupal 8 and Drupal 9.1 at same time? Thats not a goal after 9.0? Devel is obligated to type hint to an Event class because our base class does that. But there are different classes passed in 8 and 9. The TypeError comes from https://gitlab.com/drupalspoons/devel/-/blob/4.x/webprofiler/src/EventDi...

I discuss strategies for this sort of thing in this thread: https://www.drupal.org/project/maintainers/issues/3154344

So the issue is that devel is extending ContainerAwareEventDispatcher itself.

Probably the most straightforward is to have two versions of the class, then choose which one to use based on Drupal version in WebprofilerServiceProvider. Then the Drupal 8 version and registration can be dropped when you officially drop Drupal 8 support, and you'll only have one again.

Could we update the CR with the guidance about 2 subscriber versions? Plus I think we should publish it.

Will this also be committed to 9.0.x?

I added a note about extending ContainerAwareEventDispatcher to the change record and published it.

@joegraduate this is a disruptive change in some cases, so can only be committed to 9.1.x

core/modules/user/src/Event/UserFloodEvent.php File does not exists.