Introduce Container aware entity listener resolver in order to use entity listener as service

[egeloen]

Hey guys!

I'm currently moving some logic inside the new entity listeners introduced in Doctrine 2.4. The bundles supports this feature by allowing to define our own entity listener resolver service in the configuration. The issue about that is you have to redefine all the logic in order to be able to use entity listener as service.

For now, to be able to use entity listener as service, I have created a ContainerAwareEntityListenerResolver which maps class names to service id through a compiler pass. When it resolves the entity listener, if the class names exists, I just return the service else it fallbacks on the Default entity listener resolver behavior.

My question is: do you think this feature can be natively supported by the bundle? If yes, I can share my work in a PR.

[JEDIBC]

@egeloen : I would gladly have a look at your code. I'm struggling searching how to use EntityListenerResolver with symfony DIC

[egeloen]

Here, the code I currently use to solve this issue:

• EntityListenerResolver:

class ContainerAwareEntityListenerResolver extends DefaultEntityListenerResolver implements ContainerAwareInterface
{
    /** @var \Symfony\Component\DependencyInjection\ContainerInterface */
    private $container;

    /** @var array */
    private $mapping;

    /**
     * Creates a container aware entity resolver.
     *
     * @param \Symfony\Component\DependencyInjection\ContainerInterface $container The container.
     */
    public function __construct(ContainerInterface $container)
    {
        $this->setContainer($container);

        $this->mapping = array();
    }

    /**
     * {@inheritdoc}
     */
    public function setContainer(ContainerInterface $container = null)
    {
        $this->container = $container;
    }

    /**
     * Maps an entity listener to a service.
     *
     * @param string $className The entity listener class.
     * @param string $service   The service ID.
     */
    public function addMapping($className, $service)
    {
        $this->mapping[$className] = $service;
    }

    /**
     * {@inheritdoc}
     */
    public function resolve($className)
    {
        if (isset($this->mapping[$className]) && $this->container->has($this->mapping[$className])) {
            return $this->container->get($this->mapping[$className]);
        }

        return parent::resolve($className);
    }
}

Register this class as service in the dic (for example doctrine.orm.container_aware_entity_listener_resolver and configure it in your config.yml:

doctrine:
    orm:
        entity_listener_resolver: doctrine.orm.container_aware_entity_listener_resolver

Then create & register the following compiler pass:

class DoctrineCompilerPass implements CompilerPassInterface
{
    /**
     * {@inheritdoc}
     */
    public function process(ContainerBuilder $container)
    {
        $definition = $container->getDefinition('doctrine.orm.container_aware_entity_listener_resolver');

        foreach ($container->findTaggedServiceIds('doctrine.entity_listener') as $service => $attributes) {
            $definition->addMethodCall('addMapping', array($container->getDefinition($service)->getClass(), $service));
        }
    }
}

Then just need to register an entity listener as service an tag it with doctrine.entity_listener & use it in you entity as you use it usually.

[JEDIBC]

Thank you !

Doctrine automatically use this doctrine.orm.container_aware_entity_listener_resolver ?

[egeloen]

Nope, as explain, you need to register it in your config.yml and then, Doctrine will use it.

[JEDIBC]

Oops I read too quickly, missed the config part. Thank you a lot !

[KeKs0r]

Can we maybe merge this into the bundle as default? In order to enable it without additional configuration?

[egeloen]

@KeKs0r I have not yet propose a PR as I don't really know how the Doctrine team would like to integrate it. I prefer waiting feedback instead of coding for nothing :)

[Ocramius]

@egeloen I think making your listener an explicit service and handle injections in your own service definitions would be much better. Making things "ContainerAware" is a mistake in almost all cases.

[stof]

@Ocramius the issue if we inject the instances themselves in the resolver is that it requires creating all listener when instantiating the EntityManager, even if the entity for which the listener is registered is only used in 1% of the requests. A ContainerAware implementation could keep it lazy. We could consider that lazy services are a solution though.

But anyway, a custom implementation is still needed as the default implementation in the ORM does not allow to register instances. It always create them

[Ocramius]

@stof it doesn't need to be container aware - I'm recently suggesting following approach (in a factory, for example):

public function createService($container) {
    $dependencyInstantiator = function () use ($container) {
         return $container->get('something-required-here');
    };

    return new SomeService($dependencyInstantiator);
}

There isn't a real "Aware" interface for this though...

[stof]

@Ocramius This requires making your class dependent on a callable rather than on its real dependency, and would require implementing a separate service with this createService method in it and returning the closure (the DI component is not able to create such closure automatically for you, as it is not something which can be enabled without changing the service class). The second case could become generic though by passing the service id to the closure builder though

[Ocramius]

@stof correct, it would make a dependency to the callable - It is not required for the container to create such a closure, but I think you can specify factory classes to allow this kind of logic, no?

[stof]

@Ocramius my point is that it requires making your own class handling lazy-instantiation of the object graph instead of making it accept its collaborator. This makes it harder to design the business logic. This is why lazy services are nicer for this.

[Ocramius]

@stof yes, still easier to test than a locator aware service though. I suppose that we agree on locator aware not being a good thing here...

[stof]

@Ocramius The difference being that the locator aware service would be 1 service in DoctrineBundle (I still agree that it is better to avoid it though) while the factory-aware services in your pattern are each listener registered (so the business logic in each app).

However, now that we have lazy-services in Symfony, I think it is fine to require injecting an instance itself. People don't caring about loading a complex object graph (or people writing a listener which does not bring a hude object graph with it) can inject as is. Others can define their listener as a lazy service.
And they can still decide to use your pattern if they don't like proxied services. However, I don't thikn we should provide an helper class for the callable factories as lazy services are now in core

[Ocramius]

Yes, this kind of "magic closure" building shouldn't be provided by the bundle.

@egeloen anything to object? I think this is a "won't fix"

[stof]

@Ocramius no it is not. the suggested solution based on injecting the container is rejected, but we still need to implement another solution in the bundle. Being able to register services for the listener is definitely a useful case

[vadim2404]

https://gist.github.com/vadim2404/9538227

Fix for people who wants use this feature now with Doctrine Bundle v1.2

Plus code that has been written before by @egeloen only without changes in config.yml

[micronax]

+1

[Nemo64]

push

[jmontoyaa]

+1

[pkruithof]

I've implemented this in #443

[kimhemsoe]

Fixed by #443