Changed lock function to compare timestamps instead of DateTimeInterf…

…ace objects directly.

When using optimistic lock with DateTimeInterface based version field a
bug appears due to the use of the === operator for comparing the lock
version and the entity version. This comparison always resolves to false
because the === operator when comparing objects is only true when both
sides are the exact same instance of the object.

To fix the issue I have decided to compare timestamps instead the
DateTimeInterface based objects directly, calling getTimestamp() method
and doing a strict comparison.

Modified OptimisticLockException to use DateTimeInterface instead of
DateTime class.

Fixes doctrine#8499

lib/Doctrine/ORM/OptimisticLockException.php

@@ -20,7 +20,7 @@
 
 namespace Doctrine\ORM;
 
-use DateTime;
+use DateTimeInterface;
 
 /**
  * An OptimisticLockException is thrown when a version check on an object
@@ -70,8 +70,8 @@ public static function lockFailed($entity)
      */
     public static function lockFailedVersionMismatch($entity, $expectedLockVersion, $actualLockVersion)
     {
-        $expectedLockVersion = $expectedLockVersion instanceof DateTime ? $expectedLockVersion->getTimestamp() : $expectedLockVersion;
-        $actualLockVersion   = $actualLockVersion instanceof DateTime ? $actualLockVersion->getTimestamp() : $actualLockVersion;
+        $expectedLockVersion = $expectedLockVersion instanceof DateTimeInterface ? $expectedLockVersion->getTimestamp() : $expectedLockVersion;
+        $actualLockVersion   = $actualLockVersion instanceof DateTimeInterface ? $actualLockVersion->getTimestamp() : $actualLockVersion;
 
         return new self('The optimistic lock failed, version ' . $expectedLockVersion . ' was expected, but is actually ' . $actualLockVersion, $entity);
     }

lib/Doctrine/ORM/UnitOfWork.php

@@ -20,6 +20,7 @@
 
 namespace Doctrine\ORM;
 
+use DateTimeInterface;
 use Doctrine\Common\Collections\ArrayCollection;
 use Doctrine\Common\Collections\Collection;
 use Doctrine\Common\EventManager;
@@ -2494,7 +2495,11 @@ public function lock($entity, $lockMode, $lockVersion = null)
 
                 $entityVersion = $class->reflFields[$class->versionField]->getValue($entity);
 
-                if ($entityVersion !== $lockVersion) {
+                if ($entityVersion instanceof DateTimeInterface && $lockVersion instanceof DateTimeInterface) {
+                    if ($entityVersion->getTimestamp() !== $lockVersion->getTimestamp()) {
+                        throw OptimisticLockException::lockFailedVersionMismatch($entity, $lockVersion, $entityVersion);
+                    }
+                } elseif ($entityVersion !== $lockVersion) {
                     throw OptimisticLockException::lockFailedVersionMismatch($entity, $lockVersion, $entityVersion);
                 }
 

tests/Doctrine/Tests/ORM/Functional/Ticket/GH8499Test.php

@@ -0,0 +1,176 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\ORM\Functional\Ticket;
+
+use DateTime;
+use DateTimeInterface;
+use Doctrine\DBAL\LockMode;
+use Doctrine\DBAL\Schema\SchemaException;
+use Doctrine\ORM\Mapping\Column;
+use Doctrine\ORM\Mapping\Entity;
+use Doctrine\ORM\Mapping\GeneratedValue;
+use Doctrine\ORM\Mapping\Id;
+use Doctrine\ORM\Mapping\Table;
+use Doctrine\ORM\Mapping\Version;
+use Doctrine\ORM\OptimisticLockException;
+use Doctrine\Tests\OrmFunctionalTestCase;
+
+use function sleep;
+
+class GH8499Test extends OrmFunctionalTestCase
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        try {
+            $this->setUpEntitySchema(
+                [GH8499VersionableEntity::class]
+            );
+        } catch (SchemaException $e) {
+        }
+    }
+
+    /**
+     * @group GH-8499
+     */
+    public function testOptimisticLockInitializesVersionOnEntityInsert(): void
+    {
+        $entity = new GH8499VersionableEntity();
+        $entity->setName('Test Entity');
+        $entity->setDescription('Entity to test optimistic lock fix with DateTimeInterface objects');
+        $this->_em->persist($entity);
+        $this->_em->flush();
+        $this->_em->refresh($entity);
+
+        $this->assertInstanceOf(DateTimeInterface::class, $entity->getRevision(), 'Version field not set to DateTimeInterface');
+    }
+
+    /**
+     * @group GH-8499
+     */
+    public function testOptimisticLockUpdatesVersionOnEntityUpdate(): void
+    {
+        $entity = new GH8499VersionableEntity();
+        $entity->setName('Test Entity');
+        $entity->setDescription('Entity to test optimistic lock fix with DateTimeInterface objects');
+        $this->_em->persist($entity);
+        $this->_em->flush();
+        $this->_em->refresh($entity);
+        $firstVersion = $entity->getRevision();
+        sleep(1);
+
+        $entity->setName('Test Entity Updated');
+        $this->_em->persist($entity);
+        $this->_em->flush();
+        $this->_em->refresh($entity);
+        $lastVersion = $entity->getRevision();
+
+        $this->assertNotEquals($firstVersion, $lastVersion, 'Version field value not updated on persist');
+    }
+
+    /**
+     * @group GH-8499
+     */
+    public function testOptimisticLockWithDateTimeForVersion(): void
+    {
+        $entity = new GH8499VersionableEntity();
+        $entity->setName('Test Entity');
+        $entity->setDescription('Entity to test optimistic lock fix with DateTimeInterface objects');
+        $this->_em->persist($entity);
+        $this->_em->flush();
+
+        $firstVersion = $entity->getRevision();
+
+        $this->_em->lock($entity, LockMode::OPTIMISTIC, $firstVersion);
+        sleep(1);
+
+        $entity->setName('Test Entity Locked');
+        $this->_em->persist($entity);
+        $this->_em->flush();
+
+        $this->assertNotEquals($firstVersion, $entity->getRevision(), 'Version field value not updated on persist');
+    }
+
+    /**
+     * @group GH-8499
+     */
+    public function testOptimisticLockWithDateTimeForVersionThrowsException(): void
+    {
+        $this->expectException(OptimisticLockException::class);
+
+        $entity = new GH8499VersionableEntity();
+        $entity->setName('Test Entity');
+        $entity->setDescription('Entity to test optimistic lock fix with DateTimeInterface objects');
+        $this->_em->persist($entity);
+        $this->_em->flush();
+
+        $this->_em->lock($entity, LockMode::OPTIMISTIC, new DateTime('2020-07-15 18:04:00'));
+    }
+}
+
+/**
+ * @Entity
+ * @Table
+ */
+class GH8499VersionableEntity
+{
+    /**
+     * @Id
+     * @Column(type="integer")
+     * @GeneratedValue
+     * @var int
+     */
+    public $id;
+
+    /**
+     * @Column(type="string")
+     * @var string
+     */
+    public $name;
+
+    /**
+     * @Column(type="string")
+     * @var string
+     */
+    public $description;
+
+    /**
+     * @Version
+     * @Column(type="datetime")
+     * @var DateTimeInterface
+     */
+    public $revision;
+
+    public function getId(): int
+    {
+        return $this->id;
+    }
+
+    public function getName(): string
+    {
+        return $this->name;
+    }
+
+    public function setName(string $name): void
+    {
+        $this->name = $name;
+    }
+
+    public function getDescription(): string
+    {
+        return $this->description;
+    }
+
+    public function setDescription(string $description): void
+    {
+        $this->description = $description;
+    }
+
+    public function getRevision(): DateTimeInterface
+    {
+        return $this->revision;
+    }
+}