[EventDispatcher] Moves the logic of addSubscriberService to the compiler pass #12069
Conversation
|
Looks good to me. 👍 |
| foreach ($class::getSubscribedEvents() as $eventName => $params) { | ||
| if (is_string($params)) { | ||
| $definition->addMethodCall('addListenerService', array($eventName, array($id, $params), 0)); | ||
| } elseif (is_string($params[0])) { |
There was a problem hiding this comment.
You assume it's an array if it's not a string
There was a problem hiding this comment.
It's already the case in the ContainerAwareEventDispatcher (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/EventDispatcher/ContainerAwareEventDispatcher.php#L142-L152)
There was a problem hiding this comment.
I think this is a great opportunity to make sure it will not break 👍
|
This would require adding the class source files (yeah, plural because it requires considering all parent classes and traits as well) as container resources. Otherwise updating the list of event listeners in the subscriber will not be respected in the service definition (this is precisely why it was implemented this way: #2021 (comment)). Having a listener not registered because of a cache not being reloaded would be a huge WTF in dev (especially if we introduce it in 2.6 while it works previously), so I'm 👎 on it. and adding all listener classes as container resources is likely to have an impact in dev, as it means that the container will need to be rebuilt each time you change the source code of your listener.
Use statements don't trigger any autoloading, so they don't trigger file loads either. The only case where it would trigger an additional file load is when the imported class is used inside |
e9ab43a to
f13cbb9
Compare
|
I see the point for the cache... |
|
I'm closing this PR now as @stof has good arguments against this change. |
|
@stof, almost all of your arguments are about environment with debug enabled. But we can use different logic with and without |
|
@Strate I know they are. My point is that this change is making it impossible to implement a working debug mode. Having a totally different logic for the non-debug mode is possible, but it could lead to weird bugs appearing only in non-debug mode, so I'm not sure we want this. What do you think @fabpot ? |
|
I think it can break BC, if developers checks some of global state in public static function getSubscribedEvents()
{
if ($_COOKIE['blabla']) { // or getEnv(), or somethng else.
return [...]
}
} |
Currently when an event subscribers is registered as a to be lazy loaded service, the corresponding class (and so at least a file) is loaded and parsed even if this subscriber isn't used at all. And when you have a lot of events subscribers (with phpBB extensions we can easily have dozens of subscribers) it could represent a lot of files (due to the use statements we rarely have only one file to load) and on some systems (i.e if you don't have an opcode cache) it can be important on the performance side.