[FEATURE] EntityListenerInterface for autowiring

[DonCallisto]

As symfony >= 3.3 brings autowiring function, I think it could be somehow useful to have an "empty" interface called EntityListenerInterface to take advantage of the autowiring features. This way we can auto-tag all services that implements the interface with doctrine.entity_listener instead of doing it manually.

So, something like

// Doctrine\Bundle\DoctrineBundle\DoctrineBundle.php
/**
 * {@inheritDoc}
 */
public function build(ContainerBuilder $container)
{
    [...]
    $container
        ->registerForAutoconfiguration(EntityListenerInterface::class)
        ->addTag('doctrine.entity_listener');
}

Of course this will not be compatible with all sf supported version so we should think how include it properly (checking sf version, or checking method definition, and so on) or leave it for a release were sf minimum supported version is compliant with this feature.

WDYT?

[weaverryan]

These empty interfaces isn’t ideal. But, the entity listeners are already kinda magic, where iirc Doctrine looks for specific methods by name. So, a marker interface like this wouldn’t bother me.

However, there is a problem if you want to specify this on a different entity manager other than default. In that case, you would have to tag the service manually. That’s ok, but thanks to autoconfigure, you would now have 2 tags. I think this could be fixed by adding some special attribute to the autoconfgure tag, then removing that tag in the compiler pass, if there are multiple.

If you want to open a PR, you can. I like the idea, but I’m not in charge here :).

[DonCallisto]

Ok, I'll wait for other to check and validate our idea and then try to write an implementation. Thanks for reviewing this.

[kimhemsoe]

We can not use the shared interfaces for autowiring as they conflict with ODM and etc. It is a mistake that we already accept some of them and i want to kill them off as soon as i can.

I agree with @weaverryan that a empty interface is not something we should do. If we had a subscriber interface for ORM only and we could solve the "which entity manager" issue i would be quite open for the feature.

[weaverryan]

and we could solve the "which entity manager"

I think we can solve this

If we had a subscriber interface for ORM only

Yea, that would be best. So, if you really want this feature (it would be cool), we would need to propose that to the ORM first I think.

Btw, there is something easier that can be done now. Doctrine does have normal event subscribers (with an interface). But these do not play well with autoconfiguration yet. That could be done now, entirely in this bundle.

[DonCallisto]

Doctrine does have normal event subscribers (with an interface). But these do not play well with autoconfiguration yet. That could be done now, entirely in this bundle.

How the subscribers comes in handy here?

Another thought I had is, can't we leverege on annotations/configuration for the entities with EntityListeners? Maybe in a CompilerPass?

[alcaeus]

Looking at this for a future release. I like the idea of an interface for autoconfiguration, but the entityManager, event and entity attributes of the tag bother me a bit. When tagging manually, you can tag the listener for multiple combinations of managers and entity classes:

<tag name="doctrine.orm.entity_listener" event="preUpdate" entity="User" />
<tag name="doctrine.orm.entity_listener" entity="Group" entity_manager="custom" />

This would register the same service for the preUpdate event for the User entity in the default document manager and the Group entity for all events in the custom entity manager. However, you can't replicate this with methods like getEntityManager, getEvent, and getEntity in the listener interface.

Opinions @DonCallisto @weaverryan?

[DonCallisto]

@alcaeus autoconfiguration doesn't prevent you to explicitly define this kind of attributes so it should simply not be an issue.

[fabpot]

Having auto-configuration would be great! It's one of the last class that is not auto-configured by Symfony/Doctrine.

[alcaeus]

@fabpot I'll add this for DoctrineBundle 1.12 which will come out with Symfony 4.4. If anyone wants to contribute this for Hacktoberfest, please go ahead!

[DonCallisto]

If we decide how to proceed I can try to implement something. From what I got with this thread, the main "logical issues" to be faced are:

• Using or not an empty interface (only for ORM and not ODM?)
• The role of Compiler Pass (remove duplicate definitions?)
• The attributes attached to the tagged services
• Others ... ?

[alcaeus]

1. Using an empty interface is not unprecedented, see ServiceEntityRepositoryInterface. The interface will only cover ORM, as MongoDB ODM doesn't know any entity (document) listeners yet. See note 3 though.
2. The compiler pass already exists: https://github.com/doctrine/DoctrineBundle/blob/1.12.x/DependencyInjection/Compiler/EntityListenerPass.php
3. For the entity_manager, entity, event, method, and lazy options we need to find a solution. This could either be done by using static methods in the interface, or by having a single method return a EntityListenerConfiguration object that returns the attributes for the listener.

Especially the last one is where I'm not sure what is best - any opinions @doctrine/team-symfony-integration?

[nicolas-grekas]

What about this?

/**
 * A marker interface for Doctrine entity listeners.
 *
 * Add a "public static getDefaultEntityListenerConfiguration(): array" method to define default
 * attributes for the listener, which should return an array with keys:
 *  - entity: App\Entity\TheEntityClassToListenTo
 *  - ...
 */
interface EntityListenerInterface
{
}

[alcaeus]

So you would have an empty interface and then check whether a method exists to fetch the configuration? What's the advantage of that compared to separate interfaces?

[nicolas-grekas]

interface ConfiguredEntityListenerInterface extends EntityListenerInterface
{
    public static getDefaultEntityListenerConfiguration(): array;
}

would work too - not inheriting wouldn't as instanceof checks would then be misleading. maybe we can put getDefaultEntityListenerConfiguration inside EntityListenerInterface if forcing one to create the method and return []; is not considered cumbersome.

[alcaeus]

maybe we can put getDefaultEntityListenerConfiguration inside EntityListenerInterface if forcing one to create the method and return []; is not considered cumbersome.

I prefer having explicit interfaces and document them properly. We've previously had instances where people need to add empty methods without thinking about them and I'd rather avoid that.

The only change I would make is returning a value object instead of an array (with a simple constructor in the value object) so we're not passing around arrays anymore.

[nicolas-grekas]

The only change I would make is returning a value object instead of an array (with a simple constructor in the value object) so we're not passing around arrays anymore.

I've objected this on Symfony as this is for DI wiring: it doesn't make much sense to me to create semantic types (a VO) to configure a hash map.

[alcaeus]

On second thought, you're right: the configuration would simply be the same thing we're getting when getting the corresponding tag from the service. An array will do just fine. Thanks!

[DonCallisto]

So this would be fine https://github.com/doctrine/DoctrineBundle/issues/821#issuecomment-543726388 except from the following consideration

maybe we can put getDefaultEntityListenerConfiguration inside EntityListenerInterface if forcing one to create the method and return []; is not considered cumbersome.

Am I right?

[alcaeus]

I like the last suggestion here:

interface ConfiguredEntityListenerInterface extends EntityListenerInterface
{
    public static getDefaultEntityListenerConfiguration(): array;
}

[ostrolucky]

Bear in mind this needs to be able to handle multiple entities in separate methods of same class. Also not sure name is still correct, looks more like subscriber to me.

[VincentLanglet]

This auto-configuration would be great ! Is this still on the road map ?

[dmaicher]

Is this still relevant now that we have AsDoctrineListener (upcoming with 2.8.0) and AsEntityListener attributes?

[stof]

No. The attribute is a better implementation than an empty marker interface