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.

Sign up for GitHub

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

Allow multiple doctrine entity manager instances #99

Closed
wants to merge 2 commits into from
Closed

Allow multiple doctrine entity manager instances #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57ce628
Allow multiple doctrine entity manager instances
Zayon Sep 10, 2018
2216d97
WIP
Zayon Sep 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 14 additions & 38 deletions src/Bridge/Doctrine/Persister/ObjectManagerPersister.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

namespace Fidry\AliceDataFixtures\Bridge\Doctrine\Persister;

use Doctrine\Common\Persistence\ManagerRegistry;
use Doctrine\Common\Persistence\Mapping\ClassMetadata;
use Doctrine\Common\Persistence\ObjectManager;
use Doctrine\ODM\MongoDB\Mapping\ClassMetadataInfo as ODMClassMetadataInfo;
Expand All @@ -27,36 +28,30 @@ class ObjectManagerPersister implements PersisterInterface
{
use IsAServiceTrait;

private $objectManager;

/**
* @var array|null Values are FQCN of persistable objects
*/
private $persistableClasses;
/** @var ManagerRegistry $managerRegistry */
private $managerRegistry;

/**
* @var ClassMetadata[] Entity metadata, FQCN being the key
*/
private $metadata = [];

public function __construct(ObjectManager $manager)
public function __construct(ManagerRegistry $managerRegistry)
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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 .

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@dkarlovi dkarlovi Oct 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

{
$this->objectManager = $manager;
$this->managerRegistry = $managerRegistry;
}

/**
* @inheritdoc
*/
public function persist($object)
{
if (null === $this->persistableClasses) {
$this->persistableClasses = array_flip($this->getPersistableClasses($this->objectManager));
}

$class = get_class($object);

if (isset($this->persistableClasses[$class])) {
$metadata = $this->getMetadata($class);
$manager = $this->managerRegistry->getManagerForClass($class);

if ($manager) {
$metadata = $this->getClassMetadata($manager, $class);

$generator = null;
$generatorType = null;
Expand All @@ -78,7 +73,7 @@ public function persist($object)
}

try {
$this->objectManager->persist($object);
$manager->persist($object);
} catch (ORMException $exception) {
if ($metadata->idGenerator instanceof ORMAssignedGenerator) {
throw ObjectGeneratorPersisterExceptionFactory::createForEntityMissingAssignedIdForField($object);
Expand All @@ -100,27 +95,9 @@ public function persist($object)
*/
public function flush()
{
$this->objectManager->flush();
}

/**
* @return string[]
*/
private function getPersistableClasses(ObjectManager $manager): array
{
$persistableClasses = [];
$allMetadata = $manager->getMetadataFactory()->getAllMetadata();

foreach ($allMetadata as $metadata) {
/** @var ORMClassMetadataInfo|ODMClassMetadataInfo $metadata */
if (false === $metadata->isMappedSuperclass
&& false === (isset($metadata->isEmbeddedClass) && $metadata->isEmbeddedClass)
) {
$persistableClasses[] = $metadata->getName();
}
foreach ($this->managerRegistry->getManagers() as $manager) {
$manager->flush();
}

return $persistableClasses;
}

protected function configureIdGenerator(ORMClassMetadataInfo $metadata): void
Expand All @@ -129,11 +106,10 @@ protected function configureIdGenerator(ORMClassMetadataInfo $metadata): void
$metadata->setIdGenerator(new ORMAssignedGenerator());
}

private function getMetadata(string $class): ClassMetadata
private function getClassMetadata(ObjectManager $manager, string $class): ClassMetadata
{
if (false === array_key_exists($class, $this->metadata)) {
$classMetadata = $this->objectManager->getClassMetadata($class);
$this->metadata[$class] = $classMetadata;
$this->metadata[$class] = $manager->getClassMetadata($class);
}

return $this->metadata[$class];
Expand Down
3 changes: 2 additions & 1 deletion src/Bridge/Symfony/Resources/config/doctrine_mongodb_odm.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
<service id="fidry_alice_data_fixtures.persistence.doctrine_mongodb.purger.purger_factory"
class="Fidry\AliceDataFixtures\Bridge\Doctrine\Purger\Purger"
lazy="true">
<!-- TODO: Change to <argument type="service" id="doctrine_mongodb" />-->
<argument type="service" id="doctrine_mongodb.odm.document_manager" />
</service>

Expand All @@ -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 type="service" id="doctrine_mongodb" />
</service>

</services>
Expand Down
3 changes: 2 additions & 1 deletion src/Bridge/Symfony/Resources/config/doctrine_orm.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
<service id="fidry_alice_data_fixtures.persistence.doctrine.purger.purger_factory"
class="Fidry\AliceDataFixtures\Bridge\Doctrine\Purger\Purger"
lazy="true">
<!-- TODO: Change to <argument type="service" id="doctrine" />-->
<argument type="service" id="doctrine.orm.entity_manager" />
</service>

Expand Down Expand Up @@ -79,7 +80,7 @@
<service id="fidry_alice_data_fixtures.persistence.persister.doctrine.object_manager_persister"
Zayon marked this conversation as resolved.
Show resolved Hide resolved
class="Fidry\AliceDataFixtures\Bridge\Doctrine\Persister\ObjectManagerPersister"
lazy="true">
<argument type="service" id="doctrine.orm.entity_manager" />
<argument type="service" id="doctrine" />
</service>
</services>

Expand Down
4 changes: 3 additions & 1 deletion src/Bridge/Symfony/Resources/config/doctrine_phpcr_odm.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,14 @@
<service id="fidry_alice_data_fixtures.persistence.doctrine_phpcr.purger.purger_factory"
class="Fidry\AliceDataFixtures\Bridge\Doctrine\Purger\Purger"
lazy="true">
<!-- TODO: Change to <argument type="service" id="doctrine_phpcr" />-->
<argument type="service" id="doctrine_phpcr.odm.document_manager" />
</service>

<service id="fidry_alice_data_fixtures.persistence.purger.doctrine_phpcr.odm_purger"
class="Fidry\AliceDataFixtures\Bridge\Doctrine\Purger\Purger"
lazy="true">
<!-- TODO: Change to <argument type="service" id="doctrine_phpcr" />-->
<argument type="service" id="doctrine_phpcr.odm.document_manager" />

<deprecated>The service "%service_id%s" is deprecated and will be removed in future versions. Use "fidry_alice_data_fixtures.persistence.doctrine_phpcr.purger.purger_factory" instead.</deprecated>
Expand All @@ -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 type="service" id="doctrine_phpcr" />
</service>

</services>
Expand Down
14 changes: 13 additions & 1 deletion tests/Bridge/Doctrine/Persister/ObjectManagerPersisterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
namespace Fidry\AliceDataFixtures\Bridge\Doctrine\Persister;

use Doctrine\Common\DataFixtures\Purger\ORMPurger;
use Doctrine\Common\Persistence\ManagerRegistry;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\ORMException;
use Doctrine\ORM\ORMInvalidArgumentException;
Expand All @@ -25,6 +26,7 @@
use Fidry\AliceDataFixtures\Bridge\Doctrine\Entity\MappedSuperclassDummy;
use Fidry\AliceDataFixtures\Exception\ObjectGeneratorPersisterException;
use Fidry\AliceDataFixtures\Persistence\PersisterInterface;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use ReflectionClass;

Expand All @@ -48,13 +50,23 @@ class ObjectManagerPersisterTest extends TestCase
*/
private $purger;

/**
* @var ManagerRegistry|MockObject
*/
private $managerRegistry;

/**
* @inheritdoc
*/
public function setUp()
{
$this->entityManager = $GLOBALS['entity_manager'];
$this->persister = new ObjectManagerPersister($this->entityManager);

$this->managerRegistry = $this->createMock(ManagerRegistry::class);
$this->managerRegistry->method('getManagerForClass')->willReturn($this->entityManager);
$this->managerRegistry->method('getManagers')->willReturn([$this->entityManager]);

$this->persister = new ObjectManagerPersister($this->managerRegistry);
$this->purger = new ORMPurger($this->entityManager);
}

Expand Down