Allow multiple doctrine entity manager instances #99

Zayon · 2018-09-10T16:38:28Z

Fixes #90, based on #96

theofidry

I'm 👍 for the feature, thank you a lot for the PR! I would however:

Try the suggestion regarding the constructor without which it's a BC break and would require a version bump which I would like to avoid if possible
I would like to see a test for it as well; You can add it to ObjectManagerPersisterTest. The DB configuration can be found in autoload.php and a make test_doctrine_bridge should run all the right tests without you having to worry about setting up dependencies (it does require you to install a local DB instance though

theofidry · 2018-09-10T20:08:54Z

src/Bridge/Symfony/Resources/config/doctrine_mongodb_odm.xml

@@ -71,7 +71,7 @@
        <service id="fidry_alice_data_fixtures.persistence.persister.doctrine_mongodb.object_manager_persister"
                 class="Fidry\AliceDataFixtures\Bridge\Doctrine\Persister\ObjectManagerPersister"
                 lazy="true">
-            <argument type="service" id="doctrine_mongodb.odm.document_manager" />
+            <argument type="service" id="doctrine" />


I suspect this would be a different one no? doctrine is for the ORM Registry isn't it?

Same comment for the two occurrences below

Should be doctrine_mongodb, same for the purger and purger_factory

theofidry · 2018-09-10T20:09:36Z

src/Bridge/Doctrine/Persister/ObjectManagerPersister.php

@@ -30,7 +31,8 @@
 {
    use IsAServiceTrait;

-    private $objectManager;
+    /** @var array|ObjectManager[]  */


you can omit array here

theofidry · 2018-09-10T20:12:45Z

src/Bridge/Doctrine/Persister/ObjectManagerPersister.php

@@ -42,9 +44,9 @@
     */
    private $metadata = [];

-    public function __construct(ObjectManager $manager)
+    public function __construct(ManagerRegistry $managerRegistry)


Maybe a more BC friendly way would be to inject directly the managers:

function __construct(ObjectManager ...$managers) { $this->managers = $managers; }

Since the service is lazy it shouldn't cause any issue. This may require an extra dependency with the expression language though

Having the ManagerRegistry allow to call $this->managerRegistry->getManagerForClass($class); directly, wich remove a lot of complexity. See below

Yeah @jdeniau is right, having the ManagerRegistry is nice to remove some complexity.

What should we do ? ObjectManager ...$managers with some complexity but no BC or ManagerRegistry $managerRegistry with reduced complexity but BC. Up to you @theofidry .

can the registry be instantiated here since we cannot inject it?

instantiated ? You mean make a new ManagerRegistry() inside the constructor ? it seems hard, see the service declaration : https://github.com/doctrine/DoctrineBundle/blob/master/Resources/config/dbal.xml#L59L65

Looking at the implementation: https://github.com/doctrine/persistence/blob/master/lib/Doctrine/Common/Persistence/AbstractManagerRegistry.php#L156

I don't think iterating over the managers is a bad idea in the end. Maybe you can create a custom ManagerRegistry class to be able to cache the results a bit like we are doing for the metadata, which you then can instantiate more easily

Thinking about it again, what about have a dedicated class implementing ObjectManager?

class ObjectsManager implements ObjectManager { function (ManagerRegistry $registry) { $this->registry = $registry; } function getClassMetadata(string $class): Metadata { $manager = $this->managerRegistry->getManagerForClass($class) $manager->getClassMetadata($class); } }

So that manager can take the registry just fine and we can inject this new manager without any BC breaks

~~I think ObjectManager[] would be the best approach since it allows to push managers from both ORM and ODM together, you can iterate them and treat them the same regardless where they came from.~~

Nope, injecting managers alone is not enough since you can't ask the manager if it is managing a specific class, the only place where this info is available is the registry.

Hi. I don't know if my comment would help but injecting the registry and getting a fresh manager each time you want to persist will also allow insert errors. Usually as soon as the database throws an error, the manager closes. The only way to re-open the connection is to get a fresh manager from the registry. Keeping in mind this I would suggest to inject the registry instead of all the managers.

theofidry · 2018-09-10T20:53:06Z

Note: I'm trying to fix the tests on master (it should be failures related to deprecation notices, but the tests themselves are fine so you can still see the results locally)

jdeniau

Feel free to compare with master...jdeniau:jd-feat-multipleDoctrineOrmV2 if you need help

jdeniau · 2018-09-11T08:09:18Z

src/Bridge/Doctrine/Persister/ObjectManagerPersister.php

@@ -53,7 +55,7 @@ public function __construct(ObjectManager $manager)
    public function persist($object)
    {
        if (null === $this->persistableClasses) {


You can use $objectManager = $this->managerRegistry->getManagerForClass($class); with remove all the complexity of persistableClass.

It returns ObjectManager|null

jdeniau · 2018-09-11T08:10:35Z

src/Bridge/Doctrine/Persister/ObjectManagerPersister.php

@@ -42,9 +44,9 @@
     */
    private $metadata = [];

-    public function __construct(ObjectManager $manager)
+    public function __construct(ManagerRegistry $managerRegistry)


Having the ManagerRegistry allow to call $this->managerRegistry->getManagerForClass($class); directly, wich remove a lot of complexity. See below

jdeniau · 2018-09-11T08:11:01Z

src/Bridge/Doctrine/Persister/ObjectManagerPersister.php

-            } catch (ORMException $exception) {
-                if ($metadata->idGenerator instanceof ORMAssignedGenerator) {
-                    throw ObjectGeneratorPersisterExceptionFactory::createForEntityMissingAssignedIdForField($object);
+            foreach ($this->managers as $manager) {


Then, as you have an entity manager, you just need to do $objectManager->persist($object).

jdeniau · 2018-09-11T08:12:04Z

src/Bridge/Doctrine/Persister/ObjectManagerPersister.php

@@ -130,8 +143,11 @@ private function getPersistableClasses(ObjectManager $manager): array
    private function getMetadata(string $class): ClassMetadata


And here, you can inject the ObjectManager and get metadata directly with $objectManager->getMetadata($class)

src/Bridge/Symfony/Resources/config/doctrine_orm.xml

jdeniau · 2018-09-11T08:14:33Z

src/Bridge/Symfony/Resources/config/doctrine_phpcr_odm.xml

@@ -73,7 +73,7 @@
        <service id="fidry_alice_data_fixtures.persistence.persister.doctrine_phpcr.object_manager_persister"
                 class="Fidry\AliceDataFixtures\Bridge\Doctrine\Persister\ObjectManagerPersister"
                 lazy="true">
-            <argument type="service" id="doctrine_phpcr.odm.document_manager" />
+            <argument type="service" id="doctrine" />


should be doctrine_phpcr, same with the purger and purger_factory

jdeniau · 2018-09-11T08:15:09Z

src/Bridge/Symfony/Resources/config/doctrine_mongodb_odm.xml

@@ -71,7 +71,7 @@
        <service id="fidry_alice_data_fixtures.persistence.persister.doctrine_mongodb.object_manager_persister"
                 class="Fidry\AliceDataFixtures\Bridge\Doctrine\Persister\ObjectManagerPersister"
                 lazy="true">
-            <argument type="service" id="doctrine_mongodb.odm.document_manager" />
+            <argument type="service" id="doctrine" />


Should be doctrine_mongodb, same for the purger and purger_factory

Zayon · 2018-09-11T15:04:48Z

I've made an update to the PR, it's still WIP but suggestions are welcomed.

Test it works with one manager
Test it works with multiple managers
Change Purgers and PugerFactories
Add a phpunit test

stof · 2018-09-12T12:27:57Z

src/Bridge/Doctrine/Persister/ObjectManagerPersister.php


    /**
-     * @var ClassMetadata[] Entity metadata, FQCN being the key
+     * @var string[ObjectManager] Keys are FQCN of persistable objects, Values are managers


I don't think that's a valid phpdoc type.

Yeah, I don't know if it's possible to set the type of keys

stof · 2018-09-12T12:29:17Z

src/Bridge/Doctrine/Persister/ObjectManagerPersister.php


-    public function __construct(ObjectManager $manager)
+    public function __construct(ObjectManager ...$managers)


technically, this allows to pass no manager at all (a variable argument is 0+, not 1+)

Well this was a suggestion from @theofidry as I injected the ManagerRegistry in my first commit.
With this and the fact that we can't inject %doctrine.entity_managers% it seems it's not possible to use ObjectManager ...$managers. Any thought ?

Well, if you want to keep a variadic constructor, you will need an argument to add all the relevant references as arguments in your service definition.

stof · 2018-09-12T12:29:33Z

src/Bridge/Doctrine/Persister/ObjectManagerPersister.php

+        $this->managers = $managers;
+
+        foreach ($managers as $manager) {
+            $allMetadata = $manager->getMetadataFactory()->getAllMetadata();


loading all metadata of a manager is bad for performance, as this is not an optimized API. The optimized API is the one getting metadata for a given class (as the class name is the cache key).

It would be better to build the map on demand, when being asked for a specific class, rather than trying to build it upfront

stof · 2018-09-12T12:32:29Z

src/Bridge/Symfony/Resources/config/doctrine_mongodb_odm.xml

@@ -71,7 +72,7 @@
        <service id="fidry_alice_data_fixtures.persistence.persister.doctrine_mongodb.object_manager_persister"
                 class="Fidry\AliceDataFixtures\Bridge\Doctrine\Persister\ObjectManagerPersister"
                 lazy="true">
-            <argument type="service" id="doctrine_mongodb.odm.document_manager" />
+            <argument>%doctrine_mongodb.odm.document_managers%</argument>


that won't work. As you made the constructor variadic, you need to add many arguments.

and btw, this parameters contains the names of document managers, not the objects (a parameter cannot contain references to services)

stof · 2018-09-12T12:32:43Z

src/Bridge/Symfony/Resources/config/doctrine_orm.xml

@@ -79,7 +80,7 @@
        <service id="fidry_alice_data_fixtures.persistence.persister.doctrine.object_manager_persister"
                 class="Fidry\AliceDataFixtures\Bridge\Doctrine\Persister\ObjectManagerPersister"
                 lazy="true">
-            <argument type="service" id="doctrine.orm.entity_manager" />
+            <argument>%doctrine.entity_managers%</argument>


same issue here than for the ODM

stof · 2018-09-12T12:32:54Z

src/Bridge/Symfony/Resources/config/doctrine_phpcr_odm.xml

@@ -73,7 +75,7 @@
        <service id="fidry_alice_data_fixtures.persistence.persister.doctrine_phpcr.object_manager_persister"
                 class="Fidry\AliceDataFixtures\Bridge\Doctrine\Persister\ObjectManagerPersister"
                 lazy="true">
-            <argument type="service" id="doctrine_phpcr.odm.document_manager" />
+            <argument>%doctrine_phpcr.odm.document_managers%</argument>


and again here

stof · 2018-09-12T12:34:47Z

Btw, the issues I reported are telling me that I don't agree with your checked "Test it works with one manager" (and Travis agrees with me, not with you, as CI is failing)

Zayon · 2018-09-12T13:23:37Z

Btw, the issues I reported are telling me that I don't agree with your checked "Test it works with one manager" (and Travis agrees with me, not with you, as CI is failing)

You are right, too much multi-tasking made me forgot to change the service declaration in my test env :(

Zayon · 2018-09-24T15:51:25Z

@theofidry Any idea on what is the cause of the fail ?

theofidry · 2018-09-24T18:41:37Z

No idea, would need to dig into it but cannot at the moment. I would suggest to carry on and I can dig into it after the reviews to see what is causing an issue

jdeniau · 2018-09-24T19:54:35Z

Might be a nice way of dealing with it. The interface is quite complex (13 methods), but all are probably not used if the class is internal

…

On Mon, Sep 24, 2018 at 8:41 PM Théo FIDRY ***@***.***> wrote: No idea, would need to dig into it but cannot at the moment. I would suggest to carry on and I can dig into it after the reviews to see what is causing an issue — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#99 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVWxRIhY5oF-Y4CXUGHBwZSFASufdcGks5ueSdigaJpZM4WhxAa> .

theofidry · 2018-09-24T19:55:35Z

Yes, you could throw a LogicException on those at first and we can change it later on whenever we want to

dkarlovi · 2018-10-11T14:27:01Z

This looks like it's mostly finished or am I missing something?

theofidry · 2018-10-11T14:42:17Z

It still needs https://github.com/theofidry/AliceDataFixtures/pull/99/files#r219945408 + making the tests greens, but otherwise yeah we are not far off having it merged

dkarlovi · 2018-10-11T14:47:08Z

Note, my first reason for asking about this was to allow me to load ORM and ODM fixtures at the same time (ODM references ORM).

Looking at hautelook:fixtures:load, it explicitly requires an ORM manager which means it's unusable in that regard? Would this mean adding a new command is the only way currently?

dkarlovi · 2018-10-11T14:50:18Z

BTW the ODM documents are already properly constructed by hautelook:fixtures:load, they're just not flushed, which is quite unfortunate.

theofidry · 2018-10-11T15:21:36Z

Would this mean adding a new command is the only way currently?

Yes, and potentially can be switched to be the new default command, i.e. orm, odm stay specific, but the default and general one loads everything, ORM, ODM and every manager

BTW the ODM documents are already properly constructed by hautelook:fixtures:load, they're just not flushed, which is quite unfortunate

I think there is an open issue about the ORM not having its manager properly propagated, i.e. the right manager is used for loading but not purging. I'm not aware of an issue for the ODM so if there is one an issue should be opened for it

dkarlovi · 2018-10-12T12:46:28Z

Here's a quick hack to illustrate the required points, continuation from our Slack conversation:
https://github.com/hautelook/AliceBundle/pull/440

For the feature to work, the new DoctrineLoader will need to have manager registries injected. I can just do the entire thing here, but it does reduce reusability of this code (since it would be doubled).

@theofidry do you have any comments there or here?

/cc @mleczakm

jdeniau · 2020-08-17T16:21:04Z

Hi, This should be fixed in #113

Zayon mentioned this pull request Sep 10, 2018

Allow multiple doctrine ORM instance #96

Closed

theofidry requested changes Sep 10, 2018

View reviewed changes

jdeniau reviewed Sep 11, 2018

View reviewed changes

theofidry mentioned this pull request Sep 11, 2018

Exclude more root files from the archive #102

Merged

stof suggested changes Sep 12, 2018

View reviewed changes

Zayon added 2 commits September 22, 2018 00:00

Allow multiple doctrine entity manager instances

57ce628

WIP

2216d97

dkarlovi mentioned this pull request Oct 17, 2018

Managers WIP #107

Closed

theofidry mentioned this pull request Oct 24, 2018

Add a persister and purger for Doctrine which operates on all known managers #108

Open

2 tasks

jdeniau mentioned this pull request Aug 17, 2020

Allow multiple entity manager #113

Open

Zayon closed this by deleting the head repository Mar 28, 2023

		@@ -130,8 +143,11 @@ private function getPersistableClasses(ObjectManager $manager): array
		private function getMetadata(string $class): ClassMetadata


		public function __construct(ObjectManager $manager)
		public function __construct(ObjectManager ...$managers)

Allow multiple doctrine entity manager instances #99

Allow multiple doctrine entity manager instances #99

Conversation

Zayon commented Sep 10, 2018 • edited Loading

theofidry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkarlovi Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry commented Sep 10, 2018

jdeniau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zayon commented Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Sep 12, 2018

Zayon commented Sep 12, 2018

Zayon commented Sep 24, 2018

theofidry commented Sep 24, 2018

jdeniau commented Sep 24, 2018 via email

theofidry commented Sep 24, 2018

dkarlovi commented Oct 11, 2018

theofidry commented Oct 11, 2018

dkarlovi commented Oct 11, 2018

dkarlovi commented Oct 11, 2018

theofidry commented Oct 11, 2018 • edited Loading

dkarlovi commented Oct 12, 2018

jdeniau commented Aug 17, 2020 via email • edited Loading

Zayon commented Sep 10, 2018 •

edited

Loading

dkarlovi Oct 12, 2018 •

edited

Loading

Zayon commented Sep 11, 2018 •

edited

Loading

theofidry commented Oct 11, 2018 •

edited

Loading

jdeniau commented Aug 17, 2020 via email •

edited

Loading