From 0c745d9021c7a9d65157948b53c85c28b102fd17 Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Tue, 3 May 2022 07:35:10 -0700 Subject: [PATCH 1/5] Pass column name as part of the definition See https://github.com/doctrine/dbal/pull/3583 --- lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php | 1 + lib/Doctrine/ORM/Query/Exec/MultiTableDeleteExecutor.php | 1 + lib/Doctrine/ORM/Query/Exec/MultiTableUpdateExecutor.php | 1 + 3 files changed, 3 insertions(+) diff --git a/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php b/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php index cbf0b5045e7..1caa418908b 100644 --- a/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php +++ b/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php @@ -189,6 +189,7 @@ private function deleteJoinedEntityCollection(PersistentCollection $collection): foreach ($idColumnNames as $idColumnName) { $columnDefinitions[$idColumnName] = [ + 'name' => $idColumnName, 'notnull' => true, 'type' => Type::getType(PersisterHelper::getTypeOfColumn($idColumnName, $rootClass, $this->em)), ]; diff --git a/lib/Doctrine/ORM/Query/Exec/MultiTableDeleteExecutor.php b/lib/Doctrine/ORM/Query/Exec/MultiTableDeleteExecutor.php index 72a0cca3aa1..99b34cefcce 100644 --- a/lib/Doctrine/ORM/Query/Exec/MultiTableDeleteExecutor.php +++ b/lib/Doctrine/ORM/Query/Exec/MultiTableDeleteExecutor.php @@ -92,6 +92,7 @@ public function __construct(AST\Node $AST, $sqlWalker) $columnDefinitions = []; foreach ($idColumnNames as $idColumnName) { $columnDefinitions[$idColumnName] = [ + 'name' => $idColumnName, 'notnull' => true, 'type' => Type::getType(PersisterHelper::getTypeOfColumn($idColumnName, $rootClass, $em)), ]; diff --git a/lib/Doctrine/ORM/Query/Exec/MultiTableUpdateExecutor.php b/lib/Doctrine/ORM/Query/Exec/MultiTableUpdateExecutor.php index f5a2e53772b..31e6b4ebe15 100644 --- a/lib/Doctrine/ORM/Query/Exec/MultiTableUpdateExecutor.php +++ b/lib/Doctrine/ORM/Query/Exec/MultiTableUpdateExecutor.php @@ -133,6 +133,7 @@ public function __construct(AST\Node $AST, $sqlWalker) foreach ($idColumnNames as $idColumnName) { $columnDefinitions[$idColumnName] = [ + 'name' => $idColumnName, 'notnull' => true, 'type' => Type::getType(PersisterHelper::getTypeOfColumn($idColumnName, $rootClass, $em)), ]; From d4a9015c2bee8767717db3d50c071a825ac942be Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Tue, 3 May 2022 07:35:17 -0700 Subject: [PATCH 2/5] Look up only string parameter types See https://github.com/doctrine/dbal/pull/3569 --- lib/Doctrine/ORM/Query/SqlWalker.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index b42929b5a8c..9e8f1c08840 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -2225,7 +2225,7 @@ public function walkInputParameter($inputParam) if ($parameter) { $type = $parameter->getType(); - if (Type::hasType($type)) { + if (is_string($type) && Type::hasType($type)) { return Type::getType($type)->convertToDatabaseValueSQL('?', $this->platform); } } From 6c64f7db347f39c497126afcaa03348d5df91d67 Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Tue, 3 May 2022 07:35:25 -0700 Subject: [PATCH 3/5] Quote only strings See https://github.com/doctrine/dbal/pull/3488 --- lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php | 1 + lib/Doctrine/ORM/Persisters/Entity/SingleTablePersister.php | 4 ++-- lib/Doctrine/ORM/Query/Filter/SQLFilter.php | 6 ++---- lib/Doctrine/ORM/Query/SqlWalker.php | 4 ++-- psalm-baseline.xml | 3 +++ 5 files changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php b/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php index 9230a17bdd0..71e1bd2b99a 100644 --- a/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php +++ b/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php @@ -87,6 +87,7 @@ public function loadMetadataForClass($className, PersistenceClassMetadata $metad continue; } + unset($classAnnotations[$key]); $classAnnotations[get_class($annot)] = $annot; } diff --git a/lib/Doctrine/ORM/Persisters/Entity/SingleTablePersister.php b/lib/Doctrine/ORM/Persisters/Entity/SingleTablePersister.php index bc9becac1cb..af454b666bd 100644 --- a/lib/Doctrine/ORM/Persisters/Entity/SingleTablePersister.php +++ b/lib/Doctrine/ORM/Persisters/Entity/SingleTablePersister.php @@ -135,13 +135,13 @@ protected function getSelectConditionDiscriminatorValueSQL(): string $values = []; if ($this->class->discriminatorValue !== null) { // discriminators can be 0 - $values[] = $this->conn->quote($this->class->discriminatorValue); + $values[] = $this->conn->quote((string) $this->class->discriminatorValue); } $discrValues = array_flip($this->class->discriminatorMap); foreach ($this->class->subClasses as $subclassName) { - $values[] = $this->conn->quote($discrValues[$subclassName]); + $values[] = $this->conn->quote((string) $discrValues[$subclassName]); } $discColumnName = $this->class->getDiscriminatorColumn()['name']; diff --git a/lib/Doctrine/ORM/Query/Filter/SQLFilter.php b/lib/Doctrine/ORM/Query/Filter/SQLFilter.php index 260b539d6d0..5c41fba8d7f 100644 --- a/lib/Doctrine/ORM/Query/Filter/SQLFilter.php +++ b/lib/Doctrine/ORM/Query/Filter/SQLFilter.php @@ -105,9 +105,7 @@ final public function getParameter(string $name): string throw FilterException::cannotConvertListParameterIntoSingleValue($name); } - $param = $this->parameters[$name]; - - return $this->em->getConnection()->quote($param['value'], $param['type']); + return $this->em->getConnection()->quote((string) $this->parameters[$name]['value']); } /** @@ -133,7 +131,7 @@ final public function getParameterList(string $name): string $connection = $this->em->getConnection(); $quoted = array_map( - static fn (mixed $value): string => (string) $connection->quote($value, $param['type']), + static fn (mixed $value): string => $connection->quote((string) $value), $param['value'] ); diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 9e8f1c08840..4dd07526263 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -461,7 +461,7 @@ private function generateDiscriminatorColumnConditionSQL(array $dqlAliases): str } foreach ($class->subClasses as $subclassName) { - $values[] = $conn->quote($this->em->getClassMetadata($subclassName)->discriminatorValue); + $values[] = $conn->quote((string) $this->em->getClassMetadata($subclassName)->discriminatorValue); } $sqlTableAlias = $this->useSqlTableAliases @@ -1812,7 +1812,7 @@ public function walkUpdateItem($updateItem) break; default: - $sql .= $this->conn->quote($newValue); + $sql .= $this->conn->quote((string) $newValue); break; } diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 8bda3a7e50b..230c1ff886e 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1063,6 +1063,9 @@ $columnList + + (string) $discrValues[$subclassName] + From 38e954248cfc27b44746f66af293b2e484f2324d Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Tue, 3 May 2022 07:35:30 -0700 Subject: [PATCH 4/5] Improve mocks --- .../Persisters/BasicEntityPersisterTypeValueSqlTest.php | 4 ++-- tests/Doctrine/Tests/OrmTestCase.php | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Persisters/BasicEntityPersisterTypeValueSqlTest.php b/tests/Doctrine/Tests/ORM/Persisters/BasicEntityPersisterTypeValueSqlTest.php index 9d70cbe0f7a..49eaf5f9159 100644 --- a/tests/Doctrine/Tests/ORM/Persisters/BasicEntityPersisterTypeValueSqlTest.php +++ b/tests/Doctrine/Tests/ORM/Persisters/BasicEntityPersisterTypeValueSqlTest.php @@ -203,7 +203,7 @@ public function testDeleteManyToManyUsesTypeValuesSQL(): void $connection->expects($this->atLeast(2)) ->method('delete') - ->will($this->onConsecutiveCalls( + ->withConsecutive( [ 'customtype_parent_friends', ['friend_customtypeparent_id' => 1], @@ -214,7 +214,7 @@ public function testDeleteManyToManyUsesTypeValuesSQL(): void ['customtypeparent_id' => 1], ['integer'], ] - )); + ); $persister->delete($parent); } diff --git a/tests/Doctrine/Tests/OrmTestCase.php b/tests/Doctrine/Tests/OrmTestCase.php index 8400574597d..e7f7a1e9039 100644 --- a/tests/Doctrine/Tests/OrmTestCase.php +++ b/tests/Doctrine/Tests/OrmTestCase.php @@ -9,6 +9,7 @@ use Doctrine\DBAL\Driver; use Doctrine\DBAL\Platforms\AbstractPlatform; use Doctrine\DBAL\Schema\AbstractSchemaManager; +use Doctrine\DBAL\Schema\SchemaConfig; use Doctrine\ORM\Cache\CacheConfiguration; use Doctrine\ORM\Cache\CacheFactory; use Doctrine\ORM\Cache\DefaultCacheFactory; @@ -181,11 +182,15 @@ private function createDriverMock(AbstractPlatform $platform): Driver $connection->method('query') ->willReturn($result); + $schemaManager = $this->createMock(AbstractSchemaManager::class); + $schemaManager->method('createSchemaConfig') + ->willReturn(new SchemaConfig()); + $driver = $this->createMock(Driver::class); $driver->method('connect') ->willReturn($connection); $driver->method('getSchemaManager') - ->willReturn($this->createMock(AbstractSchemaManager::class)); + ->willReturn($schemaManager); $driver->method('getDatabasePlatform') ->willReturn($platform); From aa8c2937d70b660218ad456f8f3cc6ea66f4c66d Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Tue, 3 May 2022 07:35:33 -0700 Subject: [PATCH 5/5] Catch exception from Connection::commit() --- lib/Doctrine/ORM/OptimisticLockException.php | 5 +++-- lib/Doctrine/ORM/UnitOfWork.php | 15 ++++++++++++--- psalm.xml | 2 ++ tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 6 ++++-- 4 files changed, 21 insertions(+), 7 deletions(-) diff --git a/lib/Doctrine/ORM/OptimisticLockException.php b/lib/Doctrine/ORM/OptimisticLockException.php index d151c8732be..91a9e91b1de 100644 --- a/lib/Doctrine/ORM/OptimisticLockException.php +++ b/lib/Doctrine/ORM/OptimisticLockException.php @@ -7,6 +7,7 @@ use DateTimeInterface; use Doctrine\ORM\Exception\ORMException; use Exception; +use Throwable; /** * An OptimisticLockException is thrown when a version check on an object @@ -21,9 +22,9 @@ class OptimisticLockException extends Exception implements ORMException * @param string $msg * @param object|null $entity */ - public function __construct($msg, $entity) + public function __construct($msg, $entity, ?Throwable $previous = null) { - parent::__construct($msg); + parent::__construct($msg, 0, $previous); $this->entity = $entity; } diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 2ffc928b3e6..51bef411acb 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -9,6 +9,7 @@ use Doctrine\Common\Collections\Collection; use Doctrine\Common\EventManager; use Doctrine\Common\Proxy\Proxy; +use Doctrine\DBAL; use Doctrine\DBAL\Connections\PrimaryReadReplicaConnection; use Doctrine\DBAL\LockMode; use Doctrine\Deprecations\Deprecation; @@ -406,9 +407,17 @@ public function commit(): void } } - // Commit failed silently - if ($conn->commit() === false) { - throw new OptimisticLockException('Commit failed', null); + $commitFailed = false; + try { + if ($conn->commit() === false) { + $commitFailed = true; + } + } catch (DBAL\Exception $e) { + $commitFailed = true; + } + + if ($commitFailed) { + throw new OptimisticLockException('Commit failed', null, $e ?? null); } } catch (Throwable $e) { $this->em->close(); diff --git a/psalm.xml b/psalm.xml index 52abbef79b1..9cafaf741e2 100644 --- a/psalm.xml +++ b/psalm.xml @@ -94,6 +94,8 @@ + + diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index b4619a3b4f8..56de6aedcfa 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -7,6 +7,7 @@ use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; use Doctrine\Common\EventManager; +use Doctrine\DBAL; use Doctrine\DBAL\Connection; use Doctrine\DBAL\Driver; use Doctrine\DBAL\Platforms\AbstractPlatform; @@ -595,7 +596,7 @@ public function testPreviousDetectedIllegalNewNonCascadedEntitiesAreCleanedUpOnS /** * @group #7946 Throw OptimisticLockException when connection::commit() returns false. */ - public function testCommitThrowOptimisticLockExceptionWhenConnectionCommitReturnFalse(): void + public function testCommitThrowOptimisticLockExceptionWhenConnectionCommitFails(): void { $platform = $this->getMockBuilder(AbstractPlatform::class) ->onlyMethods(['supportsIdentityColumns']) @@ -618,7 +619,8 @@ public function testCommitThrowOptimisticLockExceptionWhenConnectionCommitReturn $this->_unitOfWork = new UnitOfWorkMock($this->_emMock); $this->_emMock->setUnitOfWork($this->_unitOfWork); - $this->connection->method('commit')->willReturn(false); + $this->connection->method('commit') + ->willThrowException(new DBAL\Exception()); // Setup fake persister and id generator $userPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata(ForumUser::class));