From 768c291cd10a6e4e36a702b3429e288a3a807038 Mon Sep 17 00:00:00 2001 From: Darien Hager <dhager@gmi-mr.com> Date: Fri, 10 Apr 2015 11:15:30 -0700 Subject: [PATCH 1/8] Stumbled across a bug where signatures didn't match, but also the current persister-type didn't support getCacheRegion(). Unsure of exact mechanism, but clearly the constructor doesn't take the second argument anyway, may be old code. --- lib/Doctrine/ORM/Cache/DefaultCacheFactory.php | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/lib/Doctrine/ORM/Cache/DefaultCacheFactory.php b/lib/Doctrine/ORM/Cache/DefaultCacheFactory.php index 69d2fd1c1..82843d359 100644 --- a/lib/Doctrine/ORM/Cache/DefaultCacheFactory.php +++ b/lib/Doctrine/ORM/Cache/DefaultCacheFactory.php @@ -180,13 +180,7 @@ class DefaultCacheFactory implements CacheFactory */ public function buildCollectionHydrator(EntityManagerInterface $em, array $mapping) { - /* @var $targetPersister \Doctrine\ORM\Cache\Persister\CachedPersister */ - $targetPersister = $em->getUnitOfWork()->getEntityPersister($mapping['targetEntity']); - - return new DefaultCollectionHydrator( - $em, - $targetPersister->getCacheRegion() - ); + return new DefaultCollectionHydrator($em); } /** From d29cc3660fbbbeffe567755bfea1f416b83ec6f5 Mon Sep 17 00:00:00 2001 From: Darien Hager <dhager@gmi-mr.com> Date: Fri, 10 Apr 2015 16:02:38 -0700 Subject: [PATCH 2/8] Change the test listener than layers on second-level-caching so that it is more conservative, only turning on caching-associations when it knows the target entity is cache-able. --- .../EventListener/CacheMetadataListener.php | 49 +++++++++++++++++-- 1 file changed, 45 insertions(+), 4 deletions(-) diff --git a/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php b/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php index f9e2616c6..daacef07e 100644 --- a/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php +++ b/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php @@ -3,33 +3,74 @@ namespace Doctrine\Tests\EventListener; use Doctrine\Common\Persistence\Event\LoadClassMetadataEventArgs; +use Doctrine\Common\Persistence\ObjectManager; +use Doctrine\ORM\EntityManager; use Doctrine\ORM\Mapping\ClassMetadata; class CacheMetadataListener { + + /** + * Tracks which entities we have already forced caching enabled on. This is + * important to avoid some potential infinite-recursion issues. + * @var array + */ + protected $enabledItems = array(); + /** * @param \Doctrine\Common\Persistence\Event\LoadClassMetadataEventArgs $event */ public function loadClassMetadata(LoadClassMetadataEventArgs $event) { $metadata = $event->getClassMetadata(); - $cache = array( - 'usage' => ClassMetadata::CACHE_USAGE_NONSTRICT_READ_WRITE - ); + $em = $event->getObjectManager(); /** @var $metadata \Doctrine\ORM\Mapping\ClassMetadata */ if (strstr($metadata->name, 'Doctrine\Tests\Models\Cache')) { return; } + if( ! $em instanceof EntityManager){ + return; + } + + $this->enableCaching($metadata, $em); + } + + /** + * @param ClassMetadata $metadata + * @param EntityManager $em + */ + protected function enableCaching(ClassMetadata $metadata, EntityManager $em){ + + if(array_key_exists($metadata->getName(), $this->enabledItems)){ + return; // Already handled in the past + } + + $cache = array( + 'usage' => ClassMetadata::CACHE_USAGE_NONSTRICT_READ_WRITE + ); + if ($metadata->isVersioned) { return; } $metadata->enableCache($cache); + $this->enabledItems[$metadata->getName()] = $metadata; + + /* + * Only enable association-caching when the target has already been + * given caching settings + */ foreach ($metadata->associationMappings as $mapping) { - $metadata->enableAssociationCache($mapping['fieldName'], $cache); + + $targetMeta = $em->getClassMetadata($mapping['targetEntity']); + $this->enableCaching($targetMeta, $em); + + if(array_key_exists($targetMeta->getName(), $this->enabledItems)){ + $metadata->enableAssociationCache($mapping['fieldName'], $cache); + } } } } From 08be905fc38ced42cb7b3de20ec33dd256b346ef Mon Sep 17 00:00:00 2001 From: Darien Hager <dhager@gmi-mr.com> Date: Tue, 5 May 2015 13:52:08 -0700 Subject: [PATCH 3/8] Refactor LoadClassMetadataEventArgs to ensure it contains an EntityManager --- .../ORM/Event/LoadClassMetadataEventArgs.php | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php b/lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php index 5bebf7541..1a43e0890 100644 --- a/lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php +++ b/lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php @@ -20,6 +20,8 @@ namespace Doctrine\ORM\Event; use Doctrine\Common\Persistence\Event\LoadClassMetadataEventArgs as BaseLoadClassMetadataEventArgs; +use Doctrine\ORM\Mapping\ClassMetadata; +use Doctrine\ORM\EntityManager; /** * Class that holds event arguments for a loadMetadata event. @@ -29,6 +31,22 @@ use Doctrine\Common\Persistence\Event\LoadClassMetadataEventArgs as BaseLoadClas */ class LoadClassMetadataEventArgs extends BaseLoadClassMetadataEventArgs { + /** + * @param ClassMetadata $classMetadata + * @param EntityManager $entityManager + */ + function __construct(ClassMetadata $classMetadata, EntityManager $entityManager) + { + /* + We use our own constructor here to enforce type-hinting requirements, + since both inputs are specialized subclasses compared to what the super- + class is willing to accept. + + In particular, we want to have EntityManager rather than ObjectManager. + */ + parent::__construct($classMetadata, $entityManager); + } + /** * Retrieve associated EntityManager. * @@ -36,6 +54,18 @@ class LoadClassMetadataEventArgs extends BaseLoadClassMetadataEventArgs */ public function getEntityManager() { - return $this->getObjectManager(); + $em = $this->getObjectManager(); + assert($em instanceof EntityManager); + return $em; + } + + /** + * Retrieves the associated ClassMetadata. + * + * @return \Doctrine\ORM\Mapping\ClassMetadata + */ + public function getClassMetadata() + { + return parent::getClassMetadata(); } } From c507b52f20d0418f3957bdc2bc808f1875c5ca0b Mon Sep 17 00:00:00 2001 From: Darien Hager <dhager@gmi-mr.com> Date: Tue, 5 May 2015 13:56:22 -0700 Subject: [PATCH 4/8] Remove now-superfluous EntityManager check --- tests/Doctrine/Tests/EventListener/CacheMetadataListener.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php b/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php index daacef07e..dfb870d4b 100644 --- a/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php +++ b/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php @@ -30,10 +30,6 @@ class CacheMetadataListener return; } - if( ! $em instanceof EntityManager){ - return; - } - $this->enableCaching($metadata, $em); } From d5adda954d4a7514c9b088d71b20ce4cfc30a8d1 Mon Sep 17 00:00:00 2001 From: Darien Hager <dhager@gmi-mr.com> Date: Tue, 5 May 2015 13:57:52 -0700 Subject: [PATCH 5/8] Whitespace formatting tweaks --- .../Tests/EventListener/CacheMetadataListener.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php b/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php index dfb870d4b..fd0a922f8 100644 --- a/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php +++ b/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php @@ -37,13 +37,13 @@ class CacheMetadataListener * @param ClassMetadata $metadata * @param EntityManager $em */ - protected function enableCaching(ClassMetadata $metadata, EntityManager $em){ + protected function enableCaching(ClassMetadata $metadata, EntityManager $em) { - if(array_key_exists($metadata->getName(), $this->enabledItems)){ + if (array_key_exists($metadata->getName(), $this->enabledItems)) { return; // Already handled in the past } - $cache = array( + $cache = array( 'usage' => ClassMetadata::CACHE_USAGE_NONSTRICT_READ_WRITE ); @@ -64,7 +64,7 @@ class CacheMetadataListener $targetMeta = $em->getClassMetadata($mapping['targetEntity']); $this->enableCaching($targetMeta, $em); - if(array_key_exists($targetMeta->getName(), $this->enabledItems)){ + if (array_key_exists($targetMeta->getName(), $this->enabledItems)) { $metadata->enableAssociationCache($mapping['fieldName'], $cache); } } From 97e90ddefcd144899dea2baa2771c79895ebff45 Mon Sep 17 00:00:00 2001 From: Darien Hager <dhager@gmi-mr.com> Date: Tue, 5 May 2015 15:28:23 -0700 Subject: [PATCH 6/8] Clarify state-changes, replace array_key_exists() with isset() for speed --- .../EventListener/CacheMetadataListener.php | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php b/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php index fd0a922f8..82a4f0db6 100644 --- a/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php +++ b/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php @@ -13,6 +13,9 @@ class CacheMetadataListener /** * Tracks which entities we have already forced caching enabled on. This is * important to avoid some potential infinite-recursion issues. + * + * Key is the name of the entity, payload is unimportant. + * * @var array */ protected $enabledItems = array(); @@ -33,13 +36,28 @@ class CacheMetadataListener $this->enableCaching($metadata, $em); } + /** + * @param ClassMetadata $metadata + * @return bool + */ + private function isVisited(ClassMetaData $metadata) { + return isset($this->enabledItems[$metadata->getName()]); + } + + /** + * @param ClassMetadata $metadata + */ + private function recordVisit(ClassMetaData $metadata) { + $this->enabledItems[$metadata->getName()] = true; + } + /** * @param ClassMetadata $metadata * @param EntityManager $em */ protected function enableCaching(ClassMetadata $metadata, EntityManager $em) { - if (array_key_exists($metadata->getName(), $this->enabledItems)) { + if ($this->isVisited($metadata)) { return; // Already handled in the past } @@ -53,7 +71,7 @@ class CacheMetadataListener $metadata->enableCache($cache); - $this->enabledItems[$metadata->getName()] = $metadata; + $this->recordVisit($metadata); /* * Only enable association-caching when the target has already been @@ -64,7 +82,7 @@ class CacheMetadataListener $targetMeta = $em->getClassMetadata($mapping['targetEntity']); $this->enableCaching($targetMeta, $em); - if (array_key_exists($targetMeta->getName(), $this->enabledItems)) { + if ($this->isVisited($targetMeta)) { $metadata->enableAssociationCache($mapping['fieldName'], $cache); } } From fff56c7f3f19b2d25e3654b37ea121c8ecea8582 Mon Sep 17 00:00:00 2001 From: Darien Hager <dhager@gmi-mr.com> Date: Wed, 8 Jul 2015 15:52:09 -0700 Subject: [PATCH 7/8] Remove runtime assertion --- lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php b/lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php index 1a43e0890..c9d4c1fe6 100644 --- a/lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php +++ b/lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php @@ -54,9 +54,11 @@ class LoadClassMetadataEventArgs extends BaseLoadClassMetadataEventArgs */ public function getEntityManager() { - $em = $this->getObjectManager(); - assert($em instanceof EntityManager); - return $em; + /* + We can safely assume our ObjectManager is also an EventManager due to + our restrictions in the constructor. + */ + return $this->getObjectManager(); } /** From ed1c4de2b62ca7eeabf7e5fb36f86ef6d17a8b38 Mon Sep 17 00:00:00 2001 From: Marco Pivetta <ocramius@gmail.com> Date: Wed, 15 Jul 2015 20:35:21 +0100 Subject: [PATCH 8/8] DDC-3683 - #1380 - reverting BC break, annotating correct types, cs fixes --- .../ORM/Event/LoadClassMetadataEventArgs.php | 37 +++---------------- .../EventListener/CacheMetadataListener.php | 19 +++++----- 2 files changed, 14 insertions(+), 42 deletions(-) diff --git a/lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php b/lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php index c9d4c1fe6..95e75616e 100644 --- a/lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php +++ b/lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php @@ -20,33 +20,20 @@ namespace Doctrine\ORM\Event; use Doctrine\Common\Persistence\Event\LoadClassMetadataEventArgs as BaseLoadClassMetadataEventArgs; -use Doctrine\ORM\Mapping\ClassMetadata; -use Doctrine\ORM\EntityManager; /** * Class that holds event arguments for a loadMetadata event. * * @author Jonathan H. Wage <jonwage@gmail.com> * @since 2.0 + * + * Note: method annotations are used instead of method overrides (due to BC policy) + * + * @method __construct(\Doctrine\ORM\Mapping\ClassMetadata $classMetadata, \Doctrine\ORM\EntityManager $objectManager) + * @method \Doctrine\ORM\EntityManager getClassMetadata() */ class LoadClassMetadataEventArgs extends BaseLoadClassMetadataEventArgs { - /** - * @param ClassMetadata $classMetadata - * @param EntityManager $entityManager - */ - function __construct(ClassMetadata $classMetadata, EntityManager $entityManager) - { - /* - We use our own constructor here to enforce type-hinting requirements, - since both inputs are specialized subclasses compared to what the super- - class is willing to accept. - - In particular, we want to have EntityManager rather than ObjectManager. - */ - parent::__construct($classMetadata, $entityManager); - } - /** * Retrieve associated EntityManager. * @@ -54,20 +41,6 @@ class LoadClassMetadataEventArgs extends BaseLoadClassMetadataEventArgs */ public function getEntityManager() { - /* - We can safely assume our ObjectManager is also an EventManager due to - our restrictions in the constructor. - */ return $this->getObjectManager(); } - - /** - * Retrieves the associated ClassMetadata. - * - * @return \Doctrine\ORM\Mapping\ClassMetadata - */ - public function getClassMetadata() - { - return parent::getClassMetadata(); - } } diff --git a/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php b/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php index 82a4f0db6..7a7caf18e 100644 --- a/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php +++ b/tests/Doctrine/Tests/EventListener/CacheMetadataListener.php @@ -3,7 +3,6 @@ namespace Doctrine\Tests\EventListener; use Doctrine\Common\Persistence\Event\LoadClassMetadataEventArgs; -use Doctrine\Common\Persistence\ObjectManager; use Doctrine\ORM\EntityManager; use Doctrine\ORM\Mapping\ClassMetadata; @@ -38,16 +37,19 @@ class CacheMetadataListener /** * @param ClassMetadata $metadata + * * @return bool */ - private function isVisited(ClassMetaData $metadata) { + private function isVisited(ClassMetaData $metadata) + { return isset($this->enabledItems[$metadata->getName()]); } /** * @param ClassMetadata $metadata */ - private function recordVisit(ClassMetaData $metadata) { + private function recordVisit(ClassMetaData $metadata) + { $this->enabledItems[$metadata->getName()] = true; } @@ -55,8 +57,8 @@ class CacheMetadataListener * @param ClassMetadata $metadata * @param EntityManager $em */ - protected function enableCaching(ClassMetadata $metadata, EntityManager $em) { - + protected function enableCaching(ClassMetadata $metadata, EntityManager $em) + { if ($this->isVisited($metadata)) { return; // Already handled in the past } @@ -73,12 +75,9 @@ class CacheMetadataListener $this->recordVisit($metadata); - /* - * Only enable association-caching when the target has already been - * given caching settings - */ + // only enable association-caching when the target has already been + // given caching settings foreach ($metadata->associationMappings as $mapping) { - $targetMeta = $em->getClassMetadata($mapping['targetEntity']); $this->enableCaching($targetMeta, $em);