Merge pull request #7075 from BreiteSeite/fix/#7068-entitymanager-fin…

…d-with-pessimistic-lock-does-not-check-for-transaction Fix for #7068: `EntityManager::find()` with pessimistic lock should check for transaction
doctrine · Jul 3, 2018 · 035d9ed · 035d9ed
2 parents 984641d + 0cbc87a
commit 035d9ed
Show file tree

Hide file tree

Showing 2 changed files with 72 additions and 10 deletions.
diff --git a/lib/Doctrine/ORM/EntityManager.php b/lib/Doctrine/ORM/EntityManager.php
@@ -16,6 +16,7 @@
 use Doctrine\ORM\Exception\MissingIdentifierField;
 use Doctrine\ORM\Exception\MissingMappingDriverImplementation;
 use Doctrine\ORM\Exception\UnrecognizedIdentifierFields;
+use Doctrine\ORM\Mapping\ClassMetadata;
 use Doctrine\ORM\Mapping\ClassMetadataFactory;
 use Doctrine\ORM\Proxy\Factory\ProxyFactory;
 use Doctrine\ORM\Proxy\Factory\StaticProxyFactory;
@@ -376,6 +377,10 @@ public function find($entityName, $id, $lockMode = null, $lockVersion = null)
         $class     = $this->metadataFactory->getMetadataFor(ltrim($entityName, '\\'));
         $className = $class->getClassName();
 
+        if ($lockMode !== null) {
+            $this->checkLockRequirements($lockMode, $class);
+        }
+
         if (! is_array($id)) {
             if ($class->isIdentifierComposite()) {
                 throw ORMInvalidArgumentException::invalidCompositeIdentifier();
@@ -438,24 +443,14 @@ public function find($entityName, $id, $lockMode = null, $lockVersion = null)
 
         switch (true) {
             case $lockMode === LockMode::OPTIMISTIC:
-                if (! $class->isVersioned()) {
-                    throw OptimisticLockException::notVersioned($className);
-                }
-
                 $entity = $persister->load($sortedId);
 
                 $unitOfWork->lock($entity, $lockMode, $lockVersion);
 
                 return $entity;
-
             case $lockMode === LockMode::PESSIMISTIC_READ:
             case $lockMode === LockMode::PESSIMISTIC_WRITE:
-                if (! $this->getConnection()->isTransactionActive()) {
-                    throw TransactionRequiredException::transactionRequired();
-                }
-
                 return $persister->load($sortedId, null, null, [], $lockMode);
-
             default:
                 return $persister->loadById($sortedId);
         }
@@ -897,4 +892,24 @@ public function hasFilters()
     {
         return $this->filterCollection !== null;
     }
+
+    /**
+     * @throws OptimisticLockException
+     * @throws TransactionRequiredException
+     */
+    private function checkLockRequirements(int $lockMode, ClassMetadata $class) : void
+    {
+        switch ($lockMode) {
+            case LockMode::OPTIMISTIC:
+                if (! $class->isVersioned()) {
+                    throw OptimisticLockException::notVersioned($class->getClassName());
+                }
+            // Intentional fallthrough
+            case LockMode::PESSIMISTIC_READ:
+            case LockMode::PESSIMISTIC_WRITE:
+                if (! $this->getConnection()->isTransactionActive()) {
+                    throw TransactionRequiredException::transactionRequired();
+                }
+        }
+    }
 }
diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7068Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7068Test.php
@@ -0,0 +1,47 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\ORM\Functional\Ticket;
+
+use Doctrine\DBAL\LockMode;
+use Doctrine\ORM\Annotation as ORM;
+use Doctrine\ORM\TransactionRequiredException;
+use Doctrine\Tests\OrmFunctionalTestCase;
+
+final class GH7068Test extends OrmFunctionalTestCase
+{
+    /**
+     * {@inheritDoc}
+     */
+    protected function setUp() : void
+    {
+        parent::setUp();
+
+        $this->setUpEntitySchema(
+            [
+                SomeEntity::class,
+            ]
+        );
+    }
+
+    public function testLockModeIsRespected() : void
+    {
+        $entity = new SomeEntity();
+        $this->em->persist($entity);
+        $this->em->flush();
+        $this->em->clear();
+
+        $this->em->find(SomeEntity::class, 1);
+
+        $this->expectException(TransactionRequiredException::class);
+        $this->em->find(SomeEntity::class, 1, LockMode::PESSIMISTIC_WRITE);
+    }
+}
+
+/** @ORM\Entity */
+final class SomeEntity
+{
+    /** @ORM\Id @ORM\Column(type="integer") @ORM\GeneratedValue */
+    public $id;
+}