Merge pull request #11646 from greg0ire/finally-fix-bug

Run risky code in finally block

.github/workflows/coding-standards.yml

@@ -24,4 +24,4 @@ on:
 
 jobs:
   coding-standards:
-    uses: "doctrine/.github/.github/workflows/coding-standards.yml@5.0.1"
+    uses: "doctrine/.github/.github/workflows/coding-standards.yml@5.1.0"

.github/workflows/documentation.yml

@@ -5,45 +5,16 @@ on:
     branches:
       - "*.x"
     paths:
-      - .github/workflows/documentation.yml
-      - docs/**
+      - ".github/workflows/documentation.yml"
+      - "docs/**"
   push:
     branches:
       - "*.x"
     paths:
-      - .github/workflows/documentation.yml
-      - docs/**
+      - ".github/workflows/documentation.yml"
+      - "docs/**"
 
 jobs:
-  validate-with-guides:
-    name: "Validate documentation with phpDocumentor/guides"
-    runs-on: "ubuntu-22.04"
-
-    steps:
-      - name: "Checkout code"
-        uses: "actions/checkout@v4"
-
-      - name: "Install PHP"
-        uses: "shivammathur/setup-php@v2"
-        with:
-          coverage: "none"
-          php-version: "8.3"
-
-      - name: "Remove existing composer file"
-        run: "rm composer.json"
-
-      - name: "Require phpdocumentor/guides-cli"
-        run: "composer require --dev phpdocumentor/guides-cli --no-update"
-
-      - name: "Install dependencies with Composer"
-        uses: "ramsey/composer-install@v3"
-        with:
-          dependency-versions: "highest"
-
-      - name: "Add orphan metadata where needed"
-        run: |
-          printf '%s\n\n%s\n' ":orphan:" "$(cat docs/en/sidebar.rst)" > docs/en/sidebar.rst
-          printf '%s\n\n%s\n' ":orphan:" "$(cat docs/en/reference/installation.rst)" > docs/en/reference/installation.rst
-
-      - name: "Run guides-cli"
-        run: "vendor/bin/guides -vvv --no-progress docs/en 2>&1 | grep -v 'No template found for rendering directive' | ( ! grep WARNING )"
+  documentation:
+    name: "Documentation"
+    uses: "doctrine/.github/.github/workflows/documentation.yml@5.1.0"

.github/workflows/release-on-milestone-closed.yml

@@ -7,7 +7,7 @@ on:
 
 jobs:
   release:
-    uses: "doctrine/.github/.github/workflows/release-on-milestone-closed.yml@5.0.1"
+    uses: "doctrine/.github/.github/workflows/release-on-milestone-closed.yml@5.1.0"
     secrets:
       GIT_AUTHOR_EMAIL: ${{ secrets.GIT_AUTHOR_EMAIL }}
       GIT_AUTHOR_NAME: ${{ secrets.GIT_AUTHOR_NAME }}

composer.json

@@ -42,7 +42,7 @@
         "doctrine/annotations": "^1.13 || ^2",
         "doctrine/coding-standard": "^9.0.2 || ^12.0",
         "phpbench/phpbench": "^0.16.10 || ^1.0",
-        "phpstan/phpstan": "~1.4.10 || 1.11.1",
+        "phpstan/phpstan": "~1.4.10 || 1.12.6",
         "phpunit/phpunit": "^7.5 || ^8.5 || ^9.6",
         "psr/log": "^1 || ^2 || ^3",
         "squizlabs/php_codesniffer": "3.7.2",

docs/en/reference/attributes-reference.rst

@@ -14,7 +14,7 @@ Index
 -  :ref:`#[AttributeOverride] <attrref_attributeoverride>`
 -  :ref:`#[Column] <attrref_column>`
 -  :ref:`#[Cache] <attrref_cache>`
--  :ref:`#[ChangeTrackingPolicy <attrref_changetrackingpolicy>`
+-  :ref:`#[ChangeTrackingPolicy] <attrref_changetrackingpolicy>`
 -  :ref:`#[CustomIdGenerator] <attrref_customidgenerator>`
 -  :ref:`#[DiscriminatorColumn] <attrref_discriminatorcolumn>`
 -  :ref:`#[DiscriminatorMap] <attrref_discriminatormap>`

docs/en/reference/installation.rst

@@ -1,3 +1,5 @@
+:orphan:
+
 Installation
 ============
 

docs/en/sidebar.rst

@@ -1,84 +1,77 @@
-.. toc::
+:orphan:
 
-   .. tocheader:: Tutorials
+.. toctree::
+   :caption: Tutorials
+   :depth: 3
 
-   .. toctree::
-      :depth: 3
+   tutorials/getting-started
+   tutorials/getting-started-database
+   tutorials/getting-started-models
+   tutorials/working-with-indexed-associations
+   tutorials/extra-lazy-associations
+   tutorials/composite-primary-keys
+   tutorials/ordered-associations
+   tutorials/override-field-association-mappings-in-subclasses
+   tutorials/pagination
+   tutorials/embeddables
 
-      tutorials/getting-started
-      tutorials/getting-started-database
-      tutorials/getting-started-models
-      tutorials/working-with-indexed-associations
-      tutorials/extra-lazy-associations
-      tutorials/composite-primary-keys
-      tutorials/ordered-associations
-      tutorials/override-field-association-mappings-in-subclasses
-      tutorials/pagination
-      tutorials/embeddables
+.. toctree::
+   :caption: Reference
+   :depth: 3
 
-.. toc::
+   reference/architecture
+   reference/configuration
+   reference/faq
+   reference/basic-mapping
+   reference/association-mapping
+   reference/inheritance-mapping
+   reference/working-with-objects
+   reference/working-with-associations
+   reference/typedfieldmapper
+   reference/events
+   reference/unitofwork
+   reference/unitofwork-associations
+   reference/transactions-and-concurrency
+   reference/batch-processing
+   reference/dql-doctrine-query-language
+   reference/query-builder
+   reference/native-sql
+   reference/change-tracking-policies
+   reference/partial-objects
+   reference/annotations-reference
+   reference/attributes-reference
+   reference/xml-mapping
+   reference/yaml-mapping
+   reference/php-mapping
+   reference/caching
+   reference/improving-performance
+   reference/tools
+   reference/metadata-drivers
+   reference/best-practices
+   reference/limitations-and-known-issues
+   tutorials/pagination
+   reference/filters
+   reference/namingstrategy
+   reference/advanced-configuration
+   reference/second-level-cache
+   reference/security
 
-   .. tocheader:: Reference
+.. toctree::
+   :caption: Cookbook
+   :depth: 3
 
-   .. toctree::
-      :depth: 3
-
-      reference/architecture
-      reference/configuration
-      reference/faq
-      reference/basic-mapping
-      reference/association-mapping
-      reference/inheritance-mapping
-      reference/working-with-objects
-      reference/working-with-associations
-      reference/typedfieldmapper
-      reference/events
-      reference/unitofwork
-      reference/unitofwork-associations
-      reference/transactions-and-concurrency
-      reference/batch-processing
-      reference/dql-doctrine-query-language
-      reference/query-builder
-      reference/native-sql
-      reference/change-tracking-policies
-      reference/partial-objects
-      reference/annotations-reference
-      reference/attributes-reference
-      reference/xml-mapping
-      reference/yaml-mapping
-      reference/php-mapping
-      reference/caching
-      reference/improving-performance
-      reference/tools
-      reference/metadata-drivers
-      reference/best-practices
-      reference/limitations-and-known-issues
-      tutorials/pagination
-      reference/filters
-      reference/namingstrategy
-      reference/advanced-configuration
-      reference/second-level-cache
-      reference/security
-
-.. toc::
-
-   .. tocheader:: Cookbook
-
-   .. toctree::
-      :depth: 3
-
-      cookbook/aggregate-fields
-      cookbook/custom-mapping-types
-      cookbook/decorator-pattern
-      cookbook/dql-custom-walkers
-      cookbook/dql-user-defined-functions
-      cookbook/implementing-arrayaccess-for-domain-objects
-      cookbook/implementing-the-notify-changetracking-policy
-      cookbook/resolve-target-entity-listener
-      cookbook/sql-table-prefixes
-      cookbook/strategy-cookbook-introduction
-      cookbook/validation-of-entities
-      cookbook/working-with-datetime
-      cookbook/mysql-enums
-      cookbook/advanced-field-value-conversion-using-custom-mapping-types
-      cookbook/entities-in-session
+   cookbook/aggregate-fields
+   cookbook/custom-mapping-types
+   cookbook/decorator-pattern
+   cookbook/dql-custom-walkers
+   cookbook/dql-user-defined-functions
+   cookbook/implementing-arrayaccess-for-domain-objects
+   cookbook/implementing-the-notify-changetracking-policy
+   cookbook/resolve-target-entity-listener
+   cookbook/sql-table-prefixes
+   cookbook/strategy-cookbook-introduction
+   cookbook/validation-of-entities
+   cookbook/working-with-datetime
+   cookbook/mysql-enums
+   cookbook/advanced-field-value-conversion-using-custom-mapping-types
+   cookbook/entities-in-session

src/EntityManager.php

@@ -32,7 +32,6 @@ use Doctrine\ORM\Repository\RepositoryFactory;
 use Doctrine\Persistence\Mapping\MappingException;
 use Doctrine\Persistence\ObjectRepository;
 use InvalidArgumentException;
-use Throwable;
 
 use function array_keys;
 use function class_exists;
@@ -246,18 +245,24 @@ class EntityManager implements EntityManagerInterface
 
         $this->conn->beginTransaction();
 
+        $successful = false;
+
         try {
             $return = $func($this);
 
             $this->flush();
             $this->conn->commit();
 
-            return $return ?: true;
-        } catch (Throwable $e) {
-            $this->close();
-            $this->conn->rollBack();
+            $successful = true;
 
-            throw $e;
+            return $return ?: true;
+        } finally {
+            if (! $successful) {
+                $this->close();
+                if ($this->conn->isTransactionActive()) {
+                    $this->conn->rollBack();
+                }
+            }
         }
     }
 
@@ -268,18 +273,24 @@ class EntityManager implements EntityManagerInterface
     {
         $this->conn->beginTransaction();
 
+        $successful = false;
+
         try {
             $return = $func($this);
 
             $this->flush();
             $this->conn->commit();
 
-            return $return;
-        } catch (Throwable $e) {
-            $this->close();
-            $this->conn->rollBack();
+            $successful = true;
 
-            throw $e;
+            return $return;
+        } finally {
+            if (! $successful) {
+                $this->close();
+                if ($this->conn->isTransactionActive()) {
+                    $this->conn->rollBack();
+                }
+            }
         }
     }
 

src/Query/Exec/SingleTableDeleteUpdateExecutor.php

@@ -14,8 +14,6 @@ use Doctrine\ORM\Query\SqlWalker;
  * that are mapped to a single table.
  *
  * @link        www.doctrine-project.org
- *
- * @todo This is exactly the same as SingleSelectExecutor. Unify in SingleStatementExecutor.
  */
 class SingleTableDeleteUpdateExecutor extends AbstractSqlExecutor
 {

src/Tools/EntityGenerator.php

@@ -767,6 +767,9 @@ public function __construct(<params>)
 
             if ($fieldMapping['type'] === 'datetime') {
                 $param = $this->getType($fieldMapping['type']) . ' ' . $param;
+                if (! empty($fieldMapping['nullable'])) {
+                    $param = '?' . $param;
+                }
             }
 
             if (! empty($fieldMapping['nullable'])) {
@@ -1385,6 +1388,9 @@ public function __construct(<params>)
         if ($typeHint && ! isset($types[$typeHint])) {
             $variableType   =  '\\' . ltrim($variableType, '\\');
             $methodTypeHint =  '\\' . $typeHint . ' ';
+            if ($defaultValue === 'null') {
+                $methodTypeHint = '?' . $methodTypeHint;
+            }
         }
 
         $replacements = [

src/UnitOfWork.php

@@ -49,7 +49,6 @@ use Doctrine\Persistence\PropertyChangedListener;
 use Exception;
 use InvalidArgumentException;
 use RuntimeException;
-use Throwable;
 use UnexpectedValueException;
 
 use function array_chunk;
@@ -427,6 +426,8 @@ class UnitOfWork implements PropertyChangedListener
         $conn = $this->em->getConnection();
         $conn->beginTransaction();
 
+        $successful = false;
+
         try {
             // Collection deletions (deletions of complete collections)
             foreach ($this->collectionDeletions as $collectionToDelete) {
@@ -478,16 +479,18 @@ class UnitOfWork implements PropertyChangedListener
 
                 throw new OptimisticLockException('Commit failed', $object);
             }
-        } catch (Throwable $e) {
-            $this->em->close();
 
-            if ($conn->isTransactionActive()) {
-                $conn->rollBack();
+            $successful = true;
+        } finally {
+            if (! $successful) {
+                $this->em->close();
+
+                if ($conn->isTransactionActive()) {
+                    $conn->rollBack();
+                }
+
+                $this->afterTransactionRolledBack();
             }
-
-            $this->afterTransactionRolledBack();
-
-            throw $e;
         }
 
         $this->afterTransactionComplete();

tests/Tests/ORM/EntityManagerTest.php

@@ -21,16 +21,21 @@ use Doctrine\ORM\QueryBuilder;
 use Doctrine\ORM\UnitOfWork;
 use Doctrine\Persistence\Mapping\Driver\MappingDriver;
 use Doctrine\Persistence\Mapping\MappingException;
+use Doctrine\Tests\Mocks\ConnectionMock;
+use Doctrine\Tests\Mocks\EntityManagerMock;
 use Doctrine\Tests\Models\CMS\CmsUser;
 use Doctrine\Tests\Models\GeoNames\Country;
 use Doctrine\Tests\OrmTestCase;
+use Exception;
 use Generator;
 use InvalidArgumentException;
+use PHPUnit\Framework\Assert;
 use stdClass;
 use TypeError;
 
 use function get_class;
 use function random_int;
+use function sprintf;
 use function sys_get_temp_dir;
 use function uniqid;
 
@@ -382,4 +387,61 @@ class EntityManagerTest extends OrmTestCase
 
         $this->entityManager->flush($entity);
     }
+
+    /** @dataProvider entityManagerMethodNames */
+    public function testItPreservesTheOriginalExceptionOnRollbackFailure(string $methodName): void
+    {
+        $entityManager = new EntityManagerMock(new class extends ConnectionMock {
+            public function rollBack(): bool
+            {
+                throw new Exception('Rollback exception');
+            }
+        });
+
+        try {
+            $entityManager->transactional(static function (): void {
+                throw new Exception('Original exception');
+            });
+            self::fail('Exception expected');
+        } catch (Exception $e) {
+            self::assertSame('Rollback exception', $e->getMessage());
+            self::assertNotNull($e->getPrevious());
+            self::assertSame('Original exception', $e->getPrevious()->getMessage());
+        }
+    }
+
+    /** @dataProvider entityManagerMethodNames */
+    public function testItDoesNotAttemptToRollbackIfNoTransactionIsActive(string $methodName): void
+    {
+        $entityManager = new EntityManagerMock(
+            new class extends ConnectionMock {
+                public function commit(): bool
+                {
+                    throw new Exception('Commit exception that happens after doing the actual commit');
+                }
+
+                public function rollBack(): bool
+                {
+                    Assert::fail('Should not attempt to rollback if no transaction is active');
+                }
+
+                public function isTransactionActive(): bool
+                {
+                    return false;
+                }
+            }
+        );
+
+        $this->expectExceptionMessage('Commit exception');
+        $entityManager->$methodName(static function (): void {
+        });
+    }
+
+    /** @return Generator<string, array{string}> */
+    public function entityManagerMethodNames(): Generator
+    {
+        foreach (['transactional', 'wrapInTransaction'] as $methodName) {
+            yield sprintf('%s()', $methodName) => [$methodName];
+        }
+    }
 }

tests/Tests/ORM/UnitOfWorkTest.php

@@ -41,6 +41,7 @@ use Doctrine\Tests\Models\GeoNames\City;
 use Doctrine\Tests\Models\GeoNames\Country;
 use Doctrine\Tests\OrmTestCase;
 use Doctrine\Tests\PHPUnitCompatibility\MockBuilderCompatibilityTools;
+use Exception;
 use PHPUnit\Framework\MockObject\MockObject;
 use stdClass;
 
@@ -971,6 +972,43 @@ class UnitOfWorkTest extends OrmTestCase
 
         $this->_unitOfWork->persist($phone2);
     }
+
+    public function testItPreservesTheOriginalExceptionOnRollbackFailure(): void
+    {
+        $this->_connectionMock = new class extends ConnectionMock {
+            public function commit(): bool
+            {
+                return false; // this should cause an exception
+            }
+
+            public function rollBack(): bool
+            {
+                throw new Exception('Rollback exception');
+            }
+        };
+        $this->_emMock         = new EntityManagerMock($this->_connectionMock);
+        $this->_unitOfWork     = new UnitOfWorkMock($this->_emMock);
+        $this->_emMock->setUnitOfWork($this->_unitOfWork);
+
+        // 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);
+
+        try {
+            $this->_unitOfWork->commit();
+            self::fail('Exception expected');
+        } catch (Exception $e) {
+            self::assertSame('Rollback exception', $e->getMessage());
+            self::assertNotNull($e->getPrevious());
+            self::assertSame('Commit failed', $e->getPrevious()->getMessage());
+        }
+    }
 }
 
 /** @Entity */