Merge pull request #6159 from nicolas-cajelli/backport-fix-relation-cache-#1551-to-2.5

#5821 Backport #1551 - Fixed support for inverse side second level cache

lib/Doctrine/ORM/Cache/Persister/Collection/AbstractCollectionPersister.php

@@ -32,6 +32,7 @@ use Doctrine\Common\Util\ClassUtils;
 
 /**
  * @author Fabio B. Silva <fabio.bat.silva@gmail.com>
+ * @author Guilherme Blanco <guilhermeblanco@hotmail.com>
  * @since 2.5
  */
 abstract class AbstractCollectionPersister implements CachedCollectionPersister
@@ -164,10 +165,18 @@ abstract class AbstractCollectionPersister implements CachedCollectionPersister
     public function storeCollectionCache(CollectionCacheKey $key, $elements)
     {
         /* @var $targetPersister CachedEntityPersister */
+        $associationMapping = $this->sourceEntity->associationMappings[$key->association];
         $targetPersister    = $this->uow->getEntityPersister($this->targetEntity->rootEntityName);
         $targetRegion       = $targetPersister->getCacheRegion();
         $targetHydrator     = $targetPersister->getEntityHydrator();
-        $entry              = $this->hydrator->buildCacheEntry($this->targetEntity, $key, $elements);
+
+        // Only preserve ordering if association configured it
+        if ( ! (isset($associationMapping['indexBy']) && $associationMapping['indexBy'])) {
+            // Elements may be an array or a Collection
+            $elements = array_values(is_array($elements) ? $elements : $elements->getValues());
+        }
+
+        $entry = $this->hydrator->buildCacheEntry($this->targetEntity, $key, $elements);
 
         foreach ($entry->identifiers as $index => $entityKey) {
             if ($targetRegion->contains($entityKey)) {

lib/Doctrine/ORM/Cache/Persister/Entity/NonStrictReadWriteCachedEntityPersister.php

@@ -27,6 +27,7 @@ use Doctrine\ORM\Cache\EntityCacheKey;
  * Specific non-strict read/write cached entity persister
  *
  * @author Fabio B. Silva <fabio.bat.silva@gmail.com>
+ * @author Guilherme Blanco <guilhermeblanco@hotmail.com>
  * @since  2.5
  */
 class NonStrictReadWriteCachedEntityPersister extends AbstractEntityPersister
@@ -78,13 +79,16 @@ class NonStrictReadWriteCachedEntityPersister extends AbstractEntityPersister
      */
     public function delete($entity)
     {
-        $key = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity));
+        $key     = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity));
+        $deleted = $this->persister->delete($entity);
 
-        if ($this->persister->delete($entity)) {
+        if ($deleted) {
             $this->region->evict($key);
         }
 
         $this->queuedCache['delete'][] = $key;
+
+        return $deleted;
     }
 
     /**

lib/Doctrine/ORM/Cache/Persister/Entity/ReadWriteCachedEntityPersister.php

@@ -30,6 +30,7 @@ use Doctrine\ORM\Cache\EntityCacheKey;
  * Specific read-write entity persister
  *
  * @author Fabio B. Silva <fabio.bat.silva@gmail.com>
+ * @author Guilherme Blanco <guilhermeblanco@hotmail.com>
  * @since 2.5
  */
 class ReadWriteCachedEntityPersister extends AbstractEntityPersister
@@ -100,21 +101,24 @@ class ReadWriteCachedEntityPersister extends AbstractEntityPersister
      */
     public function delete($entity)
     {
-        $key   = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity));
+        $key     = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity));
-        $lock  = $this->region->lock($key);
+        $lock    = $this->region->lock($key);
+        $deleted = $this->persister->delete($entity);
 
-        if ($this->persister->delete($entity)) {
+        if ($deleted) {
             $this->region->evict($key);
         }
 
         if ($lock === null) {
-            return;
+            return $deleted;
         }
 
         $this->queuedCache['delete'][] = array(
             'lock'   => $lock,
             'key'    => $key
         );
+
+        return $deleted;
     }
 
     /**

lib/Doctrine/ORM/Persisters/Collection/AbstractCollectionPersister.php

@@ -90,10 +90,6 @@ abstract class AbstractCollectionPersister implements CollectionPersister
 
         // If Entity is scheduled for inclusion, it is not in this collection.
         // We can assure that because it would have return true before on array check
-        if ($entityState === UnitOfWork::STATE_MANAGED && $this->uow->isScheduledForInsert($entity)) {
+        return ! ($entityState === UnitOfWork::STATE_MANAGED && $this->uow->isScheduledForInsert($entity));
-            return false;
-        }
-
-        return true;
     }
 }

lib/Doctrine/ORM/UnitOfWork.php

@@ -733,7 +733,6 @@ class UnitOfWork implements PropertyChangedListener
 
         // Look for changes in associations of the entity
         foreach ($class->associationMappings as $field => $assoc) {
-
             if (($val = $class->reflFields[$field]->getValue($entity)) === null) {
                 continue;
             }
@@ -799,7 +798,7 @@ class UnitOfWork implements PropertyChangedListener
                 // Only MANAGED entities that are NOT SCHEDULED FOR INSERTION OR DELETION are processed here.
                 $oid = spl_object_hash($entity);
 
-                if ( ! isset($this->entityInsertions[$oid]) &&  ! isset($this->entityDeletions[$oid]) && isset($this->entityStates[$oid])) {
+                if ( ! isset($this->entityInsertions[$oid]) && ! isset($this->entityDeletions[$oid]) && isset($this->entityStates[$oid])) {
                     $this->computeChangeSet($class, $entity);
                 }
             }
@@ -826,10 +825,7 @@ class UnitOfWork implements PropertyChangedListener
         if ($value instanceof PersistentCollection && $value->isDirty()) {
             $coid = spl_object_hash($value);
 
-            if ($assoc['isOwningSide']) {
+            $this->collectionUpdates[$coid] = $value;
-                $this->collectionUpdates[$coid] = $value;
-            }
-
             $this->visitedCollections[$coid] = $value;
         }
 

tests/Doctrine/Tests/Models/Cache/State.php

@@ -33,7 +33,7 @@ class State
     protected $country;
 
     /**
-     * @Cache
+     * @Cache("NONSTRICT_READ_WRITE")
      * @OneToMany(targetEntity="City", mappedBy="state")
      */
     protected $cities;

tests/Doctrine/Tests/Models/Cache/Traveler.php

@@ -26,7 +26,7 @@ class Traveler
     protected $name;
 
     /**
-     * @Cache()
+     * @Cache("NONSTRICT_READ_WRITE")
      * @OneToMany(targetEntity="Travel", mappedBy="traveler", cascade={"persist", "remove"}, orphanRemoval=true)
      *
      * @var \Doctrine\Common\Collections\Collection

tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheManyToOneTest.php

@@ -2,10 +2,10 @@
 
 namespace Doctrine\Tests\ORM\Functional;
 
+use Doctrine\Tests\Models\Cache\City;
 use Doctrine\Tests\Models\Cache\ComplexAction;
 use Doctrine\Tests\Models\Cache\Country;
 use Doctrine\Tests\Models\Cache\State;
-use Doctrine\ORM\Mapping\ClassMetadata;
 use Doctrine\Tests\Models\Cache\Token;
 use Doctrine\Tests\Models\Cache\Action;
 
@@ -98,6 +98,40 @@ class SecondLevelCacheManyToOneTest extends SecondLevelCacheAbstractTest
         $this->assertEquals($this->states[1]->getCountry()->getName(), $c4->getCountry()->getName());
     }
 
+    public function testInverseSidePutShouldEvictCollection()
+    {
+        $this->loadFixturesCountries();
+        $this->loadFixturesStates();
+
+        $this->_em->clear();
+
+        $this->cache->evictEntityRegion(State::CLASSNAME);
+        $this->cache->evictEntityRegion(Country::CLASSNAME);
+
+        //evict collection on add
+        $c3    = $this->_em->find(State::CLASSNAME, $this->states[0]->getId());
+        $prev  = $c3->getCities();
+        $count = $prev->count();
+        $city  = new City("Buenos Aires", $c3);
+
+        $c3->addCity($city);
+
+        $this->_em->persist($city);
+        $this->_em->persist($c3);
+        $this->_em->flush();
+        $this->_em->clear();
+
+        $state      = $this->_em->find(State::CLASSNAME, $c3->getId());
+        $queryCount = $this->getCurrentQueryCount();
+
+        // Association was cleared from EM
+        $this->assertNotEquals($prev, $state->getCities());
+
+        // New association has one more item (cache was evicted)
+        $this->assertEquals($count + 1, $state->getCities()->count());
+        $this->assertEquals($queryCount, $this->getCurrentQueryCount());
+    }
+
     public function testShouldNotReloadWhenAssociationIsMissing()
     {
         $this->loadFixturesCountries();

tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheOneToManyTest.php

@@ -14,18 +14,18 @@ use Doctrine\Tests\Models\Cache\Traveler;
  */
 class SecondLevelCacheOneToManyTest extends SecondLevelCacheAbstractTest
 {
-    public function testShouldNotPutCollectionInverseSideOnPersist()
+    public function testShouldPutCollectionInverseSideOnPersist()
     {
         $this->loadFixturesCountries();
         $this->loadFixturesStates();
         $this->loadFixturesCities();
+
         $this->_em->clear();
 
         $this->assertTrue($this->cache->containsEntity(State::CLASSNAME, $this->states[0]->getId()));
         $this->assertTrue($this->cache->containsEntity(State::CLASSNAME, $this->states[1]->getId()));
-
+        $this->assertTrue($this->cache->containsCollection(State::CLASSNAME, 'cities', $this->states[0]->getId()));
-        $this->assertFalse($this->cache->containsCollection(State::CLASSNAME, 'cities', $this->states[0]->getId()));
+        $this->assertTrue($this->cache->containsCollection(State::CLASSNAME, 'cities', $this->states[1]->getId()));
-        $this->assertFalse($this->cache->containsCollection(State::CLASSNAME, 'cities', $this->states[1]->getId()));
     }
 
     public function testPutAndLoadOneToManyRelation()
@@ -187,6 +187,7 @@ class SecondLevelCacheOneToManyTest extends SecondLevelCacheAbstractTest
         $this->loadFixturesCountries();
         $this->loadFixturesStates();
         $this->loadFixturesCities();
+
         $this->_em->clear();
         $this->secondLevelCacheLogger->clearStats();
 
@@ -247,8 +248,8 @@ class SecondLevelCacheOneToManyTest extends SecondLevelCacheAbstractTest
         $this->_em->remove($city0);
         $this->_em->persist($state);
         $this->_em->flush();
-
         $this->_em->clear();
+
         $this->secondLevelCacheLogger->clearStats();
 
         $queryCount = $this->getCurrentQueryCount();
@@ -261,19 +262,19 @@ class SecondLevelCacheOneToManyTest extends SecondLevelCacheAbstractTest
         $this->assertInstanceOf(City::CLASSNAME, $city1);
         $this->assertEquals($entity->getCities()->get(1)->getName(), $city1->getName());
 
-        $this->assertEquals(1, $this->secondLevelCacheLogger->getHitCount());
+        $this->assertEquals(2, $this->secondLevelCacheLogger->getHitCount());
         $this->assertEquals(1, $this->secondLevelCacheLogger->getRegionHitCount($this->getEntityRegion(State::CLASSNAME)));
-        $this->assertEquals(0, $this->secondLevelCacheLogger->getRegionHitCount($this->getCollectionRegion(State::CLASSNAME, 'cities')));
+        $this->assertEquals(1, $this->secondLevelCacheLogger->getRegionHitCount($this->getCollectionRegion(State::CLASSNAME, 'cities')));
 
-        $this->assertEquals($queryCount + 1, $this->getCurrentQueryCount());
+        $this->assertEquals($queryCount, $this->getCurrentQueryCount());
 
         $state->getCities()->remove(0);
 
         $this->_em->remove($city1);
         $this->_em->persist($state);
         $this->_em->flush();
-
         $this->_em->clear();
+
         $this->secondLevelCacheLogger->clearStats();
 
         $queryCount = $this->getCurrentQueryCount();
@@ -281,9 +282,9 @@ class SecondLevelCacheOneToManyTest extends SecondLevelCacheAbstractTest
 
         $this->assertCount(0, $state->getCities());
 
-        $this->assertEquals(1, $this->secondLevelCacheLogger->getHitCount());
+        $this->assertEquals(2, $this->secondLevelCacheLogger->getHitCount());
         $this->assertEquals(1, $this->secondLevelCacheLogger->getRegionHitCount($this->getEntityRegion(State::CLASSNAME)));
-        $this->assertEquals(0, $this->secondLevelCacheLogger->getRegionHitCount($this->getCollectionRegion(State::CLASSNAME, 'cities')));
+        $this->assertEquals(1, $this->secondLevelCacheLogger->getRegionHitCount($this->getCollectionRegion(State::CLASSNAME, 'cities')));
     }
 
     public function testOneToManyWithEmptyRelation()
@@ -346,11 +347,12 @@ class SecondLevelCacheOneToManyTest extends SecondLevelCacheAbstractTest
     public function testCacheInitializeCollectionWithNewObjects()
     {
         $this->_em->clear();
+
         $this->evictRegions();
 
         $traveler = new Traveler("Doctrine Bot");
 
-        for ($i=0; $i<3; ++$i) {
+        for ($i = 0; $i < 3; ++$i) {
             $traveler->getTravels()->add(new Travel($traveler));
         }
 
@@ -373,7 +375,7 @@ class SecondLevelCacheOneToManyTest extends SecondLevelCacheAbstractTest
         $this->assertFalse($entity->getTravels()->isInitialized());
         $this->assertCount(4, $entity->getTravels());
         $this->assertTrue($entity->getTravels()->isInitialized());
-        $this->assertEquals($queryCount + 1, $this->getCurrentQueryCount());
+        $this->assertEquals($queryCount, $this->getCurrentQueryCount());
 
         $this->_em->flush();
         $this->_em->clear();