@derrabus very nice! Event sorting always looks "heavy" when profiling Symfony apps.

About the framework.freeze_kernel_events config option. Could you please explain it a bit more? Why it's needed? It's just to provide a smooth upgrade path ... or is it intended for end users because this new behavior is "risky" in some cases and they may need to disable it?

Also, can you please tell us if this could have any impact in third-party bundles? Thanks.

About the framework.freeze_kernel_events config option. Could you please explain it a bit more? Why it's needed?

Applications might add event listeners at runtime, e.g.

$dispatcher->addListener(KernelEvents::REQUEST, function (RequestEvent $event): void {
    // do something
});

This is not possible anymore if we freeze this event. All listeners have to be registered through the container in that case. The setting lets you decide whether you want to be able to add listeners at runtime or not.

It's just to provide a smooth upgrade path ... or is it intended for end users because this new behavior is "risky" in some cases and they may need to disable it?

Enabling the setting is definitely risky and the application should be tested thoroughly afterwards.

Whether this setting enables a migration path (which means it’s always on in Symfony 6) or whether it remains permanently as a performance tweak is something we could decide on: I usually write my applications in a way they would work with this setting enabled. But I’ve seen quite a few applications that dynamically add listeners and I’m not 100% sure if migrating them to frozen events would be worth the effort. We could also think about flipping the default in Symfony 6, going from opt-in to opt-out.

Also, can you please tell us if this could have any impact in third-party bundles? Thanks.

In theory, enabling this setting could break third-party bundles if they register own event listeners for kernel events and not use the kernel.event_listener or kernel.event_subscriber tag for this purpose.

Thanks for the detailed explanation.

I have another question/comment: do you think that we could detect all this automatically so we don't have to define a config option? The idea would be:

• Assume that the app allows to do this (equivalent to framework.freeze_kernel_events = true)
• As soon as anyone in the code calls to $dispatcher->addListener() or $dispatcher->addSubscriber(), disable this option automatically (equivalent to framework.freeze_kernel_events = false).

The problem of differentiating when we call to these methods during compilation and when do others call them during runtime could be solved by adding addImmutableListener() and addImmutableSubscriber() methods so we can call those.

@javiereguiluz When compiling the dispatcher, I cannot know if someone might add a listener to a specific event at runtime later on. Also, my current implementation throws away all information on the priorities of the listeners. While I could certainly keep that information somehow, it would make the implementation more complex and we will certainly lose a bit of the performance boost gained here.

I agree with Javier we cannot make this optional - that's almost impossible to decide when you should set this option or not from an end user pov.
EventDispatcher from the contracts is immutable by design - and the one in the component is not. I think that could be our decider: we can have two dispatcher services, one per interface.
Then, we should ensure we never use the mutable one - if possible.
Thanks for starting this, that's a difficult refacto, but a nice one to me :)

What I could try is to compile the initial state of the event dispatcher into the container. One big array structure loaded via the constructor instead of many addListener() calls. That would save us a lot of method calls when booting up the dispatcher and (if no listener is added afterwards) the initial krsort call – without actually enforcing immutability.

But that would basically mean rewriting RegisterListenersPass from scratch.

Apart from that, I still think that if we want to move towards immutability, we should allow to configure this for each event individually. An immutable event dispatcher would be a performance gain, but it would also make the learning curve steeper for newcomers and break existing applications in non-trivial ways.

To me, considering the event_dispatcher service, there is never a good reason to mutate it.
Here could be a possible plan:
we make ImmutableEventDispatcher accept a constructor argument that tells it's OK to forward to the decorated dispatcher, but with a deprecation notice.
Then, we wire one as the default event_dispatcher service and we fix the deprecations.
In 6.0, we'll be able to move to a really immutable implementation.
And we keep the current EventDispatcher class as is - it's fine for standalone usages.
WDYT?

Additional idea: we add a parameter to opt-in for a really immutable dispatcher in 5.1, and we turn that param on in the recipe.

To me, considering the event_dispatcher service, there is never a good reason to mutate it.

I agree, yet people have done just that.

In 6.0, we'll be able to move to a really immutable implementation.

I have worked with a team recently that made heavy use of the event dispatcher for domain events. Because their application needed to be extensible for customization, they had developed a plugin mechanism that would add event listeners at runtime. I myself wouldn't have designed it that way, but it worked for them.

Now, if we would make the whole dispatcher immutable in 6.0, we'd probably lock them in 5.4 for quite a long time. I can imagine that other teams will have that problem as well. This is why with this PR I took the approach to only make certain events immutable, in this case the kernel events.

By sending the deprecations in 5.1, people will have 4.5 years to figure out how to improve their design.
We should do it IMHO, and we should start with our own code first to get the taste of it.

By sending the deprecation in 5.1, people will have 4.5 years to figure out how to improve their design.

The upgrade path is pretty much clear. All I‘m saying is that it might be a hard hit for certain applications.

We should do it IMHO, and we should start with our own code first to get the taste of it.

I agree. I‘ll have a look once I‘m back in Germany.

@derrabus Is it something you're still working on?

Not actively, but I'm still planning to resume my work later this year. Especially the changes to the Security components could make immutable event dispatching easier.

Thank you for the feedback. Let's close this PR for now then. I'm very much looking forward to reviewing a new PR in a few months implementing this feature.