Merge pull request #5924 from doctrine/fix/allow-empty-identifier-string-as-proxy-identifier

Allow empty string identifiers

lib/Doctrine/ORM/UnitOfWork.php

@@ -1398,12 +1398,13 @@ class UnitOfWork implements PropertyChangedListener
     public function addToIdentityMap($entity)
     {
         $classMetadata = $this->em->getClassMetadata(get_class($entity));
-        $idHash        = implode(' ', $this->entityIdentifiers[spl_object_hash($entity)]);
+        $identifier    = $this->entityIdentifiers[spl_object_hash($entity)];
 
-        if ($idHash === '') {
+        if (empty($identifier) || in_array(null, $identifier, true)) {
             throw ORMInvalidArgumentException::entityWithoutIdentity($classMetadata->name, $entity);
         }
 
+        $idHash    = implode(' ', $identifier);
         $className = $classMetadata->rootEntityName;
 
         if (isset($this->identityMap[$className][$idHash])) {

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

@@ -14,13 +14,8 @@ class Action
 
     /**
      * @Id
-     * @GeneratedValue
+     * @Column(type="string")
-     * @Column(type="integer")
+     * @GeneratedValue(strategy="NONE")
-     */
-    public $id;
-
-    /**
-     * @Column
      */
     public $name;
 

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

@@ -20,14 +20,14 @@ class ComplexAction
     /**
      * @Id
      * @OneToOne(targetEntity="Action", cascade={"persist", "remove"})
-     * @JoinColumn(name="action1_id", referencedColumnName="id")
+     * @JoinColumn(name="action1_name", referencedColumnName="name")
      */
     public $action1;
 
     /**
      * @Id
      * @OneToOne(targetEntity="Action", cascade={"persist", "remove"})
-     * @JoinColumn(name="action2_id", referencedColumnName="id")
+     * @JoinColumn(name="action2_name", referencedColumnName="name")
      */
     public $action2;
 

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

@@ -36,7 +36,7 @@ class Token
 
     /**
      * @ManyToOne(targetEntity="Action", cascade={"persist", "remove"}, inversedBy="tokens")
-     * @JoinColumn(name="action_id", referencedColumnName="id")
+     * @JoinColumn(name="action_name", referencedColumnName="name")
      * @var array
      */
     public $action;
@@ -44,8 +44,8 @@ class Token
     /**
      * @ManyToOne(targetEntity="ComplexAction", cascade={"persist", "remove"}, inversedBy="tokens")
      * @JoinColumns({
-     *   @JoinColumn(name="complex_action1_id", referencedColumnName="action1_id"),
+     *   @JoinColumn(name="complex_action1_name", referencedColumnName="action1_name"),
-     *   @JoinColumn(name="complex_action2_id", referencedColumnName="action2_id")
+     *   @JoinColumn(name="complex_action2_name", referencedColumnName="action2_name")
      * })
      * @var ComplexAction
      */

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

@@ -193,7 +193,7 @@ class SecondLevelCacheManyToOneTest extends SecondLevelCacheAbstractTest
         $this->_em->clear();
 
         $this->assertTrue($this->cache->containsEntity(Token::CLASSNAME, $token->token));
-        $this->assertFalse($this->cache->containsEntity(Token::CLASSNAME, $action->id));
+        $this->assertFalse($this->cache->containsEntity(Token::CLASSNAME, $action->name));
 
         $queryCount = $this->getCurrentQueryCount();
         $entity = $this->_em->find(Token::CLASSNAME, $token->token);
@@ -204,7 +204,7 @@ class SecondLevelCacheManyToOneTest extends SecondLevelCacheAbstractTest
         $this->assertInstanceOf(Action::CLASSNAME, $entity->getAction());
         $this->assertEquals('exec', $entity->getAction()->name);
 
-        $this->assertEquals($queryCount + 1, $this->getCurrentQueryCount());
+        $this->assertEquals($queryCount, $this->getCurrentQueryCount());
     }
 
     public function testPutAndLoadNonCacheableCompositeManyToOne()
@@ -231,9 +231,9 @@ class SecondLevelCacheManyToOneTest extends SecondLevelCacheAbstractTest
         $this->_em->clear();
 
         $this->assertTrue($this->cache->containsEntity(Token::CLASSNAME, $token->token));
-        $this->assertFalse($this->cache->containsEntity(Action::CLASSNAME, $action1->id));
+        $this->assertFalse($this->cache->containsEntity(Action::CLASSNAME, $action1->name));
-        $this->assertFalse($this->cache->containsEntity(Action::CLASSNAME, $action2->id));
+        $this->assertFalse($this->cache->containsEntity(Action::CLASSNAME, $action2->name));
-        $this->assertFalse($this->cache->containsEntity(Action::CLASSNAME, $action3->id));
+        $this->assertFalse($this->cache->containsEntity(Action::CLASSNAME, $action3->name));
 
         $queryCount = $this->getCurrentQueryCount();
         /**
@@ -255,8 +255,8 @@ class SecondLevelCacheManyToOneTest extends SecondLevelCacheAbstractTest
         $this->assertEquals($queryCount + 1, $this->getCurrentQueryCount());
 
         $this->assertEquals('login', $entity->getComplexAction()->getAction1()->name);
-        $this->assertEquals($queryCount + 2, $this->getCurrentQueryCount());
+        $this->assertEquals($queryCount + 1, $this->getCurrentQueryCount());
         $this->assertEquals('rememberme', $entity->getComplexAction()->getAction2()->name);
-        $this->assertEquals($queryCount + 3, $this->getCurrentQueryCount());
+        $this->assertEquals($queryCount + 1, $this->getCurrentQueryCount());
     }
 }

tests/Doctrine/Tests/ORM/UnitOfWorkTest.php

@@ -6,6 +6,7 @@ use Doctrine\Common\Collections\ArrayCollection;
 use Doctrine\Common\NotifyPropertyChanged;
 use Doctrine\Common\PropertyChangedListener;
 use Doctrine\ORM\Mapping\ClassMetadata;
+use Doctrine\ORM\ORMInvalidArgumentException;
 use Doctrine\ORM\UnitOfWork;
 use Doctrine\Tests\Mocks\ConnectionMock;
 use Doctrine\Tests\Mocks\DriverMock;
@@ -363,6 +364,101 @@ class UnitOfWorkTest extends OrmTestCase
             [new ArrayCollection()],
         ];
     }
+
+    /**
+     * @dataProvider entitiesWithValidIdentifiersProvider
+     *
+     * @param object $entity
+     * @param string $idHash
+     *
+     * @return void
+     */
+    public function testAddToIdentityMapValidIdentifiers($entity, $idHash)
+    {
+        $this->_unitOfWork->persist($entity);
+        $this->_unitOfWork->addToIdentityMap($entity);
+
+        self::assertSame($entity, $this->_unitOfWork->getByIdHash($idHash, get_class($entity)));
+    }
+
+    public function entitiesWithValidIdentifiersProvider()
+    {
+        $emptyString = new EntityWithStringIdentifier();
+
+        $emptyString->id = '';
+
+        $nonEmptyString = new EntityWithStringIdentifier();
+
+        $nonEmptyString->id = uniqid('id', true);
+
+        $emptyStrings = new EntityWithCompositeStringIdentifier();
+
+        $emptyStrings->id1 = '';
+        $emptyStrings->id2 = '';
+
+        $nonEmptyStrings = new EntityWithCompositeStringIdentifier();
+
+        $nonEmptyStrings->id1 = uniqid('id1', true);
+        $nonEmptyStrings->id2 = uniqid('id2', true);
+
+        $booleanTrue = new EntityWithBooleanIdentifier();
+
+        $booleanTrue->id = true;
+
+        $booleanFalse = new EntityWithBooleanIdentifier();
+
+        $booleanFalse->id = false;
+
+        return [
+            'empty string, single field'     => [$emptyString, ''],
+            'non-empty string, single field' => [$nonEmptyString, $nonEmptyString->id],
+            'empty strings, two fields'      => [$emptyStrings, ' '],
+            'non-empty strings, two fields'  => [$nonEmptyStrings, $nonEmptyStrings->id1 . ' ' . $nonEmptyStrings->id2],
+            'boolean true'                   => [$booleanTrue, '1'],
+            'boolean false'                  => [$booleanFalse, ''],
+        ];
+    }
+
+    public function testRegisteringAManagedInstanceRequiresANonEmptyIdentifier()
+    {
+        $this->expectException(ORMInvalidArgumentException::class);
+
+        $this->_unitOfWork->registerManaged(new EntityWithBooleanIdentifier(), [], []);
+    }
+
+    /**
+     * @dataProvider entitiesWithInvalidIdentifiersProvider
+     *
+     * @param object $entity
+     * @param array  $identifier
+     *
+     * @return void
+     */
+    public function testAddToIdentityMapInvalidIdentifiers($entity, array $identifier)
+    {
+        $this->expectException(ORMInvalidArgumentException::class);
+
+        $this->_unitOfWork->registerManaged($entity, $identifier, []);
+    }
+
+
+    public function entitiesWithInvalidIdentifiersProvider()
+    {
+        $firstNullString  = new EntityWithCompositeStringIdentifier();
+
+        $firstNullString->id2 = uniqid('id2', true);
+
+        $secondNullString = new EntityWithCompositeStringIdentifier();
+
+        $secondNullString->id1 = uniqid('id1', true);
+
+        return [
+            'null string, single field'      => [new EntityWithStringIdentifier(), ['id' => null]],
+            'null strings, two fields'       => [new EntityWithCompositeStringIdentifier(), ['id1' => null, 'id2' => null]],
+            'first null string, two fields'  => [$firstNullString, ['id1' => null, 'id2' => $firstNullString->id2]],
+            'second null string, two fields' => [$secondNullString, ['id1' => $secondNullString->id1, 'id2' => null]],
+        ];
+    }
 }
 
 /**
@@ -469,3 +565,43 @@ class VersionedAssignedIdentifierEntity
      */
     public $version;
 }
+
+/** @Entity */
+class EntityWithStringIdentifier
+{
+    /**
+     * @Id @Column(type="string")
+     *
+     * @var string|null
+     */
+    public $id;
+}
+
+/** @Entity */
+class EntityWithBooleanIdentifier
+{
+    /**
+     * @Id @Column(type="boolean")
+     *
+     * @var bool|null
+     */
+    public $id;
+}
+
+/** @Entity */
+class EntityWithCompositeStringIdentifier
+{
+    /**
+     * @Id @Column(type="string")
+     *
+     * @var string|null
+     */
+    public $id1;
+
+    /**
+     * @Id @Column(type="string")
+     *
+     * @var string|null
+     */
+    public $id2;
+}

tests/Doctrine/Tests/OrmFunctionalTestCase.php

@@ -460,8 +460,8 @@ abstract class OrmFunctionalTestCase extends OrmTestCase
             $conn->executeUpdate('DELETE FROM cache_state');
             $conn->executeUpdate('DELETE FROM cache_country');
             $conn->executeUpdate('DELETE FROM cache_login');
-            $conn->executeUpdate('DELETE FROM cache_complex_action');
             $conn->executeUpdate('DELETE FROM cache_token');
+            $conn->executeUpdate('DELETE FROM cache_complex_action');
             $conn->executeUpdate('DELETE FROM cache_action');
             $conn->executeUpdate('DELETE FROM cache_client');
         }