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

Jump to bottom

Added PersisterFactory to ORM. #1260

Closed
wants to merge 1 commit into from
Closed

Added PersisterFactory to ORM. #1260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 16 additions & 22 deletions lib/Doctrine/ORM/Cache/DefaultCache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,6 @@ class DefaultCache implements Cache
private $em;

/**
* @var \Doctrine\ORM\UnitOfWork
*/
private $uow;

/**
* @var \Doctrine\ORM\Cache\CacheFactory
*/
private $cacheFactory;
Expand All @@ -66,7 +61,6 @@ class DefaultCache implements Cache
public function __construct(EntityManagerInterface $em)
{
$this->em = $em;
$this->uow = $em->getUnitOfWork();
$this->cacheFactory = $em->getConfiguration()
->getSecondLevelCacheConfiguration()
->getCacheFactory();
Expand All @@ -78,7 +72,7 @@ public function __construct(EntityManagerInterface $em)
public function getEntityCacheRegion($className)
{
$metadata = $this->em->getClassMetadata($className);
$persister = $this->uow->getEntityPersister($metadata->rootEntityName);
$persister = $this->em->getPersisterFactory()->getOrCreateEntityPersister($metadata->rootEntityName);

if ( ! ($persister instanceof CachedPersister)) {
return null;
Expand All @@ -93,7 +87,7 @@ public function getEntityCacheRegion($className)
public function getCollectionCacheRegion($className, $association)
{
$metadata = $this->em->getClassMetadata($className);
$persister = $this->uow->getCollectionPersister($metadata->getAssociationMapping($association));
$persister = $this->em->getPersisterFactory()->getOrCreateCollectionPersister($metadata->getAssociationMapping($association));

if ( ! ($persister instanceof CachedPersister)) {
return null;
Expand All @@ -108,7 +102,7 @@ public function getCollectionCacheRegion($className, $association)
public function containsEntity($className, $identifier)
{
$metadata = $this->em->getClassMetadata($className);
$persister = $this->uow->getEntityPersister($metadata->rootEntityName);
$persister = $this->em->getPersisterFactory()->getOrCreateEntityPersister($metadata->rootEntityName);

if ( ! ($persister instanceof CachedPersister)) {
return false;
Expand All @@ -123,7 +117,7 @@ public function containsEntity($className, $identifier)
public function evictEntity($className, $identifier)
{
$metadata = $this->em->getClassMetadata($className);
$persister = $this->uow->getEntityPersister($metadata->rootEntityName);
$persister = $this->em->getPersisterFactory()->getOrCreateEntityPersister($metadata->rootEntityName);

if ( ! ($persister instanceof CachedPersister)) {
return;
Expand All @@ -138,7 +132,7 @@ public function evictEntity($className, $identifier)
public function evictEntityRegion($className)
{
$metadata = $this->em->getClassMetadata($className);
$persister = $this->uow->getEntityPersister($metadata->rootEntityName);
$persister = $this->em->getPersisterFactory()->getOrCreateEntityPersister($metadata->rootEntityName);

if ( ! ($persister instanceof CachedPersister)) {
return;
Expand All @@ -152,10 +146,11 @@ public function evictEntityRegion($className)
*/
public function evictEntityRegions()
{
$metadatas = $this->em->getMetadataFactory()->getAllMetadata();
$metadatas = $this->em->getMetadataFactory()->getAllMetadata();
$persisterFactory = $this->em->getPersisterFactory();

foreach ($metadatas as $metadata) {
$persister = $this->uow->getEntityPersister($metadata->rootEntityName);
$persister = $persisterFactory->getOrCreateEntityPersister($metadata->rootEntityName);

if ( ! ($persister instanceof CachedPersister)) {
continue;
Expand All @@ -171,7 +166,7 @@ public function evictEntityRegions()
public function containsCollection($className, $association, $ownerIdentifier)
{
$metadata = $this->em->getClassMetadata($className);
$persister = $this->uow->getCollectionPersister($metadata->getAssociationMapping($association));
$persister = $this->em->getPersisterFactory()->getOrCreateCollectionPersister($metadata->getAssociationMapping($association));

if ( ! ($persister instanceof CachedPersister)) {
return false;
Expand All @@ -186,7 +181,7 @@ public function containsCollection($className, $association, $ownerIdentifier)
public function evictCollection($className, $association, $ownerIdentifier)
{
$metadata = $this->em->getClassMetadata($className);
$persister = $this->uow->getCollectionPersister($metadata->getAssociationMapping($association));
$persister = $this->em->getPersisterFactory()->getOrCreateCollectionPersister($metadata->getAssociationMapping($association));

if ( ! ($persister instanceof CachedPersister)) {
return;
Expand All @@ -201,7 +196,7 @@ public function evictCollection($className, $association, $ownerIdentifier)
public function evictCollectionRegion($className, $association)
{
$metadata = $this->em->getClassMetadata($className);
$persister = $this->uow->getCollectionPersister($metadata->getAssociationMapping($association));
$persister = $this->em->getPersisterFactory()->getOrCreateCollectionPersister($metadata->getAssociationMapping($association));

if ( ! ($persister instanceof CachedPersister)) {
return;
Expand All @@ -215,17 +210,16 @@ public function evictCollectionRegion($className, $association)
*/
public function evictCollectionRegions()
{
$metadatas = $this->em->getMetadataFactory()->getAllMetadata();
$metadatas = $this->em->getMetadataFactory()->getAllMetadata();
$persisterFactory = $this->em->getPersisterFactory();

foreach ($metadatas as $metadata) {

foreach ($metadata->associationMappings as $association) {

if ( ! $association['type'] & ClassMetadata::TO_MANY) {
if ( ! ($association['type'] & ClassMetadata::TO_MANY)) {
continue;
}

$persister = $this->uow->getCollectionPersister($association);
$persister = $persisterFactory->getOrCreateCollectionPersister($association);

if ( ! ($persister instanceof CachedPersister)) {
continue;
Expand Down Expand Up @@ -329,7 +323,7 @@ private function buildCollectionCacheKey(ClassMetadata $metadata, $association,
private function toIdentifierArray(ClassMetadata $metadata, $identifier)
{
if (is_object($identifier) && $this->em->getMetadataFactory()->hasMetadataFor(ClassUtils::getClass($identifier))) {
$identifier = $this->uow->getSingleIdentifierValue($identifier);
$identifier = $this->em->getUnitOfWork()->getSingleIdentifierValue($identifier);

if ($identifier === null) {
throw ORMInvalidArgumentException::invalidIdentifierBindingEntity();
Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/ORM/Cache/DefaultCacheFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class DefaultCacheFactory implements CacheFactory
/**
* @var \Doctrine\Common\Cache\CacheProvider
*/
private $cache;
protected $cache;

/**
* @var \Doctrine\ORM\Cache\RegionsConfiguration
Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/ORM/Cache/DefaultCollectionHydrator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public function buildCacheEntry(ClassMetadata $metadata, CollectionCacheKey $key
public function loadCacheEntry(ClassMetadata $metadata, CollectionCacheKey $key, CollectionCacheEntry $entry, PersistentCollection $collection)
{
$assoc = $metadata->associationMappings[$key->association];
$targetPersister = $this->uow->getEntityPersister($assoc['targetEntity']);
$targetPersister = $this->em->getPersisterFactory()->getOrCreateEntityPersister($assoc['targetEntity']);
$targetRegion = $targetPersister->getCacheRegion();
$list = array();

Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/ORM/Cache/DefaultEntityHydrator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ public function loadCacheEntry(ClassMetadata $metadata, EntityCacheKey $key, Ent
}

$assocKey = new EntityCacheKey($assoc['targetEntity'], $assocId);
$assocPersister = $this->uow->getEntityPersister($assoc['targetEntity']);
$assocPersister = $this->em->getPersisterFactory()->getOrCreateEntityPersister($assoc['targetEntity']);
$assocRegion = $assocPersister->getCacheRegion();
$assocEntry = $assocRegion->get($assocKey);

Expand Down
19 changes: 10 additions & 9 deletions lib/Doctrine/ORM/Cache/DefaultQueryCache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
*/
class DefaultQueryCache implements QueryCache
{
/**
/**
* @var \Doctrine\ORM\EntityManagerInterface
*/
private $em;
Expand Down Expand Up @@ -107,7 +107,7 @@ public function get(QueryCacheKey $key, ResultSetMapping $rsm, array $hints = ar
$result = array();
$entityName = reset($rsm->aliasMap);
$hasRelation = ( ! empty($rsm->relationMap));
$persister = $this->uow->getEntityPersister($entityName);
$persister = $this->em->getPersisterFactory()->getOrCreateEntityPersister($entityName);
$region = $persister->getCacheRegion();
$regionName = $region->getName();

Expand Down Expand Up @@ -138,7 +138,7 @@ public function get(QueryCacheKey $key, ResultSetMapping $rsm, array $hints = ar

foreach ($entry['associations'] as $name => $assoc) {

$assocPersister = $this->uow->getEntityPersister($assoc['targetEntity']);
$assocPersister = $this->em->getPersisterFactory()->getOrCreateEntityPersister($assoc['targetEntity']);
$assocRegion = $assocPersister->getCacheRegion();

if ($assoc['type'] & ClassMetadata::TO_ONE) {
Expand Down Expand Up @@ -230,11 +230,12 @@ public function put(QueryCacheKey $key, ResultSetMapping $rsm, $result, array $h
return false;
}

$data = array();
$entityName = reset($rsm->aliasMap);
$hasRelation = ( ! empty($rsm->relationMap));
$metadata = $this->em->getClassMetadata($entityName);
$persister = $this->uow->getEntityPersister($entityName);
$data = array();
$entityName = reset($rsm->aliasMap);
$hasRelation = ( ! empty($rsm->relationMap));
$metadata = $this->em->getClassMetadata($entityName);
$persisterFactory = $this->em->getPersisterFactory();
$persister = $persisterFactory->getOrCreateEntityPersister($entityName);

if ( ! ($persister instanceof CachedPersister)) {
throw CacheException::nonCacheableEntity($entityName);
Expand Down Expand Up @@ -270,7 +271,7 @@ public function put(QueryCacheKey $key, ResultSetMapping $rsm, $result, array $h
throw CacheException::nonCacheableEntityAssociation($entityName, $name);
}

$assocPersister = $this->uow->getEntityPersister($assoc['targetEntity']);
$assocPersister = $persisterFactory->getOrCreateEntityPersister($assoc['targetEntity']);
$assocRegion = $assocPersister->getCacheRegion();
$assocMetadata = $assocPersister->getClassMetadata();

Expand Down
10 changes: 8 additions & 2 deletions lib/Doctrine/ORM/Cache/Persister/Collection/AbstractCollectionPersister.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@
*/
abstract class AbstractCollectionPersister implements CachedCollectionPersister
{
/**
/**
* @var \Doctrine\ORM\EntityManagerInterface
*/
private $em;

/**
* @var \Doctrine\ORM\UnitOfWork
*/
protected $uow;
Expand Down Expand Up @@ -106,6 +111,7 @@ public function __construct(CollectionPersister $persister, Region $region, Enti
$this->persister = $persister;
$this->association = $association;
$this->regionName = $region->getName();
$this->em = $em;
$this->uow = $em->getUnitOfWork();
$this->metadataFactory = $em->getMetadataFactory();
$this->cacheLogger = $cacheConfig->getCacheLogger();
Expand Down Expand Up @@ -162,7 +168,7 @@ public function loadCollectionCache(PersistentCollection $collection, Collection
*/
public function storeCollectionCache(CollectionCacheKey $key, $elements)
{
$targetPersister = $this->uow->getEntityPersister($this->targetEntity->rootEntityName);
$targetPersister = $this->em->getPersisterFactory()->getOrCreateEntityPersister($this->targetEntity->rootEntityName);
$targetRegion = $targetPersister->getCacheRegion();
$targetHydrator = $targetPersister->getEntityHydrator();
$entry = $this->hydrator->buildCacheEntry($this->targetEntity, $key, $elements);
Expand Down
14 changes: 10 additions & 4 deletions lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,12 @@
*/
abstract class AbstractEntityPersister implements CachedEntityPersister
{
/**
/**
* @var \Doctrine\ORM\EntityManagerInterface
*/
private $em;

/**
* @var \Doctrine\ORM\UnitOfWork
*/
protected $uow;
Expand Down Expand Up @@ -125,6 +130,7 @@ public function __construct(EntityPersister $persister, Region $region, EntityMa
$this->persister = $persister;
$this->cache = $em->getCache();
$this->regionName = $region->getName();
$this->em = $em;
$this->uow = $em->getUnitOfWork();
$this->metadataFactory = $em->getMetadataFactory();
$this->cacheLogger = $cacheConfig->getCacheLogger();
Expand Down Expand Up @@ -273,7 +279,7 @@ private function storeJoinedAssociations($entity)

$assocId = $this->uow->getEntityIdentifier($assocEntity);
$assocKey = new EntityCacheKey($assoc['targetEntity'], $assocId);
$assocPersister = $this->uow->getEntityPersister($assoc['targetEntity']);
$assocPersister = $this->em->getPersisterFactory()->getOrCreateEntityPersister($assoc['targetEntity']);

$assocPersister->storeEntityCache($assocEntity, $assocKey);
}
Expand Down Expand Up @@ -544,7 +550,7 @@ public function loadCriteria(Criteria $criteria)
*/
public function loadManyToManyCollection(array $assoc, $sourceEntity, PersistentCollection $coll)
{
$persister = $this->uow->getCollectionPersister($assoc);
$persister = $this->em->getPersisterFactory()->getOrCreateCollectionPersister($assoc);
$hasCache = ($persister instanceof CachedPersister);
$key = null;

Expand Down Expand Up @@ -580,7 +586,7 @@ public function loadManyToManyCollection(array $assoc, $sourceEntity, Persistent
*/
public function loadOneToManyCollection(array $assoc, $sourceEntity, PersistentCollection $coll)
{
$persister = $this->uow->getCollectionPersister($assoc);
$persister = $this->em->getPersisterFactory()->getOrCreateCollectionPersister($assoc);
$hasCache = ($persister instanceof CachedPersister);

if ($hasCache) {
Expand Down
8 changes: 8 additions & 0 deletions lib/Doctrine/ORM/Decorator/EntityManagerDecorator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,14 @@ public function getProxyFactory()
return $this->wrapped->getProxyFactory();
}

/**
* {@inheritdoc}
*/
public function getPersisterFactory()
{
return $this->wrapped->getPersisterFactory();
}

/**
* {@inheritdoc}
*/
Expand Down
24 changes: 21 additions & 3 deletions lib/Doctrine/ORM/EntityManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

namespace Doctrine\ORM;

use Doctrine\ORM\Persisters\PersisterFactory;
use Exception;
use Doctrine\Common\EventManager;
use Doctrine\DBAL\Connection;
Expand Down Expand Up @@ -82,6 +83,13 @@
*/
private $metadataFactory;

/**
* The persister factory, used to retrieve the ORM persister of collections and entity classes.
*
* @var \Doctrine\ORM\Persisters\PersisterFactory
*/
private $persisterFactory;

/**
* The UnitOfWork used to coordinate object-level transactions.
*
Expand Down Expand Up @@ -156,6 +164,7 @@ protected function __construct(Connection $conn, Configuration $config, EventMan
$this->metadataFactory->setEntityManager($this);
$this->metadataFactory->setCacheDriver($this->config->getMetadataCacheImpl());

$this->persisterFactory = new PersisterFactory($this);
Copy link
Member

Choose a reason for hiding this comment

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

would it be possible to avoid creating yet another circular object graph ? Given that we consider that extending the EntityManager is not supported and that we encourage using composition instead, we should try to make it actually work. Anytime we pass $this to another object, we are making it impossible to use composition (because we pass the inner object, not the decorator).

Note that it might be very hard to do this in Doctrine 2.x (meaning that we are actually lying when saying they should decorate the EntityManager) because almost every low layers of the ORM are actually depending on the high level EntityManager (which itself depends on the lower layers)

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a way to fix this, if we introduce 2 concepts:

  • EntityGraph which would drastically simplify UoW by being part of another class ChangeSet that tracks Entity states.
  • EntityBinding which would store a list of Metadata, Persister and have access to a given Entity's identifiers.

If I could traverse between EntityBindings to get Metadata of others, its Persisters inside of an existing Persister, then yes, we could circle back these dependency hell. But it would introduce some serious cost in performance. As I said in the beginning, I'm proposing to do the bare minimum to fully support many to many extra lazy operations.
The PersisterFactory is required because a subsequent PR will be responsible to generate SQL pieces, allowing to address the currently impossible bug #1033 . This SQL class would be instantiated inside of PersisterFactory and shared between Persisters. That's the only way I found to do it without entering on a Refactoring hell of UoW, Persisters and small bits of Hydrators.

Copy link
Member

Choose a reason for hiding this comment

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

EntityGraph which would drastically simplify UoW by being part of another class ChangeSet that tracks Entity states.

I'm already designing that stuff, but it's out of scope for this PR.

$this->repositoryFactory = $config->getRepositoryFactory();
$this->unitOfWork = new UnitOfWork($this);
$this->proxyFactory = new ProxyFactory(
Expand Down Expand Up @@ -190,6 +199,16 @@ public function getMetadataFactory()
return $this->metadataFactory;
}

/**
* Gets the persister factory used to manipulate collection and entity classes.
*
* @return \Doctrine\ORM\Persisters\PersisterFactory
*/
public function getPersisterFactory()
Copy link
Member

Choose a reason for hiding this comment

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

This method doesn't seem to fit the EntityManager at all: it would honestly be better to not expose it to end-users like this.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it seems the wrong way to expose it

Copy link
Member Author

Choose a reason for hiding this comment

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

Hibernate exposes it at EntityManager level because it can be swappable with another implementation.
Default implementation is also reachable from outside, allowing you to provide a ServiceInjector (read as DI Container).
I'm ok to move this to UnitOfWork, but I would not consider this as an extension point (the part I wanted to discuss internally). This means there'll be no interface bound to PersisterFactory.
Right now I made it part of EntityManager and wanted to discuss IF we'd make it an extension point.

Copy link
Member

Choose a reason for hiding this comment

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

TL;DR: the UnitOfWork as-is is already poop: let's stash poop in there instead of shoveling it on other classes.

That makes our EntityManager some kind of locator (meh).
I'd prefer to make a tradeoff here, and do this crappy thing in the UnitOfWork instead (yes, with all the deficiencies that come from it).

The UnitOfWork as we know it will go away at some point in time, while the EntityManager will stay and be consumed by userland.

{
return $this->persisterFactory;
}

/**
* {@inheritDoc}
*/
Expand Down Expand Up @@ -411,6 +430,7 @@ public function find($entityName, $id, $lockMode = null, $lockVersion = null)
throw ORMException::unrecognizedIdentifierFields($class->name, array_keys($id));
}

$persister = $this->persisterFactory->getOrCreateEntityPersister($class->name);
$unitOfWork = $this->getUnitOfWork();

// Check identity map first
Expand All @@ -427,16 +447,13 @@ public function find($entityName, $id, $lockMode = null, $lockVersion = null)
case LockMode::NONE === $lockMode:
case LockMode::PESSIMISTIC_READ === $lockMode:
case LockMode::PESSIMISTIC_WRITE === $lockMode:
$persister = $unitOfWork->getEntityPersister($class->name);
$persister->refresh($sortedId, $entity, $lockMode);
break;
}

return $entity; // Hit!
}

$persister = $unitOfWork->getEntityPersister($class->name);

switch (true) {
case LockMode::OPTIMISTIC === $lockMode:
if ( ! $class->isVersioned) {
Expand Down Expand Up @@ -540,6 +557,7 @@ public function getPartialReference($entityName, $identifier)
*/
public function clear($entityName = null)
{
$this->persisterFactory->clear();
$this->unitOfWork->clear($entityName);
}

Expand Down
Loading