Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autoconfiguration #674

Closed
backbone87 opened this issue Jun 27, 2017 · 18 comments · Fixed by #1119
Closed

Autoconfiguration #674

backbone87 opened this issue Jun 27, 2017 · 18 comments · Fixed by #1119
Assignees
Labels
Projects
Milestone

Comments

@backbone87
Copy link

@backbone87 backbone87 commented Jun 27, 2017

Can we have the EventSubscriber autoconfigured?
And it would be also nice if entity listeners can be autoconfigured, maybe by creating a marker interface?

@Ocramius

This comment has been minimized.

Copy link
Member

@Ocramius Ocramius commented Jun 28, 2017

@backbone87

This comment has been minimized.

Copy link
Author

@backbone87 backbone87 commented Jun 28, 2017

Erm, i dont think we are on the same page, i talk about marker interface for http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/events.html#entity-listeners that are different from event listeners.

@stof

This comment has been minimized.

Copy link
Member

@stof stof commented Jun 28, 2017

we cannot autoconfigure entity listeners, as the tag requires additional info (the name of the entity on which it applies for instance)

@backbone87

This comment has been minimized.

Copy link
Author

@backbone87 backbone87 commented Jun 28, 2017

But that link is defined in the entity metadata? With the EntityListener annotation?

@codedmonkey

This comment has been minimized.

Copy link

@codedmonkey codedmonkey commented Dec 5, 2017

@stof Would it be ok to add auto-tagging for event subscribers since those don't require any additional arguments? It wouldn't be ideal to only have some of the tags autoconfigured, but it still makes sense for the simple use cases, especially since all we need to add is:

        $container->registerForAutoconfiguration(EventSubscriber::class)
            ->addTag('doctrine.event_subscriber');

Would love to know if such a pull request would be accepted.

@alcaeus

This comment has been minimized.

Copy link
Member

@alcaeus alcaeus commented Dec 5, 2017

Would it be ok to add auto-tagging for event subscribers since those don't require any additional arguments?

Not @stof, but no. The EventSubscriber interface is from Doctrine\Common and is valid for ORM, ODM and possibly a bunch of other object mappers. Adding the autoconfiguration you posted would also register event subscribers for other mappers in ORM which is not desirable.

The only option you can do this is by introducing a ORM-specific interface.

@codedmonkey

This comment has been minimized.

Copy link

@codedmonkey codedmonkey commented Dec 5, 2017

Thanks for the response. In that case I would be in favor of creating some interfaces inside this bundle that extend the original interfaces to "mark a class optimized for Symfony features". Auto-tagging not working for such an integral part of the Symfony framework feels like a miss to me.

@alcaeus

This comment has been minimized.

Copy link
Member

@alcaeus alcaeus commented Dec 7, 2017

TBH, I dislike the notion of marker interfaces. You can achieve the very same thing by putting a small prototype service:

        <prototype namespace="AppBundle\Doctrine\ORM\EventSubscriber\" resource="../../Doctrine/ORM/EventSubscriber/*">
            <tag name="doctrine.event_subscriber"/>
        </prototype>

However, since this is the bundle and Symfony folks have taken over, it's up to them to decide this. Opinions @weaverryan @fabpot?

@kimhemsoe kimhemsoe self-assigned this Mar 29, 2018
@kimhemsoe kimhemsoe added the Won't Fix label Mar 29, 2018
@kimhemsoe

This comment has been minimized.

Copy link
Member

@kimhemsoe kimhemsoe commented Mar 29, 2018

May reconsider in the future if a good solution comes or someone thinks otherwise.

@kimhemsoe kimhemsoe closed this Mar 29, 2018
@TomasVotruba

This comment has been minimized.

Copy link

@TomasVotruba TomasVotruba commented May 25, 2019

I was like WTF is autoconfigure broken? It's designed to not bother users with cases like:

services:
    # why this is not autoconfigured? :(
    Pehapkari\Doctrine\EventSubscriber\SetUploadDestinationOnPostLoadEventSubscriber:
        tags:
            - { name: doctrine.event_subscriber }

It's not, it's just missing for un-related reason. This subscriber needs only tag, nothing else.

@TomasVotruba

This comment has been minimized.

Copy link

@TomasVotruba TomasVotruba commented May 25, 2019

In case you have the same problem, put this in in your Kernel:

protected function configureContainer(ContainerBuilder $containerBuilder, LoaderInterface $loader): void
    {
        $containerBuilder->registerForAutoconfiguration(\Doctrine\Common\EventSubscriber::class)
            ->addTag('doctrine.event_subscriber');
@alcaeus

This comment has been minimized.

Copy link
Member

@alcaeus alcaeus commented May 25, 2019

It’s no WTF, I reiterate what I have said before: not every instance of an event subscriber is meant to be an ORM subscriber. This would cause a lot of issues for people running ORM and another mapper (e.g. MongoDB ODM) side-by-side.

@weaverryan

This comment has been minimized.

Copy link
Contributor

@weaverryan weaverryan commented May 26, 2019

It is a bit of an inconsistency, since almost everything else uses autoconfigure. I think we should consider (welcome someone to create the PR) a marker interface for the ORM specifically. We didn’t rush into tho for these reasons, but my opinion is that we should go for it.

And I think a marker interface for event subscribers or entity listeners and autoconfiguration are both valid ideas. If we’re going to do one, we might as well do both, but probably in separate PRs.

@alcaeus

This comment has been minimized.

Copy link
Member

@alcaeus alcaeus commented May 26, 2019

They can go into the same PR, I wouldn’t have a problem with that.

@latenzio

This comment has been minimized.

Copy link

@latenzio latenzio commented Jun 19, 2019

I prefer to add an example to the documentation how to implement auto configuration for this case:

security.yml

services:
    _instanceof:
        App\Doctrine\EventSubscriber:
            tags: [ doctrine.event_subscriber ]

App\Doctrine\EventSubscriber.php:

<?php

namespace App\Doctrine;

use Doctrine\Common\EventSubscriber as BaseEventSubscriber;

/**
 * Interface EventSubscriber to use for auto configuration
 *
 * @author Enrico Thies <et@mandarin-medien.de>
 */
interface EventSubscriber extends BaseEventSubscriber
{

}

Instead of implementing Doctrine\Common\EventSubscriber use App\Doctrine\EventSubscriber and everything works fine.

@lyrixx

This comment has been minimized.

Copy link
Contributor

@lyrixx lyrixx commented Dec 20, 2019

A collegue just hit "this issue".
If I do the PR, would you consider merging it?

@alcaeus

This comment has been minimized.

Copy link
Member

@alcaeus alcaeus commented Dec 20, 2019

Yes, absolutely! As Ryan suggested, we should add marker interfaces for both entity listeners and event subscribers 👍

@alcaeus

This comment has been minimized.

Copy link
Member

@alcaeus alcaeus commented Jan 9, 2020

Thanks to @lyrixx, this will be released with 2.1.0! 🎉

@alcaeus alcaeus added this to the 2.1.0 milestone Jan 9, 2020
@alcaeus alcaeus added Improvement and removed Won't Fix labels Jan 9, 2020
@alcaeus alcaeus added this to 2.1 in Roadmap Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
10 participants
You can’t perform that action at this time.