Throw OptimisticLockException when connection::commit() returns… (#7946)

* Throw OptimisticLockException when connection::commit() returns false

* Update unit tests

* Fix doctrine persistence version to avoid deprecations changes

* Apply changes from 2.8.x

* Update from 2.8.x

.scrutinizer.yml

@@ -3,7 +3,7 @@ build:
         analysis:
             environment:
                 php:
-                    version: 7.1
+                    version: 7.2
             cache:
                 disabled: false
                 directories:

.travis.yml

@@ -8,7 +8,6 @@ stages:
     - Code Quality
 
 php:
-  - 7.1
   - 7.2
   - 7.3
   - 7.4
@@ -62,16 +61,6 @@ jobs:
       addons:
         mariadb: 10.1
 
-    - stage: Test
-      dist: xenial
-      env: DB=mysql MYSQL_VERSION=5.7
-      php: 7.1
-      services:
-        - mysql
-      before_script:
-        - ./tests/travis/install-mysql-$MYSQL_VERSION.sh
-      sudo: required
-
     - stage: Test
       dist: xenial
       env: DB=mysql MYSQL_VERSION=5.7

UPGRADE.md

@@ -1,3 +1,10 @@
+# Upgrade to 2.8
+
+## Minor BC BREAK: Failed commit now throw OptimisticLockException
+
+Method `Doctrine\ORM\UnitOfWork#commit()` can throw an OptimisticLockException when a commit silently fails and returns false
+since `Doctrine\DBAL\Connection#commit()` signature changed from returning void to boolean
+
 # Upgrade to 2.7
 
 ## Added `Doctrine\ORM\AbstractQuery#enableResultCache()` and `Doctrine\ORM\AbstractQuery#disableResultCache()` methods	
@@ -30,7 +37,7 @@ Method `Doctrine\ORM\AbstractQuery#useResultCache()` is deprecated because it is
 and `disableResultCache()`. It will be removed in 3.0.
 
 ## Deprecated code generators and related console commands
- 
+
 These console commands have been deprecated:
 
  * `orm:convert-mapping`

composer.json

@@ -16,16 +16,16 @@
         "sort-packages": true
     },
     "require": {
-        "php": "^7.1",
+        "php": "^7.2",
         "ext-pdo": "*",
         "doctrine/annotations": "^1.8",
         "doctrine/cache": "^1.9.1",
         "doctrine/collections": "^1.5",
         "doctrine/common": "^2.11",
-        "doctrine/dbal": "^2.9.3",
+        "doctrine/dbal": "^2.10.0",
         "doctrine/event-manager": "^1.1",
         "doctrine/instantiator": "^1.3",
-        "doctrine/persistence": "^1.2",
+        "doctrine/persistence": "~1.2.0",
         "ocramius/package-versions": "^1.2",
         "symfony/console": "^3.0|^4.0|^5.0"
     },

lib/Doctrine/ORM/Decorator/EntityManagerDecorator.php

@@ -19,9 +19,9 @@
 
 namespace Doctrine\ORM\Decorator;
 
+use Doctrine\Common\Persistence\ObjectManagerDecorator;
 use Doctrine\ORM\Query\ResultSetMapping;
 use Doctrine\ORM\EntityManagerInterface;
-use Doctrine\Common\Persistence\ObjectManagerDecorator;
 
 /**
  * Base class for EntityManager decorators

lib/Doctrine/ORM/UnitOfWork.php

@@ -47,6 +47,7 @@
 use Throwable;
 use UnexpectedValueException;
 use function get_class;
+use function is_object;
 
 /**
  * The UnitOfWork is responsible for tracking changes to objects during an
@@ -424,10 +425,18 @@ public function commit($entity = null)
                 }
             }
 
-            $conn->commit();
+            // Commit failed silently
+            if ($conn->commit() === false) {
+                $object = is_object($entity) ? $entity : null;
+
+                throw new OptimisticLockException('Commit failed', $object);
+            }
         } catch (Throwable $e) {
             $this->em->close();
-            $conn->rollBack();
+
+            if ($conn->isTransactionActive()) {
+                $conn->rollBack();
+            }
 
             $this->afterTransactionRolledBack();
 

tests/Doctrine/Tests/ORM/UnitOfWorkTest.php

@@ -9,6 +9,7 @@
 use Doctrine\Common\PropertyChangedListener;
 use Doctrine\ORM\Events;
 use Doctrine\ORM\Mapping\ClassMetadata;
+use Doctrine\ORM\OptimisticLockException;
 use Doctrine\ORM\ORMInvalidArgumentException;
 use Doctrine\ORM\UnitOfWork;
 use Doctrine\Tests\Mocks\ConnectionMock;
@@ -277,7 +278,7 @@ public function testNoUndefinedIndexNoticeOnScheduleForUpdateWithoutChanges()
 
         // Create a test user
         $user = new ForumUser();
-        $user->name = 'Jasper';
+        $user->username = 'Jasper';
         $this->_unitOfWork->persist($user);
         $this->_unitOfWork->commit();
 
@@ -789,6 +790,36 @@ public function testPreviousDetectedIllegalNewNonCascadedEntitiesAreCleanedUpOnS
         self::assertCount(1, $persister1->getInserts());
         self::assertCount(0, $persister2->getInserts());
     }
+
+    /**
+     * @group #7946 Throw OptimisticLockException when connection::commit() returns false.
+     */
+    public function testCommitThrowOptimisticLockExceptionWhenConnectionCommitReturnFalse() : void
+    {
+        // Set another connection mock that fail on commit
+        $this->_connectionMock = $this->getMockBuilder(ConnectionMock::class)
+            ->setConstructorArgs([[], new DriverMock()])
+            ->setMethods(['commit'])
+            ->getMock();
+        $this->_emMock         = EntityManagerMock::create($this->_connectionMock, null, $this->eventManager);
+        $this->_unitOfWork     = new UnitOfWorkMock($this->_emMock);
+        $this->_emMock->setUnitOfWork($this->_unitOfWork);
+
+        $this->_connectionMock->method('commit')->willReturn(false);
+
+        // Setup fake persister and id generator
+        $userPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata(ForumUser::class));
+        $userPersister->setMockIdGeneratorType(ClassMetadata::GENERATOR_TYPE_IDENTITY);
+        $this->_unitOfWork->setEntityPersister(ForumUser::class, $userPersister);
+
+        // Create a test user
+        $user           = new ForumUser();
+        $user->username = 'Jasper';
+        $this->_unitOfWork->persist($user);
+
+        $this->expectException(OptimisticLockException::class);
+        $this->_unitOfWork->commit();
+    }
 }
 
 /**