[DDC-2430] Prevent Criteria queries using the ID of an assocation on …

…PersistentCollections, as the in-memory ArrayCollection does not work with this kind of query. Attention this is a BC-BREAK, that is necessary to fix potentially very hard to debug bugs. Therefore it is not merged back into 2.3

UPGRADE.md

@@ -28,6 +28,20 @@ Now parenthesis are considered, the previous DQL will generate:
 
     SELECT 100 / (2 * 2) FROM my_entity
 
+## Compatibility Bugfix in PersistentCollection#matching() breaks BC
+
+In Doctrine 2.3 it was possible to use the new ``matching($criteria)``
+functionality by adding constraints for assocations based on ID:
+
+    Criteria::expr()->eq('association', $assocation->getId());
+
+This functionality does not work on InMemory collections however, because
+in memory criteria compares object values based on reference.
+As of 2.4 the above code will throw an exception. You need to change
+offending code to pass the ``$assocation`` reference directly:
+
+    Criteria::expr()->eq('association', $assocation);
+
 # Upgrade to 2.3
 
 ## EntityManager#find() not calls EntityRepository#find() anymore

lib/Doctrine/ORM/PersistentCollection.php

@@ -850,12 +850,8 @@ public function matching(Criteria $criteria)
             throw new \RuntimeException("Matching Criteria on PersistentCollection only works on OneToMany associations at the moment.");
         }
 
-        $id              = $this->em
-            ->getClassMetadata(get_class($this->owner))
-            ->getSingleIdReflectionProperty()
-            ->getValue($this->owner);
         $builder         = Criteria::expr();
-        $ownerExpression = $builder->eq($this->backRefFieldName, $id);
+        $ownerExpression = $builder->eq($this->backRefFieldName, $this->owner);
         $expression      = $criteria->getWhereExpression();
         $expression      = $expression ? $builder->andX($expression, $ownerExpression) : $ownerExpression;
 

lib/Doctrine/ORM/Persisters/BasicEntityPersister.php

@@ -1583,7 +1583,7 @@ protected function getSelectConditionCriteriaSQL(Criteria $criteria)
             return '';
         }
 
-        $visitor = new SqlExpressionVisitor($this);
+        $visitor = new SqlExpressionVisitor($this, $this->class);
 
         return $visitor->dispatch($expression);
     }

lib/Doctrine/ORM/Persisters/PersisterException.php

@@ -0,0 +1,20 @@
+<?php
+
+namespace Doctrine\ORM\Persisters;
+
+use Doctrine\ORM\ORMException;
+
+class PersisterException extends ORMException
+{
+    /**
+     * @return PersisterException
+     */
+    static public function matchingAssocationFieldRequiresObject($class, $associationName)
+    {
+        return new self(sprintf(
+            "Cannot match on %s::%s with a non-object value. Matching objects by id is " .
+            "not compatible with matching on an in-memory collection, which compares objects by reference.",
+            $class, $associationName
+        ));
+    }
+}

lib/Doctrine/ORM/Persisters/SqlExpressionVisitor.php

@@ -19,6 +19,8 @@
 
 namespace Doctrine\ORM\Persisters;
 
+use Doctrine\ORM\Mapping\ClassMetadata;
+
 use Doctrine\Common\Collections\Expr\ExpressionVisitor;
 use Doctrine\Common\Collections\Expr\Comparison;
 use Doctrine\Common\Collections\Expr\Value;
@@ -37,12 +39,18 @@ class SqlExpressionVisitor extends ExpressionVisitor
      */
     private $persister;
 
+    /**
+     * @var \Doctrine\ORM\Mapping\ClassMetadata
+     */
+    private $classMetadata;
+
     /**
      * @param \Doctrine\ORM\Persisters\BasicEntityPersister $persister
      */
-    public function __construct(BasicEntityPersister $persister)
+    public function __construct(BasicEntityPersister $persister, ClassMetadata $classMetadata)
     {
         $this->persister = $persister;
+        $this->classMetadata = $classMetadata;
     }
 
     /**
@@ -57,6 +65,10 @@ public function walkComparison(Comparison $comparison)
         $field = $comparison->getField();
         $value = $comparison->getValue()->getValue(); // shortcut for walkValue()
 
+        if (isset($this->classMetadata->associationMappings[$field]) && ! is_object($value)) {
+            throw PersisterException::matchingAssocationFieldRequiresObject($this->classMetadata->name, $field);
+        }
+
         return $this->persister->getSelectConditionStatementSQL($field, $value, null, $comparison->getOperator());
     }
 

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

@@ -7,8 +7,7 @@
 use Doctrine\Tests\Models\CMS\CmsAddress;
 use Doctrine\Tests\Models\CMS\CmsPhonenumber;
 use Doctrine\Common\Collections\Criteria;
-
-require_once __DIR__ . '/../../TestInit.php';
+use Doctrine\Common\Collections\ArrayCollection;
 
 /**
  * @author robo
@@ -781,6 +780,29 @@ public function testMatchingCriteriaGteComparison()
         $this->assertEquals(4, count($users));
     }
 
+    /**
+     * @group DDC-2430
+     */
+    public function testMatchingCriteriaAssocationByObjectInMemory()
+    {
+        list($userId, $addressId) = $this->loadAssociatedFixture();
+
+        $user = $this->_em->find('Doctrine\Tests\Models\CMS\CmsUser', $userId);
+
+        $criteria = new Criteria(
+            Criteria::expr()->gte('user', $user)
+        );
+
+        $repository = $this->_em->getRepository('Doctrine\Tests\Models\CMS\CmsAddress');
+        $addresses = $repository->matching($criteria);
+
+        $this->assertEquals(1, count($addresses));
+
+        $addresses = new ArrayCollection($repository->findAll());
+
+        $this->assertEquals(1, count($addresses->matching($criteria)));
+    }
+
     public function testMatchingCriteriaContainsComparison()
     {
         $this->loadFixture();

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

@@ -343,17 +343,32 @@ public function testInheritanceMatching()
 
         $repository = $this->_em->getRepository("Doctrine\Tests\Models\Company\CompanyContract");
         $contracts = $repository->matching(new Criteria(
-            Criteria::expr()->eq('salesPerson', $this->salesPerson->getId())
+            Criteria::expr()->eq('salesPerson', $this->salesPerson)
         ));
         $this->assertEquals(3, count($contracts));
 
         $repository = $this->_em->getRepository("Doctrine\Tests\Models\Company\CompanyFixContract");
         $contracts = $repository->matching(new Criteria(
-            Criteria::expr()->eq('salesPerson', $this->salesPerson->getId())
+            Criteria::expr()->eq('salesPerson', $this->salesPerson)
         ));
         $this->assertEquals(1, count($contracts));
     }
 
+    /**
+     * @group DDC-2430
+     */
+    public function testMatchingNonObjectOnAssocationThrowsException()
+    {
+        $this->loadFullFixture();
+
+        $repository = $this->_em->getRepository("Doctrine\Tests\Models\Company\CompanyContract");
+
+        $this->setExpectedException('Doctrine\ORM\Persisters\PersisterException', 'annot match on Doctrine\Tests\Models\Company\CompanyContract::salesPerson with a non-object value.');
+        $contracts = $repository->matching(new Criteria(
+            Criteria::expr()->eq('salesPerson', $this->salesPerson->getId())
+        ));
+    }
+
     /**
      * @group DDC-834
      */