Listeners __construct() aren't lazy called

[steevanb]

Hi !

In Symfony 3.1.1 (and maybe other versions), if i have this 2 listeners (this is just an example, with @service_container in arguments, i know) :

services:
    last_listener:
        class: Foo\LastServiceListener
        arguments: [ '%foo.bar%' ]
        tags:
            - { name: kernel.event_listener, event: kernel.request, priority: 17 }

    first_listener:
        class: Foo\FirstServiceListener
        arguments: [ '@service_container' ]
        tags:
            - { name: kernel.event_listener, event: kernel.request, priority: 16 }

namespace Huttosoft\Back\Bundle\ContextBundle\EventListener;

use Symfony\Component\DependencyInjection\ContainerInterface;

class FirstServiceListener
{
    /** @var ContainerInterface */
    protected $container;

    /**
     * @param ContainerInterface $container
     */
    public function __construct(ContainerInterface $container)
    {
        $this->container = $container;
    }

    public function onKernelRequest()
    {
      $this->container->setParameter('foo.bar', uniqid());
    }
}

namespace Huttosoft\Back\Bundle\ContextBundle\EventListener;

class LastServiceListener
{
    /**
     * @param bool $fooBar
     */
    public function __construct($fooBar)
    {
    }
}

Listeners priority aren't taken into account for __construct() call, and more generally, "all" listeners __construct() methods are called, "before" any onKernelEvent methods is called.

In var/cache/classes.php, in lazyLoad($eventName) method :

protected function lazyLoad($eventName)
{
    var_dump($eventName, $this->listenerIds[$eventName]);
    exit();
    if (isset($this->listenerIds[$eventName])) {
        foreach ($this->listenerIds[$eventName] as list($serviceId, $method, $priority)) {
            $listener = $this->container->get($serviceId);
            $key = $serviceId.'.'.$method;
            if (!isset($this->listeners[$eventName][$key])) {
                $this->addListener($eventName, array($listener, $method), $priority);
            } elseif ($listener !== $this->listeners[$eventName][$key]) {
                parent::removeListener($eventName, array($this->listeners[$eventName][$key], $method));
            $this->addListener($eventName, array($listener, $method), $priority);
        }
        $this->listeners[$eventName][$key] = $listener;
    }
}

Output, without useless listeners :

string(14) "kernel.request"
array(19) {
  [2]=>
  array(3) {
    [0]=>
    string(13) "last_listener"
    [1]=>
    string(15) "onKernelRequest"
    [2]=>
    int(17)
  }
  [3]=>
  array(3) {
    [0]=>
    string(14) "first_listener"
    [1]=>
    string(15) "onKernelRequest"
    [2]=>
    int(16)
  }
}

First problem : __construct() are not called by priority, but by registration in Container.
Second problem : FirstListener::onKernelRequest() can't do something required in LastListener::__construct().

I was waiting for this call tree :

• FirstService::__construct()
• FirstService::onKernelRequest()
• LastService::__construct()
• LastService::onKernelRequest()

But i have this one :

• LastService::__construct()
• FirstService::__construct()
• FirstService::onKernelRequest()
• LastService::onKernelRequest()

I know it's a huge change to lazy call __construct(), but could you at least call it by priority order, instead of Container registration order ?

Thanks !

[unkind]

First problem : __construct() are not called by priority, but by registration in Container.

They are called in descending order of priority value. Listeners with higher priority would be called earlier.

Second problem : FirstListener::onKernelRequest() can't do something required in LastListener::__construct().

They are lazy loaded, i.e. __construct is called only when event occurs.
However, it shouldn't work like you expect: at this stage you have compiled container and you can't dynamically change the parameter. Even if you don't have compiled container for some reason (custom container/project), this approach looks wrong.

[steevanb]

First problem : __construct() are not called by priority, but by registration in Container.

They are called in descending order of priority value. Listeners with higher priority would be called earlier.

Not for __construct(), see my example ;)

They are lazy loaded, i.e. __construct is called only when event occurs.

Yes, but not "one by one".

However, it shouldn't work like you expect: at this stage you have compiled container and you can't dynamically change the parameter. Even if you don't have compiled container for some reason (custom container/project), this approach looks wrong.

That's why i wrote "this is just an example". My problem is much more complicated to explain, so i took this example cause it's easy to understand.

[unkind]

Not for __construct(), see my example

This should not be relevant. It requires to revise the design if you depend on this order.

[steevanb]

I think it's not a design problem :)

Actually, we can't use service priority to defines something used by another service with a lower priority.
To me, that's a bug, priority is here for that too, no ?

[unkind]

The priority defines order of service calls. It has nothing to do with construction. Lazy loading is just performance trick and in fact construction can take place in undefined time point (similar to __destruct, by the way). If your logic depends on the __construct order, then you probably should fix it.

[allflame]

@steevanb I'm on board with @unkind. Only to me depending on order in which listeners are called is already a bad design (no matter that there is support for priority)

As for you actual problem, define class SomethingProvider with set...($something) get...() : something and use it within onKernelRequest to pass data across listeners - this should be less harmful and solve your problem. For some reason i'm pretty sure you just want to override constructor injection and my fair advice is you should never do that.

Considering your initial suggestion, can't you imagine that in parallel universe certain someone already has an application that depends on existing ordering and not the one you suggest?

[steevanb]

Ok, i don't agree with it, but it's ok :)

Another problem i see with this kind of code : if i have 50 kernel.request listeners, and first one stop propagation.
So, all 50 __construct() are called, with all their dependencies, but it's useless.

I'm not sure it's a good way to lazy load listeners (that's not really what i call lazy load), but i understand what you say :)

Thanks for your answers !