8000 Adding option to allow repositories to be fetched from the service container by weaverryan · Pull Request #727 · doctrine/DoctrineBundle · GitHub
[go: up one dir, main page]

Skip to content

Adding option to allow repositories to be fetched from the service container #727

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

Merged
merged 2 commits into from
Nov 5, 2017

Conversation

weaverryan
Copy link
Contributor
@weaverryan weaverryan commented Oct 29, 2017

Hi guys!

Sorry again for the noise :). The goal is to allow repositories to be added to the container easily (i.e. in an autowired way). There are 2 possible approaches:

1) In symfony/symfony#24693, we add a new trait to make creating services that are similar to repositories easily.

  1. In this PR, we allow for the $em->getRepository() to return a service, instead of it being created by Doctrine.

I'm currently leaning towards the approach in this PR, as it keeps only one idea of a "repository" and migrating to the new system would be super easy. This idea was inspired by @beberlei in #725. I have a few pending questions:

Answered: #727 (comment)
A) Can an entity only be managed by exactly one EntityManager (as you seem to suggest)? This PR (which uses your code) suggests this.

B) Are there any issues with Registry::getManagerForClass()? It loops over all of the EM's to find the correct one. Is there a possible issue with instantiating (thanks to this loop) other EM's?

Example

The feature is automatically enabled. But existing repositories will still be created the "default" way.

The users will configure their repositoryClass like normal. But now, repositoryClass is allowed to be a service id (which usually == the class name these days, but that's not important).

/**
 * @ORM\Entity(repositoryClass="App\Repository\CoolStuffRepository")
 */
class CoolStuff
{

}

Your repository class will (with default config) auto-registered as a service. So, without touching any config, you create the class and it just works:

class CoolStuffRepository extends ServiceEntityRepository /* this class extends EntityRepository, implements ServiceEntityRepositoryInterface */
{
    public function __construct(RegistryInterface $registry)
    {
        parent::__construct($registry, CoolStuff::class);
    }

    // your custom methods
}

That's it! The new ContainerRepositoryFactory is capable of fetching repositories from the container or creating them like normal. How does it know which to do?

A) If your class still extends EntityRepository, then it's created like normal. This is a great BC/upgrade path.
B) If your class implements a new ServiceEntityRepositoryInterface, this is a signal to fetch from the container.

The entire system is functionally-tested, including strict errors if you mess up (e.g. implement the new interface, but forget to register your service).

Your new services are required to extend EntityRepository, because outside systems could be type-hinting against this. If you don't like that, you could ignore this feature and create your own service classes outside of the repository system.

Cheers!

@weaverryan weaverryan force-pushed the service_repositories_try2 branch 2 times, most recently from f4632fb to 09fe708 Compare October 29, 2017 02:55
Copy link
Member
@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of all the approaches I’ve seen, I like this one the best. Only thing that feels a bit kludgy is the constructor for the repository, see below.

@@ -64,6 +64,7 @@
<argument>%doctrine.default_entity_manager%</argument>
</service>
<service id="Doctrine\Common\Persistence\ManagerRegistry" alias="doctrine" public="false" />
<service id="Symfony\Bridge\Doctrine\RegistryInterface" alias="doctrine" public="false" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alias looks wrong here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea... this is a bit unrelated. The issue is that the existing alias is a bit difficult, because there is an abstract class in the Bridge by the same name. It’s one of those situations where the user needs to guess the correct use statement, and only one will work. I’d like to add this ORM-specific alias and tell people to target it (but also mention the other interface, for those who care).

@@ -375,6 +378,9 @@ protected function ormLoad(array $config, ContainerBuilder $container)
$def->addTag('doctrine.event_subscriber');
}
}

$container->registerForAutoconfiguration(EntityRepository::class)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn’t the tag only be applied to the new ServiceEntityRepository?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing so wuld allow removing the config option altogether, which would be nice, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... to remove the option entirely, it would mean that, when an entity has a custom repository, we would sometimes instantiate that class (like current behavior) and sometimes fetch it from the container. Effectively, if their repo extends the new ServiceEntityRepository, then we fetch from container (or error if not exists). Otherwise, we instantiate.

I like this because it makes migrating to the new system even easier and removes this option. But is it too magic? The choice of your base class will determine how your repo is fetched/created.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what autoconfig is already, adding behavior based on the base classes :)


class TestCustomRepoRepository extends ServiceEntityRepository
{
public function __construct(RegistryInterface $registry)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having to override the constructor is going to feel weird. People are likely to forget to override it or to change the signature. One option to avoid this is to loop through the metadata array from the entity manager and look for an entity that has our class as custom repository. It would be nice if we could even move that to the service itself to avoid looking up that information on every instantiation, but i believe changing this will improve DX.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The need to override the constructor is indeed the weirdest part :/ (but you only need to do it if you need to add more constructor args, otherwise the default constructor is fine). So, I’m interested in your idea, but I don’t understand yet. Where would we loop over the class metadata? And how would that help? Doesn’t the user need to (somehow) do some work/chores in their service to set the protected properties that EntityRepository needs internally?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that the constructor override was necessary to get the new repositories to play nice with service auto-registration. Since there will most likely be a repository for a single entity (unlike the default EntityRepository), it would make sense trying to deduct the entity class name from the entity manager in ServiceEntityRepository if it isn't given:

  • If entity class name is missing, fetch a list of all metadata from the document managers (yes, plural unfortunately).
  • Loop over metadata, looking for an entity with a customRepositoryClass of get_class($this).
  • If a single entity is found, use it. If more/less than one entity is foun 8000 d, throw an exception.

@weaverryan weaverryan force-pushed the service_repositories_try2 branch from 09fe708 to ca926d5 Compare October 29, 2017 16:56
@weaverryan weaverryan force-pushed the service_repositories_try2 branch from ca926d5 to 3af691b Compare October 30, 2017 18:20
@beberlei
Copy link
Member

@weaverryan to answer your questions:

a.) Theoretically, an entity can be in multiple managers, but I haven't seen someone using it this way and would consider it a 1% special case where its ok that repository as service + autowiring doesn't work. Other code in the DoctrineBundle will not work (Forms, Validators) if you are using one entity with multiple managers.

b.) You are right, this might cause additional overhead with multiple EMs, and you need to probably look out for issues with automatically connecting to databases, although afaik this problem has been fixed recently? Again, this method is used in form and validation component already, so any potential problems should have surfaced there already.

In general I think this approach works for the 80-90% use case and as a workaround users can just register the service manually to provide the correct entity manager.

Copy link
Member
@beberlei beberlei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct in assuming that this requires users to create a service for each repository themselves?

Is this desired or would an even better result be that repository services are automagically registered?

I don't have a good solution on how to implement this, but would be willing to think about it if you want this as a feature.


if (null !== $repositoryServiceId) {
if (!$this->container->has($metadata->customRepositoryClassName)) {
throw new \RuntimeException(sprintf('Could not find the repository service for the "%s" entity. Make sure the "%s" service exists and is tagged with "%s".', $entityName, $repositoryServiceId, ServiceRepositoryCompilerPass::REPOSITORY_SERVICE_TAG));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit harsh, wouldn't it be better to default to the original use by checking $customRepositoryClassName does not implement the ServiceEntityRepository and instantiate it with the regular approach?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way to do this would be remove the if (null !== $repositoryServiceId) check to get back into the default code starting in line 64+.

@weaverryan
Copy link
Contributor Author

Thank you for the answers!

Am I correct in assuming that this requires users to create a service for each repository themselves?
Is this desired or would an even better result be that repository services are automagically registered?

Yes, you're correct! But the registration of the services can already be automated with the "service auto-registration" feature in 3.3. And for repositories that don't have a custom class, auto-registering a service with an EntityRepository class doesn't help the user much anyways, as you wouldn't be able to autowire those services as arguments to other services. So, I think it's not a problem :).

I'm working on a few changes related to your comments and others right now...

@beberlei
Copy link
Member

@weaverryan can you point me to details of the "service auto-registration" feature of 3.3? I am still living in my small symfony 2 world ;-)

@weaverryan
Copy link
Contributor Author

@beberlei LOL, of course :).

New Symfony projects ship with this optional config: https://github.com/symfony/symfony-standard/blob/3.3/app/config/services.yml#L19-L23

This automatically registers every service in the container. The service id === the class name. Those services are private (important because it means that unused services are removed before dumping the compiled container... so no extra overhead if some classes in src/ are not actually services). Thanks to _defaults (https://github.com/symfony/symfony-standard/blob/3.3/app/config/services.yml#L8-L15), these services are also autowired (+ private and "autoconfigured" - fancy word that means we will try to add tags automatically based on the interface of services).

The Repository directory is excluded from the auto-registration (https://github.com/symfony/symfony-standard/blob/3.3/app/config/services.yml#L23). That's actually not needed, just a minor performance boost in dev. If this PR is merged, users would uncomment that and their repositories would instantly become services (well, autowiring will fail until they change their base class to the new, autowiring friendly ServiceEntityRepository from this PR... and stolen from you).

So, the user won't care much if their default "EntityRepository" repositories are in the container or not... using $em->getRepository() is even easier than manually fetching/wiring up some service id.

But as soon as they do have a custom repo class, they'll want that to be auto-registered so that it is available for autowiring. That's actually pretty simple - only the ServiceEntityRepository is necessary for that. Most of this PR is really centered around making $em->getRepository() also smart enough to now get things from the container so that there's consistency.

@weaverryan weaverryan force-pushed the service_repositories_try2 branch 4 times, most recently from 105454d to f7e14a5 Compare October 31, 2017 15:21
@weaverryan
Copy link
Contributor Author

Update! The config option is gone. The feature is now always on, but you need to activate it by making your repository implement a new interface. So, all existing repositories will work exactly like before.

The new functionality is still Symfony 3.3+ only, and clear exceptions are thrown if you're on a lower version. Actually, most of the ugliness in this PR is keeping 3.2 and lower compat. If we bumped the next version of this bundle to 3.3, we could remove all of that.

There are no current blockers. This is ready again for review.

// Symfony 3.2 and lower sanity check
if (!class_exists(ServiceLocatorTagPass::class)) {
if (!empty($repoServiceIds)) {
throw new RuntimeException(sprintf('The "%s" tag can only be used with Symfony 3.3 or higher. Remove the tag from the following services (%s) or upgrade to Symfony 3.3.', self::REPOSITORY_SERVICE_TAG, implode(', ', $repoServiceIds)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say "upgrade to Symfony 3.3 or higher".

@weaverryan weaverryan force-pushed the service_repositories_try2 branch from f7e14a5 to bec9174 Compare October 31, 2017 16:11
*/
if (class_exists(ServiceLocator::class)) {
$container->getDefinition('doctrine.orm.container_repository_factory')
->replaceArgument(0, (new Definition(ServiceLocator::class))->setArgument(0, array()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: use short array syntax.

This adds a new "marker" interface that is used to determine whether or
not the user determined for their repository to be fetched as a service
or created manually by the entity manager.

A lot of attention is given to throwing clear errors.
@weaverryan weaverryan force-pushed the service_repositories_try2 branch from bec9174 to 4efcb13 Compare November 2, 2017 18:49
@weaverryan
Copy link
Contributor Author

Nit-pick handled :)

@fabpot
Copy link
Member
fabpot commented Nov 5, 2017

FYI, I've bumped the version of the bundle to 1.8 before merging this PR, which will be part of 1.8.0 stable.

@fabpot fabpot merged commit b8eb928 into doctrine:master Nov 5, 2017
@weaverryan weaverryan deleted the service_repositories_try2 branch November 6, 2017 15:24
public function getRepository(EntityManagerInterface $entityManager, $entityName)
{
$metadata = $entityManager->getClassMetadata($entityName);
$repositoryServiceId = $metadata->customRepositoryClassName;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless I read something wrong, this variable doesn't seem to be used anywhere within the body of this method

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 years later 😆I agree with @Taluu , the variable doesn't seem to be used, as it used only used for the message of the RuntimeException that is thrown a few lines below.

But the $customRepositoryName class contains exactly the same value, and could be used instead. Maybe @weaverryan can tell more about it :)

@weaverryan
Copy link
Contributor Author

Hmm. Did you change your entity’s base class to the new ServiceEntityRepository? That should fix autowiring :)

@yceruto
Copy link
yceruto commented Nov 12, 2017

yep, I did, that's why the autowiring asks me by the $entityClass argument :/

@ogizanagi
Copy link
Contributor

@yceruto : Overwritting the ServiceEntityRepository::__construct to pass the $entityClass argument yourself in own PostRepository is a required step. I don't think we can/want to skip it, as that'll mean accessing the doctrine metadata during compile time to get the entity class associated to the repository service and also because the service constructor does belong to you, so you're free to not expose the $entityClass argument the same way or at all.

@yceruto
Copy link
yceruto commented Nov 13, 2017

@ogizanagi I thought extending from ServiceEntityRepository was enough (https://gist.github.com/weaverryan/eec33e4a6d3f98efd3b56939bc08697d), that means I need to do this also for each repository, right?

public function __construct(RegistryInterface $registry)
{
    parent::__construct($registry, CoolStuff::class);
}

javiereguiluz added a commit to symfony/demo that referenced this pull request Nov 24, 2017
…anywhere (yceruto)

This PR was merged into the master branch.

Discussion
----------

Make Doctrine repository as service and autowire/use it anywhere

Living in the edge of the [new awesome features like this](doctrine/DoctrineBundle#727), I'm proposing before doctrine-bundle 1.8 release, show/test this great feature that simplify our lives and to see how it works! and because soon, best practices should adopt this approach by default. (In fact [maker-bundle will do it](https://github.com/symfony/maker-bundle/blob/master/src/Resources/skeleton/doctrine/Repository.php.txt) <3)

What do you think?

Commits
-------

ceac1df Make repository as service and autowire/use it anywhere
@ostrolucky
Copy link
Member
ostrolucky commented Nov 25, 2017

Yes. Not a fan myself of this constructor overriding requirement. Not sure how is this

+use App\Entity\Company;
+use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
+use Doctrine\Common\Persistence\ManagerRegistry;
 
-class CompanyRepository extends Repository
+class CompanyRepository extends ServiceEntityRepository
 {
+    public function __construct(ManagerRegistry $registry, $entityClass)
+    {
+        parent::__construct($registry, Company::class);
+    }
+
 ////////////////
+use App\Entity\Project\Project;
+use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
+use Doctrine\Common\Persistence\ManagerRegistry;
 
-class ProjectRepository extends Repository
+class ProjectRepository extends ServiceEntityRepository
 {
+    public function __construct(ManagerRegistry $registry, $entityClass)
+    {
+        parent::__construct($registry, Project::class);
+    }
+
 ////////////////
+use App\Entity\User\UserAdmin;
+use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
+use Doctrine\Common\Persistence\ManagerRegistry;
 
-class UserAdminRepository extends Repository
+class UserAdminRepository extends ServiceEntityRepository
 {
+    public function __construct(ManagerRegistry $registry, $entityClass)
+    {
+        parent::__construct($registry, UserAdmin::class);
+    }
+
 ////////////////
+use App\Entity\User\UserClient;
+use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
+use Doctrine\Common\Persistence\ManagerRegistry;
 
-class UserClientRepository extends Repository
+class UserClientRepository extends ServiceEntityRepository
 {
+    public function __construct(ManagerRegistry $registry, $entityClass)
+    {
+        parent::__construct($registry, UserClient::class);
+    }
+
 ////////////////
+use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
+use Doctrine\Common\Persistence\ManagerRegistry;
 
-class UserPermissionRepository extends Repository
+class UserPermissionRepository extends ServiceEntityRepository
 {
+    public function __construct(ManagerRegistry $registry, $entityClass)
+    {
+        parent::__construct($registry, UserPermission::class);
+    }
+

Better than this:

services:
    repository_factory:
        public: false
        abstract: true
        factory: ['@doctrine', getRepository]

    App\Repository\CompanyRepository:
        parent: repository_factory
        arguments: [App\Entity\Company]

    App\Repository\UserAdminRepository:
        parent: repository_factory
        arguments: [App\Entity\User\UserAdmin]

    App\Repository\UserClientRepository:
        parent: repository_factory
        arguments: [App\Entity\User\UserClient]

    App\Repository\UserPermissionRepository:
        parent: repository_factory
        arguments: [App\Entity\Project\UserPermission]

    App\Repository\ProjectRepository:
        parent: repository_factory
        arguments: [App\Entity\Project\Project]

Can I have a go with different approach, where I get rid of this overriding and access metadata during compilation? I will do the benchmark as part of that.

@Koc
Copy link
Contributor
Koc commented Dec 13, 2017

@ostrolucky accessing metadata during compilation is not good idea.

@Koc
Copy link
Contributor
Koc commented Dec 13, 2017

@weaverryan why not create ServiceEntityRepository abstract with method getEntityClass? We could do not import ManagerRegistry in this case and do not override and call parent constructor.

@alcaeus
Copy link
Member
alcaeus commented Dec 15, 2017

accessing metadata during compilation is not good idea.

Please elaborate as to why it's a bad idea.

why not create ServiceEntityRepository abstract with method getEntityClass

IIRC, @weaverryan didn't want users to implement additional methods - it should "just work". On top of that, entities and repositories are tied together using the mapping (namely the repositoryClass option in the Entity mapping). There shouldn't be the need for the repository class to be tied to an entity in a second place. This is also a great antipattern, as from a Doctrine perspective an entity has one repository class, but a repository class may be used for multiple entities.

@Koc
Copy link
Contributor
Koc commented Dec 15, 2017

@stof explained why symfony/symfony#18673 (comment) and some time ago was some attempts to deprecate this behavior symfony/symfony#18728

@yceruto
Copy link
yceruto commented Dec 15, 2017

@weaverryan didn't want users to implement additional methods - it should "just work".

Users need override the __constructor method though to make it work.

There shouldn't be the need for the repository class to be tied to an entity in a second place.

It mean we've already a antipattern now parent::__construct($registry, CoolStuff::class);? We can reuse this repository for another entity?

@alcaeus
Copy link
Member
alcaeus commented Dec 15, 2017

@yceruto

It mean we've already a antipattern now parent::__construct($registry, CoolStuff::class);? We can reuse this repository for another entity?

Correct - I've also mentioned that this approach feels wrong in one of the pull requests involved with this feature. I still think that this shouldn't have to be handled by the user.

@alcaeus
Copy link
Member
alcaeus commented Dec 15, 2017

@Koc the issues you linked deal with fetching a service from an unbuilt container builder. Since this bundle is the place where the services in questions are generated, there are ways to work around this if you wanted to.

@bobvandevijver
Copy link
Contributor

I wonder why this is merged without any documentation, as it is actually I feature which is pretty awesome!

@xabbuh
Copy link
Member
xabbuh commented Mar 13, 2018

sayjun0505 added a commit to sayjun0505/sym_proj that referenced this pull request Apr 16, 2023
…anywhere (yceruto)

This PR was merged into the master branch.

Discussion
----------

Make Doctrine repository as service and autowire/use it anywhere

Living in the edge of the [new awesome features like this](doctrine/DoctrineBundle#727), I'm proposing before doctrine-bundle 1.8 release, show/test this great feature that simplify our lives and to see how it works! and because soon, best practices should adopt this approach by default. (In fact [maker-bundle will do it](https://github.com/symfony/maker-bundle/blob/master/src/Resources/skeleton/doctrine/Repository.php.txt) <3)

What do you think?

Commits
-------

ceac1df Make repository as service and autowire/use it anywhere
spider-yamet added a commit to spider-yamet/sym_proj that referenced this pull request Apr 16, 2023
…anywhere (yceruto)

This PR was merged into the master branch.

Discussion
----------

Make Doctrine repository as service and autowire/use it anywhere

Living in the edge of the [new awesome features like this](doctrine/DoctrineBundle#727), I'm proposing before doctrine-bundle 1.8 release, show/test this great feature that simplify our lives and to see how it works! and because soon, best practices should adopt this approach by default. (In fact [maker-bundle will do it](https://github.com/symfony/maker-bundle/blob/master/src/Resources/skeleton/doctrine/Repository.php.txt) <3)

What do you think?

Commits
-------

ceac1df Make repository as service and autowire/use it anywhere
markovlatkovic pushed a commit to markovlatkovic/recipes that referenced this pull request May 24, 2024
See doctrine/DoctrineBundle#727

But also, this directory was always safe to auto-register. If they cannot be autowired, unless you try to *use* them as services, they will be discarded without showing the autowiring failure.
markovlatkovic pushed a commit to markovlatkovic/recipes that referenced this pull request May 24, 2024
See doctrine/DoctrineBundle#727

But also, this directory was always safe to auto-register. If they cannot be autowired, unless you try to *use* them as services, they will be discarded without showing the autowiring failure.
frederickboyd added a commit to frederickboyd/frederickboyd that referenced this pull request May 25, 2025
…anywhere (yceruto)

This PR was merged into the master branch.

Discussion
----------

Make Doctrine repository as service and autowire/use it anywhere

Living in the edge of the [new awesome features like this](doctrine/DoctrineBundle#727), I'm proposing before doctrine-bundle 1.8 release, show/test this great feature that simplify our lives and to see how it works! and because soon, best practices should adopt this approach by default. (In fact [maker-bundle will do it](https://github.com/symfony/maker-bundle/blob/master/src/Resources/skeleton/doctrine/Repository.php.txt) <3)

What do you think?

Commits
-------

ceac1df Make repository as service and autowire/use it anywhere
mo-m AEDA elvin77 added a commit to mo-melvin77/recipes that referenced this pull request Jun 9, 2025
See doctrine/DoctrineBundle#727

But also, this directory was always safe to auto-register. If they cannot be autowired, unless you try to *use* them as services, they will be discarded without showing the autowiring failure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0