-
-
Notifications
You must be signed in to change notification settings - Fork 461
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
Conversation
f4632fb
to
09fe708
Compare
There was a problem hiding this 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" /> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
ofget_class($this)
. - If a single entity is found, use it. If more/less than one entity is foun 8000 d, throw an exception.
09fe708
to
ca926d5
Compare
ca926d5
to
3af691b
Compare
@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. |
There was a problem hiding this 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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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+.
Thank you for the answers!
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 I'm working on a few changes related to your comments and others right now... |
@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 ;-) |
@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 The So, the user won't care much if their default "EntityRepository" repositories are in the container or not... using 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 |
105454d
to
f7e14a5
Compare
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))); |
There was a problem hiding this comment.
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".
f7e14a5
to
bec9174
Compare
*/ | ||
if (class_exists(ServiceLocator::class)) { | ||
$container->getDefinition('doctrine.orm.container_repository_factory') | ||
->replaceArgument(0, (new Definition(ServiceLocator::class))->setArgument(0, array())); |
There was a problem hiding this comment.
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.
bec9174
to
4efcb13
Compare
Nit-pick handled :) |
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. |
public function getRepository(EntityManagerInterface $entityManager, $entityName) | ||
{ | ||
$metadata = $entityManager->getClassMetadata($entityName); | ||
$repositoryServiceId = $metadata->customRepositoryClassName; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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 :)
Hmm. Did you change your entity’s base class to the new ServiceEntityRepository? That should fix autowiring :) |
yep, I did, that's why the autowiring asks me by the |
@yceruto : Overwritting the |
@ogizanagi I thought extending from public function __construct(RegistryInterface $registry)
{
parent::__construct($registry, CoolStuff::class);
} |
…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
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. |
@ostrolucky accessing metadata during compilation is not good idea. |
@weaverryan why not create |
Please elaborate as to why it's a bad idea.
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 |
@stof explained why symfony/symfony#18673 (comment) and some time ago was some attempts to deprecate this behavior symfony/symfony#18728 |
Users need override the
It mean we've already a antipattern now |
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. |
@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. |
I wonder why this is merged without any documentation, as it is actually I feature which is pretty awesome! |
…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
…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
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.
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.
…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
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.
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.$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 withRegistry::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).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:
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!