Replace ContainerAwareEventDispatcher with Symfony EventDispatcher

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterEventSubscribersPass.php
@@ -2,11 +2,18 @@
+@trigger_error('The ' . __NAMESPACE__ . '\RegisterEventSubscribersPass is deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. Instead, use Symfony\Component\EventDispatcher\DependencyInjection\RegisterListenersPass. See https://www.drupal.org/project/drupal/issues/2909185', E_USER_DEPRECATED);
...
+ * @deprecated in drupal:9.4.0 and is removed from drupal:10.0.0.
+ * Use \Symfony\Component\EventDispatcher\DependencyInjection\RegisterListenersPass.

needs update

Also needs reroll following ✨ Support ServiceClosureArgument in \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::dumpValue Fixed , which added the new feature to the container plus tests (so I don't think we need additional tests here any more)

Filed CR https://www.drupal.org/node/3376090 →

Patch also deprecating ContainerAwareEventDispatcher

Fix CS

This is mentioned above, but https://github.com/symfony/symfony/pull/23008 was the eventual commit that hopefully made this obsolete in Symfony.

#1972300: Write a more scalable dispatcher → has quite a lot of performance numbers.

We might want to port https://www.drupal.org/sandbox/znerol/2345951 → to Drupal 10 and run the same thing again.

We might want to port https://www.drupal.org/sandbox/znerol/2345951 → to Drupal 10 and run the same thing again.

I might have ported that already. And I'm still using the same machine, so I'm going to try and reproduce the numbers.

I see the RegisterListenersPass has hard-coded tag names like 'kernel.event_listener' and 'kernel.event_subscriber'.
This seems like a blocker to me. Am I wrong?

The faster instantiation is not resolved, and this is not currently possible to override; the $listeners property is private in the Symfony EventDispatcher and there is no way to inject a precompiled set of listeners. Perhaps Symfony would be amenable to addressing this upstream.

A first idea is that this does not scale well with many events and listeners.

Consider a project with 100 different event types, with 10 listeners each.
To load the event dispatcher, we need 1000 times of resolving a service argument, creating the service closure, and calling ->addListener().
This happens on every request that needs the event dispatcher, even if only one event is fired, or perhaps no event at all.

We do _not_ load all the services, which is good. But still there is an overhead that grows with the number of total registered listeners.
It seems the overhead should be proportional to the total number of listeners.

needs profiling

Yes!
So, i applied the patch, and did some hacking to make it _somewhat_ work:
It still breaks because there is an object parameter that cannot be compiled.
But we can make measurements before the compilation.
So:
- I changed the tag name 'kernel.event_subscriber' to just 'event_subscriber' in RegisterListenersPass in symfony.
- I added a $container->get('event_dispatcher'); in DrupalKernel, after the container is compiled.
- I added microtime(TRUE) in various places.
- with xdebug enabled, I run "drush cr" to inspect the number of calls in the event_dispatcher service.
- with xdebug fully disabled (phpdismod), I run "drush cr" and other commands for the actual profiling.
- I profile with and without the patch.

I see 121 calls to ->addListener() in the definition for 'event_dispatcher'.
This means there are 121 event listener methods in core. (this is the actual value for the placeholder number 1000 from above)
In a real-world project, this number could grow quite a lot. But would it grow significantly > 1000?

Profiling results:

Without the patch, drush uli (as good as any other command or request), Container->get('event_dispatcher'):

• Container->get('event_dispatcher') uses ~0.49 milliseconds.
• Biggest part is including the actual class file in Container->createService(), which consumes ~0.47 milliseconds.
  (Maybe my opcache is misconfigured?)
• Resolving the arguments takes ~0.002 milliseconds.

Without the patch, drush uli (as good as any other command or request), Container->get('event_dispatcher'), but with the class file pre-loaded:

• Container->get('event_dispatcher') uses ~0.007 milliseconds.

Without the patch, in drush cr after container is compiled, ContainerBuilder->get('event_dispatcher'):

• ContainerBuilder->get('event_dispatcher') uses ~0.51 milliseconds.
• The biggest part is for resolving arguments in ContainerBuilder->createService().
  This could actually be sped up, because we don't need the container to go through all the ids, we just want the unchanged array value.
  But, it does not really matter, because normally we don't deal with ContainerBuilder but with Container.
• Loading the actual class file for EventDispatcher.php takes no time, because with drush cr, that class is already loaded.

With the patch, in drush cr, after container is compiled, ContainerBuilder->get('event_dispatcher'):

• ContainerBuilder->get('event_dipatcher') uses ~0.84 milliseconds.
• In ContainerBuilder->createService(), the biggest part is 121 calls to EventDispatcher->addListener(), which takes ~0.78 milliseconds.
• Loading the actual class file for EventDispatcher.php takes no time, because with drush cr, that class is already loaded.
• Unfortunately, we can only measure ContainerBuilder, not the real Container, unless we fix the compilation.

So this means, with ContainerBuilder, with the patch, there is a cost proportional to the number of listeners.
It seems that ~150 listeners cost 1 millisecond.
Far from dramatic, but still worth avoiding.

How would this number look like in the real container?
It might be smaller, but it would still grow proportionally with the number of listeners, because we still need to call ->addListener() that many times.

The above only profiles getting the event dispatcher from the container.
This is relevant on requests that only fire a few events, which might be the majority of all requests.

I don't think it makes sense to profile total page load time, the difference would disappear in the margin of error.

However, if we do want to profile page load, we could pump up the subscribers/listeners to a significant number. E.g. 150.000 listeners would mean ~1 second to get the dispatcher, and this would definitely make a difference in the page load time.
In the same spirit, we can fire a high number of different events, or fire a single event a high number of times, and compare page load time with and without.
But for all of this, we should fix the compiler first, so that we profile the real Container and not ContainerBuilder.

In a real-world project, this number could grow quite a lot. But would it grow significantly > 1000?

When we originally added this, hooks to events was still in contention, but I think it almost entirely out of contention now with hux-style hooks, we might even be able to convert some core events (back) to hooks once that lands. So I think it's probably fine to assume that we'll have core subscribers to Symfony events, and some contrib subscribers to Symfony events, and some of the existing core/contrib events for a while, but not a big expansion from where we are.

To add to my previous post with the profiling:
I did not have all the core modules enabled when i ran this test. So the number 121 would be higher.

I now tested with a real-world project where I get 245 listeners/subscribers.

There might be some contrib modules that make excessive use of events.
For a regular project we might lose one or two milliseconds if we go with symfony EvenDispatcher, but it would also depend on the infrastructure.

My opinion: If we can make the ContainerAwareEventDispatcher do what we need, let's continue working with that. At least in 📌 Support #[AsEventListener] attribute on classes and methods Active I will see what we can do with ContainerAwareEventDispatcher.

@donquixote is that just with container builder or also with the compiled container?

@donquixote is that just with container builder or also with the compiled container?

The profiling was done with ContainerBuilder, because I was not able to make it work with the compiled container.
But the calls to ->addListener() won't go away with the compiled container.
So there is still going to be a linear cost factor with the number of subscribers.

But actually I should be able to make it work with the compiled container!
I will give it a try now.

Actually, now with the compiled container, I get ~0.12 milliseconds for the 121 calls.
This is much better, at the point where we no longer have to worry about it.

It works now thanks to ✨ Support ServiceClosureArgument in \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::dumpValue Fixed .
But we still need to change 'kernel.event_subscriber' to 'event_subscriber' in RegisterListenersPass, unless we want to rename the tag everywhere else.

More soon.

But we still need to change 'kernel.event_subscriber' to 'event_subscriber' in RegisterListenersPass, unless we want to rename the tag everywhere else.

The ability to configure the tags used by the compiler pass was removed in https://github.com/symfony/symfony/pull/41937 and the reasoning listed in https://github.com/symfony/symfony/pull/40468 - "nobody uses this possibility anyway"

However the suggestion there is a good workaround for us, without having to copy/paste the entire class: we add another pass that either renames or adds kernel.event_subscriber to all services that are tagged with event_subscriber.

However the suggestion there is a good workaround for us, without having to copy/paste the entire class: we add another pass that either renames or adds kernel.event_subscriber to all services that are tagged with event_subscriber.

Good point.

Somehow it fails, maybe because core is not using tags or pass should come later

The related CR https://www.drupal.org/node/3357408 →

PHP Fatal error:  Uncaught Error: Cannot use object of type Symfony\Component\DependencyInjection\ChildDefinition as array in /var/www/html/core/lib/Drupal/Core/DependencyInjection/Compiler/RenameTagsPass.php:44
Stack trace:
#0 /var/www/html/vendor/symfony/dependency-injection/Compiler/Compiler.php(80): Drupal\Core\DependencyInjection\Compiler\RenameTagsPass->process(Object(Drupal\Core\DependencyInjection\ContainerBuilder))
#1 /var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php(767): Symfony\Component\DependencyInjection\Compiler\Compiler->compile(Object(Drupal\Core\DependencyInjection\ContainerBuilder))
#2 /var/www/html/core/lib/Drupal/Core/DrupalKernel.php(1335): Symfony\Component\DependencyInjection\ContainerBuilder->compile()

It looks more readable

I was looking for a ->deleteTag() :)
So ->clearTag() it shall be.

Somehow it fails, maybe because core is not using tags or pass should come later

It was a mistake.

Simplified the history.
This reveals that most of the required changes were already covered by the patch in #45.

The branch contains a commit that introduces profiling statements, which is later reverted.
I am curious about profiling results from other people.

I think we need a kernel test for event subscribers / listeners.

We have ContainerAwareEventDispatcherTest, but this only tests the class itself, not how it is used.
Event subscribers are covered indirectly by other tests, but we should have a dedicated test for this.

Test run took 45mins, which means no slowdown overall

Gonna profile tomorrow, but I bet if any regressions appear the whole test-suite exposed it

Looking at https://git.drupalcode.org/project/drupal/-/commit/553666589ad34a1b80bf7...

We can deprecate the event_subscriber tag and use autoconfiguraton with proper tag (kernel.event_subscriber) for SF dispatcher to minimize amount of renames

We can deprecate the event_subscriber tag and use autoconfiguraton with proper tag (kernel.event_subscriber) for SF dispatcher to minimize amount of renames

How do we deprecate a tag?

If we rename all the existing tags (which goes beyond #64), we should probably add a test to prove that the old tag still works.

Test run took 45mins, which means no slowdown overall

I think we should expect that the performance impact will be small compared to an average page load, and very small compared to other operations that happen during a test run.
The most relevant impact might be for very simple requests that are otherwise very fast, where one millisecond could make a measurable difference.
For a feature like this, the usual margin of error for page load time is higher than the difference we want to measure and optimize.

We should add a kernel test for event subscribers and listeners, and to cover the old tag.

It was initially renamed in #1824400: Rename the service tag 'kernel.event_subscriber' to just 'event_subscriber' →

OTOH we can use to check for EventSubscriberInterface of service and 📌 Support #[AsEventListener] attribute on classes and methods Active

Yes.
But we still need to decide what to do if we encounter the old tag:

1. Just rename the tag, and don't show a deprecation notice.
2. Show a deprecation notice and recommend the new tag.
3. Show a deprecation notice and recommend to use either EventSubscriberInterface with autoconfiguration, or the new tag.

And if we show a deprecation notice, we have to decide how and when it should be displayed.
I would prefer to have it only once on or after container rebuild, not on every request.
I also would want it to be in Drupal's log system.

I tried to call trigger_error(.., E_USER_DEPRECATED) during container rebuild, but on drush cr I saw nothing. Maybe wrong config in php? I think it is because we change the error handler during container rebuild. And anyway we should assume that messages from container rebuild don't reach the usual logger, because the logger requires a working container.

Other instances of deprecated service or deprecated tag produce a E_USER_DEPRECATED message every time the respective service is instantiated, not just once on container rebuild. This seems too much - or is it not?

If we want to trigger the E_USER_DEPRECATED after container rebuild, but not repeat it, we could set a container parameter with a collection of messages, and then have a mechanism that will log these messages once after a container rebuild. Then something in state to make sure it happens only once. But this seems too much for the scope of this issue.

It was initially renamed in #1824400: Rename the service tag 'kernel.event_subscriber' to just 'event_subscriber'

I do like the motivation for the rename.
But now I guess we have to go back to the old tag name.

OR we continue fully supporting the old tag name, without deprecation, but we recommend autoconfigure with the EventSubscriberInterface.

Just keep in mind, autoconfigure does not remove the tag, it only removes the need to explicitly add it to a service definition.

Thinking about this again..
I don't think we should deprecate the 'event_dispatcher' tag name.

It will cause lots of disruption for little gain.
Imagine all the issues and tickets that will be opened in contrib and in client projects.
The cost of renaming only happens on cache rebuild, and will vanish with autoconfigure.

Contrib and custom code can slowly move to autoconfigure. Until then they just use the old tag name.

Perhaps in the future, symfony will re-add the option to use a different tag name instead of 'kernel.event_subscriber'.

(To be clear: I am talking about the tag name here, _not_ about the ContainerAwareEventDispatcher class!)

New issue: 📌 Add kernel tests for event system RTBC
Let's add these tests _before_ we do this big change.

Actually, in https://github.com/symfony/symfony/pull/40468, Nicolas suggests to add a decorator instead of a separate pass.

A decorator would allow to only temporarily rename the tag, and then rename it back.
This would take more time during container rebuild, but would be more clean, in that we would only support the existing 'event_subscriber' tag.

With the current MR with the RenameTagPass, we run into the following "problems":

• We allow to use 'kernel.event_subscriber' and 'event_subscriber' interchangeably. This means if we ever want to go back to only supporting 'event_subscriber', we now have to add BC support 'kernel.event_subscriber'.
• Compiler passes that run _after_ the event subscriber pass and look for services tagged with 'event_subscriber' will find nothing. I don't know _why_ there would be any compiler pass like this, but what to I know.

Using a decorator to rename and then revert would solve these two problems, but at a cost of (a bit) slower rebuild.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased against 11.x

I am torn between whether changing the tag name is a good idea. On one hand it makes it easier for Symfony developers to use Drupal as it's the tag they already know. But on the other hand it is a lot of busy work for Drupal developers if we deprecate the existing tag just to add kernel. in front of it. It also seems like scope creep from the original issue where all we wanted to do was replace the dispatcher.

Perhaps the simpler solution is take inspiration from grumphp who had the same issue, and borrow their decorator code, which looks fairly trivial to implement: https://github.com/phpro/grumphp/blob/v2.x/src/Configuration/Compiler/Re...

Postponed 📌 Support #[AsEventListener] attribute on classes and methods Active on this issue.

Retitled this issue with proper name of class.

MR!6657 starts again from #45 and borrows the GrumPHP approach of decorating the Symfony compiler pass to temporarily rename the tag. It also renames it back again afterwards, just in case.

I think an IS update would help lesser gods, certainly like myself, to figure out what and why we're doing here.

Updated the IS. I don't think we need any additional testing but profiling the difference in instantiation time when a large number of subscribers are in use would be helpful.

Is profiling still required?

The comments from #26, #46, #47 are still valid. If @znerol still has the script that was mentioned that would help here, otherwise we need to port the script or write something new.

Leaving at NR in the meantime to try and get more eyes on this.

Except profiling it probably needs empty update hook to make sure container rebuild for 10.3

For minor-only things we've been skipping empty update hooks because there's always at least one non-empty update in a minor release, 10.3 has quite a lot already.

Used znerol's script and I see no difference in before and after on standard profile

missed to enable the module, but there's no viable diff

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

but there's no viable diff

So is this something to still pursue?

I think this is still worth pursuing, plus it will unlock further possible improvements such as 📌 Make event subscribers private by default Needs review which will let us inline most subscribers as noted in #5.

Tried to do some profiling in xhprof and I don't think there's much in it at all. Symfony has split the work across more methods but we can still compare them.

After cache rebuild, first page load:

Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch: 12 calls, 619µs

vs

Symfony\Component\EventDispatcher\EventDispatcher::dispatch: 12 calls, 57µs
Symfony\Component\EventDispatcher\EventDispatcher::optimizeListeners: 8 calls, 97µs
Symfony\Component\EventDispatcher\EventDispatcher::callListeners: 11 calls, 452µs
Total: 606µs

Second page load:

Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch: 9 calls, 583µs

vs

Symfony\Component\EventDispatcher\EventDispatcher::dispatch: 9 calls, 46µs
Symfony\Component\EventDispatcher\EventDispatcher::optimizeListeners: 5 calls, 61µs
Symfony\Component\EventDispatcher\EventDispatcher::callListeners: 9 calls, 426µs
Total: 533µs

Invoking the actual service is done differently in both - inside a closure in Symfony, calling Container::get() then call_user_func() from dispatch() in Drupal but as far as I can see these look comparable as well.

In terms of initially constructing the event dispatcher there is a difference: Symfony has 116 calls to Symfony\Component\EventDispatcher\EventDispatcher::addListener() which take 76µs (first run) or 53µs (second run), these are not required with the Drupal dispatcher because we send the prebuilt set of listeners to the constructor. The constructors themselves take 1-2µs. But as per the above it seems that the Symfony dispatcher is slightly faster anyway which negates some of this difference.

Overall I don't think this is worth worrying about - it might be "an order of magnitude" slower to instantiate the dispatcher but we are talking about less than 100 microseconds here.

CR outdated, and I think it needs examples from conversions

Updated CR: https://www.drupal.org/node/3376090 →

Not sure what else there is to say, this shouldn't affect anyone unless they were replacing the dispatcher for some reason, which seems an unlikely thing to do?

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased.

Thank you!

If we end up doing 🌱 [META] Hooks via attributes on service methods (hux style) Active and go the 'hook as event' route there, then we'll have a lot more registered listeners and it's possible Symfony\Component\EventDispatcher\EventDispatcher::addListener() might turn into an issue after all, but... I think that's something we can look at if and when we get there.

alexpott → changed the visibility of the branch 2909185-deprecate-containerawareeventdispatcher-in to hidden.

Committed 9dbd2a6 and pushed to 11.x. Thanks!

We need a 10.3.x version of the MR as this does not apply cleanly to RegisterEventSubscribersPass

Spokje → changed the visibility of the branch 11.x to hidden.

Spokje → changed the visibility of the branch 10.3.x to hidden.

The 10.3.x version looks good.

Committed 6e73386 and pushed to 10.3.x. Thanks!

Spokje → closed merge request !7244

Spokje → changed the visibility of the branch 10.3.x to active.

longwave → closed merge request !6657

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

Previously, the subscribers were added as argument of event_dispatcher service. From where can I get a list?

@claudiu.cristea I suggest to register a custom service_collector → for the tag: event_subscriber in the container and then just build your own list.

OK, I found it.

In my case I want to deprecate a list of event constants because i want to switch to class fully qualified names. What I did:]

Registered a new compile pass acting after RegisterEventSubscribersPass

<?php

declare(strict_types=1);

namespace Drupal\cas\DependencyInjection;

use Drupal\cas\Event\CasPostLoginEvent;
use Drupal\cas\Event\CasPostValidateEvent;
use Drupal\cas\Event\CasPreLoginEvent;
use Drupal\cas\Event\CasPreRedirectEvent;
use Drupal\cas\Event\CasPreRegisterEvent;
use Drupal\cas\Event\CasPreUserLoadEvent;
use Drupal\cas\Event\CasPreUserLoadRedirectEvent;
use Drupal\cas\Event\CasPreValidateEvent;
use Drupal\cas\Event\CasPreValidateServerConfigEvent;
use Drupal\cas\Service\CasHelper;
use Drupal\Component\Utility\DeprecationHelper;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;

@trigger_error('The ' . __CLASS__ . ' is deprecated in cas:3.0.0 and is removed from cas:3.1.0. No replacement is provided. See https://www.drupal.org/node/3462792', E_USER_DEPRECATED);

/**
 * Compiler pass to detect deprecated event constants.
 *
 * @deprecated in cas:3.0.0 and is removed from cas:3.1.0. No replacement is
 *   provided.
 *
 * @see https://www.drupal.org/node/3462792
 */
class DeprecatedEventConstants implements CompilerPassInterface {

  /**
   * List of deprecated constant names and their replacement.
   *
   * @var string[]
   */
  private const DEPRECATE_EVENT_IDS = [
    CasHelper::class . '::EVENT_PRE_USER_LOAD' => CasPreUserLoadEvent::class,
    CasHelper::class . '::EVENT_PRE_USER_LOAD_REDIRECT' => CasPreUserLoadRedirectEvent::class,
    CasHelper::class . '::EVENT_PRE_REGISTER' => CasPreRegisterEvent::class,
    CasHelper::class . '::EVENT_PRE_LOGIN' => CasPreLoginEvent::class,
    CasHelper::class . '::EVENT_PRE_REDIRECT' => CasPreRedirectEvent::class,
    CasHelper::class . '::EVENT_PRE_VALIDATE_SERVER_CONFIG' => CasPreValidateServerConfigEvent::class,
    CasHelper::class . '::EVENT_PRE_VALIDATE' => CasPreValidateEvent::class,
    CasHelper::class . '::EVENT_POST_VALIDATE' => CasPostValidateEvent::class,
    CasHelper::class . '::EVENT_POST_LOGIN' => CasPostLoginEvent::class,
  ];

  /**
   * {@inheritdoc}
   */
  public function process(ContainerBuilder $container): void {
    $definition = $container->getDefinition('event_dispatcher');
    $arguments = DeprecationHelper::backwardsCompatibleCall(
      \Drupal::VERSION,
      '10.3',
      fn(): array => array_map(fn(array $call): string => $call[1][0], $definition->getMethodCalls()),
      fn(): array => array_keys($definition->getArgument(1)),
    );

    $deprecated = [];
    foreach (self::DEPRECATE_EVENT_IDS as $constantName => $eventClass) {
      if (in_array(constant($constantName), $arguments, TRUE)) {
        $deprecated[] = $constantName;
      }
    }

    if ($deprecated) {
      $message = sprintf(
        'The following event constants are deprecated: %s. Use the corresponding event class fully qualified names instead: %s. See https://www.drupal.org/node/3462792',
        implode(', ', $deprecated),
        implode(', ', array_map(
          fn(string $constantName): string => $constantName . ' -> ' . self::DEPRECATE_EVENT_IDS[$constantName],
          $deprecated,
        )),
      );
      \Drupal::getContainer()->get('logger.factory')->get('cas')->warning($message);
      @trigger_error($message, E_USER_DEPRECATED);
    }
  }

}