From 553bd45806a40e72f520cf259fda34f77f83fd8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Niedzielski?= Date: Wed, 20 Nov 2024 13:41:52 +0100 Subject: [PATCH 1/5] Fixed passing DateTime instances into `DoctrineDatabase::findBy()` methods --- .../Gateway/AbstractDoctrineDatabase.php | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/contracts/Gateway/AbstractDoctrineDatabase.php b/src/contracts/Gateway/AbstractDoctrineDatabase.php index 8a9a94d..7da0a54 100644 --- a/src/contracts/Gateway/AbstractDoctrineDatabase.php +++ b/src/contracts/Gateway/AbstractDoctrineDatabase.php @@ -11,6 +11,7 @@ use Doctrine\Common\Collections\Expr\Expression; use Doctrine\DBAL\Connection; use Doctrine\DBAL\Query\QueryBuilder; +use Doctrine\DBAL\Types\ConversionException; use Ibexa\CorePersistence\Gateway\ExpressionVisitor; use InvalidArgumentException; @@ -382,7 +383,21 @@ private function buildCondition(QueryBuilder $qb, string $column, $value): strin return $qb->expr()->isNull($fullColumnName); } + $columnType = $metadata->getColumnType($column); + $platform = $this->connection->getDatabasePlatform(); + if (is_array($value)) { + $value = array_map( + static function ($value) use ($columnType, $platform) { + try { + return $columnType->convertToDatabaseValue($value, $platform); + } catch (ConversionException $e) { + return $value; + } + }, + $value, + ); + $parameter = $qb->createPositionalParameter( $value, $columnBinding + Connection::ARRAY_PARAM_OFFSET @@ -391,6 +406,11 @@ private function buildCondition(QueryBuilder $qb, string $column, $value): strin return $qb->expr()->in($fullColumnName, $parameter); } + try { + $value = $columnType->convertToDatabaseValue($value, $this->connection->getDatabasePlatform()); + } catch (ConversionException $e) { + } + $parameter = $qb->createPositionalParameter($value, $columnBinding); return $qb->expr()->eq($fullColumnName, $parameter); From 2c486339ab49c5c54dfe821b4c8a4beee7b36738 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Niedzielski?= Date: Wed, 20 Nov 2024 14:55:55 +0100 Subject: [PATCH 2/5] Added basic test --- .../Gateway/AbstractDoctrineDatabase.php | 8 +- src/contracts/Gateway/GatewayInterface.php | 6 +- .../Gateway/AbstractDoctrineDatabaseTest.php | 87 +++++++++++++++++++ 3 files changed, 93 insertions(+), 8 deletions(-) create mode 100644 tests/lib/Gateway/AbstractDoctrineDatabaseTest.php diff --git a/src/contracts/Gateway/AbstractDoctrineDatabase.php b/src/contracts/Gateway/AbstractDoctrineDatabase.php index 7da0a54..088de3c 100644 --- a/src/contracts/Gateway/AbstractDoctrineDatabase.php +++ b/src/contracts/Gateway/AbstractDoctrineDatabase.php @@ -117,8 +117,6 @@ public function countAll(): int } /** - * @param \Doctrine\Common\Collections\Expr\Expression|array|null> $criteria - * * @throws \Doctrine\DBAL\Driver\Exception * @throws \Doctrine\DBAL\Exception * @throws \Ibexa\Contracts\CorePersistence\Exception\MappingException @@ -258,7 +256,7 @@ private function applyInheritance(QueryBuilder $qb): void } /** - * @param \Doctrine\Common\Collections\Expr\Expression|array|null> $criteria + * @param \Doctrine\Common\Collections\Expr\Expression|array|null> $criteria * * @return \Doctrine\DBAL\Query\Expression\CompositeExpression|string|null * @@ -326,7 +324,7 @@ private function buildConditionExpression(ExpressionVisitor $visitor, QueryBuild } /** - * @param scalar|array|null $value + * @param scalar|\DateTimeInterface|array|null $value * * @throws \Doctrine\DBAL\Exception * @throws \Ibexa\Contracts\CorePersistence\Exception\MappingException @@ -449,7 +447,7 @@ final protected function applyOrderBy(QueryBuilder $qb, ?array $orderBy = []): v } /** - * @param \Doctrine\Common\Collections\Expr\Expression|array|null> $criteria + * @param \Doctrine\Common\Collections\Expr\Expression|array|null> $criteria */ final protected function applyCriteria(QueryBuilder $qb, $criteria): void { diff --git a/src/contracts/Gateway/GatewayInterface.php b/src/contracts/Gateway/GatewayInterface.php index 248ccde..4e1cc17 100644 --- a/src/contracts/Gateway/GatewayInterface.php +++ b/src/contracts/Gateway/GatewayInterface.php @@ -23,7 +23,7 @@ public function getMetadata(): DoctrineSchemaMetadataInterface; public function countAll(): int; /** - * @param \Doctrine\Common\Collections\Expr\Expression|array<\Doctrine\Common\Collections\Expr\Expression|scalar|array> $criteria + * @param \Doctrine\Common\Collections\Expr\Expression|array<\Doctrine\Common\Collections\Expr\Expression|scalar|\DateTimeInterface|array> $criteria */ public function countBy($criteria): int; @@ -33,7 +33,7 @@ public function countBy($criteria): int; public function findAll(?int $limit = null, int $offset = 0): array; /** - * @param \Doctrine\Common\Collections\Expr\Expression|array<\Doctrine\Common\Collections\Expr\Expression|scalar|array|null> $criteria Map of column names to values that will be used as part of WHERE query + * @param \Doctrine\Common\Collections\Expr\Expression|array<\Doctrine\Common\Collections\Expr\Expression|scalar|\DateTimeInterface|array|null> $criteria Map of column names to values that will be used as part of WHERE query * @param array|null $orderBy Map of column names to "ASC" or "DESC", that will be used in SORT query * * @phpstan-param array|null $orderBy @@ -43,7 +43,7 @@ public function findAll(?int $limit = null, int $offset = 0): array; public function findBy($criteria, ?array $orderBy = null, ?int $limit = null, int $offset = 0): array; /** - * @param array|null> $criteria Map of column names to values that will be used as part of WHERE query + * @param array|null> $criteria Map of column names to values that will be used as part of WHERE query * @param array|null $orderBy Map of column names to "ASC" or "DESC", that will be used in SORT query * * @phpstan-param array|null $orderBy diff --git a/tests/lib/Gateway/AbstractDoctrineDatabaseTest.php b/tests/lib/Gateway/AbstractDoctrineDatabaseTest.php new file mode 100644 index 0000000..69ee98b --- /dev/null +++ b/tests/lib/Gateway/AbstractDoctrineDatabaseTest.php @@ -0,0 +1,87 @@ +createMock(Connection::class); + $connection->expects(self::once()) + ->method('createQueryBuilder') + ->willReturn(new QueryBuilder($connection)); + + $connection->expects(self::atLeastOnce()) + ->method('getDatabasePlatform') + ->willReturn(new PostgreSQL94Platform()); + + $connection->expects(self::atLeastOnce()) + ->method('getExpressionBuilder') + ->willReturn(new ExpressionBuilder($connection)); + + $result = $this->createMock(ResultStatement::class); + $result->expects(self::once()) + ->method('fetch') + ->willReturn(false); + + $connection->expects(self::once()) + ->method('executeQuery') + ->with( + self::identicalTo( + 'SELECT __foo_table__.id, __foo_table__.date_time_immutable_column FROM __foo_table__ __foo_table__ WHERE __foo_table__.date_time_immutable_column = ?', + ), + self::identicalTo([ + 1 => '2024-01-01 00:00:00', + ]), + ) + ->willReturn($result); + + $registry = $this->createMock(DoctrineSchemaMetadataRegistryInterface::class); + + $database = new class($connection, $registry) extends AbstractDoctrineDatabase { + protected function getTableName(): string + { + return '__foo_table__'; + } + + protected function buildMetadata(): DoctrineSchemaMetadataInterface + { + return new DoctrineSchemaMetadata( + $this->connection, + null, + $this->getTableName(), + [ + 'id' => Types::INTEGER, + 'date_time_immutable_column' => Types::DATETIME_IMMUTABLE, + ], + ['id'], + ); + } + }; + + $database->findBy([ + 'date_time_immutable_column' => new \DateTimeImmutable('2024-01-01 00:00:00'), + ]); + } +} From cf73857b3c388644f719fc4e6e91d832cea69a20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Niedzielski?= Date: Fri, 7 Feb 2025 11:04:44 +0100 Subject: [PATCH 3/5] Fixed deprecation report --- phpunit.xml.dist | 6 ++++++ tests/allowed.json | 12 ++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 tests/allowed.json diff --git a/phpunit.xml.dist b/phpunit.xml.dist index cc379dc..208e3a0 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -12,4 +12,10 @@ tests/lib + + + + + + diff --git a/tests/allowed.json b/tests/allowed.json new file mode 100644 index 0000000..0b7ade0 --- /dev/null +++ b/tests/allowed.json @@ -0,0 +1,12 @@ +[ + { + "location": "Doctrine\\DBAL\\Platforms\\PostgreSqlPlatform", + "message": "Using ${var} in strings is deprecated, use {$var} instead", + "count": 1 + }, + { + "location": "Ibexa\\Tests\\CorePersistence\\Gateway\\AbstractDoctrineDatabaseTest::testDateTimeQueries", + "message": "The Doctrine\\DBAL\\Driver\\ResultStatement::fetch method is deprecated (Use fetchNumeric(), fetchAssociative() or fetchOne() instead.).", + "count": 1 + } +] \ No newline at end of file From 9ba1f490812194acac84cf49bf9651341e7e490a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Niedzielski?= Date: Tue, 11 Feb 2025 20:33:00 +0100 Subject: [PATCH 4/5] Added Comparison usage test --- .../Gateway/DoctrineSchemaMetadata.php | 23 ++- .../DoctrineSchemaMetadataInterface.php | 14 ++ src/lib/Gateway/ExpressionVisitor.php | 2 + .../Gateway/AbstractDoctrineDatabaseTest.php | 157 ++++++++++++++---- 4 files changed, 160 insertions(+), 36 deletions(-) diff --git a/src/contracts/Gateway/DoctrineSchemaMetadata.php b/src/contracts/Gateway/DoctrineSchemaMetadata.php index 09fe4a5..7453538 100644 --- a/src/contracts/Gateway/DoctrineSchemaMetadata.php +++ b/src/contracts/Gateway/DoctrineSchemaMetadata.php @@ -236,17 +236,36 @@ public function convertToPHPValues(array $data): array /** * @throws \Doctrine\DBAL\Exception */ - public function convertToDatabaseValues(array $data): array + public function convertToPHPValue(string $columnName, $value) { $platform = $this->connection->getDatabasePlatform(); + + return $this->getColumnType($columnName)->convertToPHPValue($value, $platform); + } + + /** + * @throws \Doctrine\DBAL\Exception + */ + public function convertToDatabaseValues(array $data): array + { $result = []; foreach ($data as $columnName => $value) { - $result[$columnName] = $this->getColumnType($columnName)->convertToDatabaseValue($value, $platform); + $result[$columnName] = $this->convertToDatabaseValue($columnName, $value); } return $result; } + /** + * @throws \Doctrine\DBAL\Exception + */ + public function convertToDatabaseValue(string $column, $value) + { + $platform = $this->connection->getDatabasePlatform(); + + return $this->getColumnType($column)->convertToDatabaseValue($value, $platform); + } + /** * @param array $data * diff --git a/src/contracts/Gateway/DoctrineSchemaMetadataInterface.php b/src/contracts/Gateway/DoctrineSchemaMetadataInterface.php index 18e4b1e..a32cb27 100644 --- a/src/contracts/Gateway/DoctrineSchemaMetadataInterface.php +++ b/src/contracts/Gateway/DoctrineSchemaMetadataInterface.php @@ -67,6 +67,13 @@ public function isInheritedColumn(string $column): bool; */ public function convertToPHPValues(array $data): array; + /** + * @param mixed $value + * + * @return mixed + */ + public function convertToPHPValue(string $columnName, $value); + /** * Similarly to Doctrine\DBAL\Types\Type::convertToDatabaseValue, converts PHP representation to database * representation. @@ -81,6 +88,13 @@ public function convertToPHPValues(array $data): array; */ public function convertToDatabaseValues(array $data): array; + /** + * @param mixed $value + * + * @return mixed + */ + public function convertToDatabaseValue(string $column, $value); + /** * @param array $data * diff --git a/src/lib/Gateway/ExpressionVisitor.php b/src/lib/Gateway/ExpressionVisitor.php index 0fa4a40..e082233 100644 --- a/src/lib/Gateway/ExpressionVisitor.php +++ b/src/lib/Gateway/ExpressionVisitor.php @@ -116,6 +116,8 @@ public function walkComparison(Comparison $comparison) } else { $fullColumnName = $this->tableAlias . '.' . $column; } + + $value = $this->schemaMetadata->convertToDatabaseValue($column, $value); $parameter = new Parameter($parameterName, $value, $type); return $this->handleComparison($comparison, $parameter, $fullColumnName, $placeholder); diff --git a/tests/lib/Gateway/AbstractDoctrineDatabaseTest.php b/tests/lib/Gateway/AbstractDoctrineDatabaseTest.php index 69ee98b..c398e83 100644 --- a/tests/lib/Gateway/AbstractDoctrineDatabaseTest.php +++ b/tests/lib/Gateway/AbstractDoctrineDatabaseTest.php @@ -8,6 +8,8 @@ namespace Ibexa\Tests\CorePersistence\Gateway; +use DateTimeImmutable; +use Doctrine\Common\Collections\Expr\Comparison; use Doctrine\DBAL\Connection; use Doctrine\DBAL\Driver\ResultStatement; use Doctrine\DBAL\Platforms\PostgreSQL94Platform; @@ -25,20 +27,19 @@ */ final class AbstractDoctrineDatabaseTest extends TestCase { - public function testDateTimeQueries(): void - { - $connection = $this->createMock(Connection::class); - $connection->expects(self::once()) - ->method('createQueryBuilder') - ->willReturn(new QueryBuilder($connection)); - - $connection->expects(self::atLeastOnce()) - ->method('getDatabasePlatform') - ->willReturn(new PostgreSQL94Platform()); - - $connection->expects(self::atLeastOnce()) - ->method('getExpressionBuilder') - ->willReturn(new ExpressionBuilder($connection)); + /** + * @dataProvider provideForDateTimeQueries + * + * @param array $query + * @param array $expectedParameters + */ + public function testDateTimeQueries( + array $query, + string $sql, + array $expectedParameters + ): void { + $connection = $this->createConnectionMock(); + $database = $this->createDatabaseGateway($connection); $result = $this->createMock(ResultStatement::class); $result->expects(self::once()) @@ -48,18 +49,98 @@ public function testDateTimeQueries(): void $connection->expects(self::once()) ->method('executeQuery') ->with( - self::identicalTo( - 'SELECT __foo_table__.id, __foo_table__.date_time_immutable_column FROM __foo_table__ __foo_table__ WHERE __foo_table__.date_time_immutable_column = ?', - ), - self::identicalTo([ - 1 => '2024-01-01 00:00:00', - ]), + self::identicalTo($sql), + self::identicalTo($expectedParameters), ) ->willReturn($result); + $database->findBy($query); + } + + /** + * @return iterable, + * non-empty-string, + * array, + * }> + */ + public static function provideForDateTimeQueries(): iterable + { + yield [ + [ + new Comparison( + 'date_time_immutable_column', + Comparison::LT, + new DateTimeImmutable('2024-01-01 00:00:00'), + ), + ], + str_replace( + "\n", + ' ', + << '2024-01-01 00:00:00', + ], + ]; + + yield [ + [ + 'date_time_immutable_column' => new DateTimeImmutable('2024-01-01 00:00:00'), + ], + str_replace( + "\n", + ' ', + << '2024-01-01 00:00:00', + ], + ]; + } + + /** + * @return \Ibexa\Contracts\CorePersistence\Gateway\AbstractDoctrineDatabase> + */ + private function createDatabaseGateway(Connection $connection): AbstractDoctrineDatabase + { $registry = $this->createMock(DoctrineSchemaMetadataRegistryInterface::class); - $database = new class($connection, $registry) extends AbstractDoctrineDatabase { + $metadata = new DoctrineSchemaMetadata( + $connection, + null, + '__foo_table__', + [ + 'id' => Types::INTEGER, + 'date_time_immutable_column' => Types::DATETIME_IMMUTABLE, + ], + ['id'], + ); + + $registry->expects(self::atLeastOnce()) + ->method('getMetadataForTable') + ->with(self::identicalTo('__foo_table__')) + ->willReturn($metadata); + + return new class($connection, $registry, $metadata) extends AbstractDoctrineDatabase { + private DoctrineSchemaMetadata $metadata; + + public function __construct( + Connection $connection, + DoctrineSchemaMetadataRegistryInterface $registry, + DoctrineSchemaMetadata $metadata + ) { + parent::__construct($connection, $registry); + $this->metadata = $metadata; + } + protected function getTableName(): string { return '__foo_table__'; @@ -67,21 +148,29 @@ protected function getTableName(): string protected function buildMetadata(): DoctrineSchemaMetadataInterface { - return new DoctrineSchemaMetadata( - $this->connection, - null, - $this->getTableName(), - [ - 'id' => Types::INTEGER, - 'date_time_immutable_column' => Types::DATETIME_IMMUTABLE, - ], - ['id'], - ); + return $this->metadata; } }; + } + + /** + * @return \Doctrine\DBAL\Connection&\PHPUnit\Framework\MockObject\MockObject + */ + private function createConnectionMock(): Connection + { + $connection = $this->createMock(Connection::class); + $connection->expects(self::once()) + ->method('createQueryBuilder') + ->willReturn(new QueryBuilder($connection)); + + $connection->expects(self::atLeastOnce()) + ->method('getDatabasePlatform') + ->willReturn(new PostgreSQL94Platform()); + + $connection->expects(self::atLeastOnce()) + ->method('getExpressionBuilder') + ->willReturn(new ExpressionBuilder($connection)); - $database->findBy([ - 'date_time_immutable_column' => new \DateTimeImmutable('2024-01-01 00:00:00'), - ]); + return $connection; } } From 24a9bba9a0912aeb21844888be9a17d1bb8008ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Niedzielski?= Date: Wed, 12 Feb 2025 09:58:20 +0100 Subject: [PATCH 5/5] Added Comparison usage test --- tests/bundle/Gateway/ExpressionVisitorTest.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/bundle/Gateway/ExpressionVisitorTest.php b/tests/bundle/Gateway/ExpressionVisitorTest.php index 7e9fc27..ecbd203 100644 --- a/tests/bundle/Gateway/ExpressionVisitorTest.php +++ b/tests/bundle/Gateway/ExpressionVisitorTest.php @@ -437,5 +437,13 @@ private function configureFieldInMetadata(DoctrineSchemaMetadataInterface $metad ...array_map(static fn (string $fieldName): IsIdentical => self::identicalTo($fieldName), $fields), )) ->willReturn(true); + + $metadata + ->expects(self::atLeastOnce()) + ->method('convertToDatabaseValue') + ->with(self::logicalOr( + ...array_map(static fn (string $fieldName): IsIdentical => self::identicalTo($fieldName), $fields), + )) + ->willReturnArgument(1); } }