[DependencyInjection] Fix register event listeners compiler pass

[X-Coder264]

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets
License	MIT
Doc PR

I've wanted to use the simpler event listener registration syntax (https://symfony.com/blog/new-in-symfony-4-4-simpler-event-listeners) in my project and it didn't work so I'm sending this fix.

We use the KnpPaginatorBundle bundle which also calls the RegisterListenersPass compiler pass in order to register with the event dispatcher their custom tags for listeners and subscribers (knp_paginator.listener and knp_paginator.subscriber).

Their compiler pass is TYPE_BEFORE_REMOVING and priority zero which is the same type and priority as the pass that gets added by FrameworkBundle. Since both the type and priority is the same the order of execution is undefined (because that is how regular sort behaves in PHP which is used by default by krsort) and the RegisterListenersPass currently removes the eventAliasesParameter parameter from the container if it is set (which is set here). So what happens in my app is that the Knp compiler pass runs first, the event_dispatcher.event_aliases parameter is removed and then the FrameworkBundle registered compiler pass runs and since the aliases are not present anymore the events do not get aliased properly. The event dispatcher service in the compiled container looks like:

$instance->addListener('Symfony\Component\HttpKernel\Event\RequestEvent', ...);

instead of the expected

$instance->addListener('kernel.request', ...);

This means that my listener never gets called on the kernel request event.

Another potential fix would be to adjust the Knp compiler pass priority, but seeing as that would fix only that bundle (who knows how many bundles out there have the same problem) and that I don't see any drawback in letting the event_dispatcher.event_aliases parameter stay in the container I think that this is better to fix here.

[nicolas-grekas]

/cc @dmaicher maybe?

[nicolas-grekas]

Oups, I meant /cc @derrabus actually

[X-Coder264]

What are the chances that this RFC got proposed at the same time as this PR?

https://wiki.php.net/rfc/stable_sorting

😂

[derrabus]

The PR removes the piece of code that removes the event aliases parameter from the container.

@nicolas-grekas Do you recall why the event aliases parameter is removed from the container by the pass? It has been there ever since the aliases have been introduced with 75369da.

[nicolas-grekas]

Oh, yes: non-removed parameters end up being inlined in the dumped container and available at runtime. Since this is not desired, the parameter is removed.

I still think this parameter should be removed before the container is dumped.

@X-Coder264 any other idea that would preserve the removal?

[X-Coder264]

@nicolas-grekas The only other solution that I see is to make the constructor $eventAliasesParameter to be null by default and then from the FrameworkBundle pass the parameter name explicitly. That way each caller gets to specify their event aliases parameter name and the pass will take care of removing only that parameter (instead of the current case where the default parameter name is the same as the one that FrameworkBundle sets so if any bundle calls this compiler pass it will remove the parameter that FrameworkBundle set which is not the expected behavior).

[nicolas-grekas]

The default would be to use the current parameter, and you would pass null to skip using it?
Looks interesting to me.

[X-Coder264]

The default would be null so that the pass does not fetch nor remove any parameter from the container and I'd pass here 'event_dispatcher.event_aliases' explicitly as the fourth parameter.

[nicolas-grekas]

Not possible, that would be a BC break.
Another option is to change the priority of some passes as you mentioned.

[X-Coder264]

What would exactly break in that scenario?

Changing the priority of the passes could also potentially break some applications.

That's why I went with the original approach to just remove the parameter removal logic, because that is a 100% safe code change and like I've said in the original post I don't see any issue with the parameter being left in the compiled container. Yes, maybe it's not ideal but it does not hurt anyone if that parameter remains in the container.

[nicolas-grekas]

Aliases would break for ppl that use the pass directly.

Another option: add a new compiler pass that gets a list of parameters to remove and register it last, after removing passes.

That'd be a new feature of course, so that the fix on your side would be to adjust the priority.

[X-Coder264]

Another option: add a new compiler pass that gets a list of parameters to remove and register it last, after removing passes.

Or I can add a new constructor bool parameter which would dictate if the parameter should be removed or not. It'd be false by default and we'd pass in true from FrameworkBundle so that the parameter will still get removed when using FrameworkBundle.

That'd be a new feature of course, so that the fix on your side would be to adjust the priority.

By new feature you mean it'd have to go to master instead of being treated as a bug fix so that it can go into 4.4? If so I disagree, since I can't fix the priority on my side since the issue is in a third party bundle (which means that I depend on the maintainers of that bundle to merge the fix and tag a new release). Also, that'd mean that the issue would still be present on 4.4 (with other bundles) and it is really hard to spot it in the first place so a lot of people could be affected even if they don't know it (especially if they don't write tests).

[nicolas-grekas]

OK, let's go this way.
We might want to be able to deprecate fetching some parameters at runtime so that we can remove them later on.

[derrabus]

We might want to be able to deprecate fetching some parameters at runtime so that we can remove them later on.

Wouldn't that be a feature worth investigating? Parameters that are available at compile-time, but won't be compiled into the dumped container?

[nicolas-grekas]

Issue created at #36831

[nicolas-grekas]

Thank you @X-Coder264.