From 9fff33b8cd0103716e570098d6a4ec4093700153 Mon Sep 17 00:00:00 2001 From: Michael Babker Date: Tue, 7 May 2024 20:58:08 -0400 Subject: [PATCH] Deprecate not returning a query builder from the DBAL adapter's modifier callback --- composer.json | 3 ++- docs/adapters.md | 8 +++++-- lib/Adapter/Doctrine/DBAL/QueryAdapter.php | 12 +++++++--- .../Doctrine/DBAL/SingleTableQueryAdapter.php | 4 +++- .../Doctrine/DBAL/Tests/QueryAdapterTest.php | 22 ++++++++++++++----- lib/Adapter/Doctrine/DBAL/composer.json | 3 ++- 6 files changed, 39 insertions(+), 13 deletions(-) diff --git a/composer.json b/composer.json index e07b08e0..4f2036e5 100644 --- a/composer.json +++ b/composer.json @@ -6,7 +6,8 @@ "license": "MIT", "require": { "php": "^8.1", - "ext-json": "*" + "ext-json": "*", + "symfony/deprecation-contracts": "^2.1 || ^3.0" }, "require-dev": { "doctrine/collections": "^1.8 || ^2.0", diff --git a/docs/adapters.md b/docs/adapters.md index 3c35bd54..348308e5 100644 --- a/docs/adapters.md +++ b/docs/adapters.md @@ -107,7 +107,9 @@ $adapter = new SingleTableQueryAdapter($query, 'p.id'); The `QueryAdapter` is the main adapter for use with the DBAL package, you should use this on queries that have join statements. -The class constructor requires a `Doctrine\DBAL\Query\QueryBuilder` and a callable which can be used to modify a clone of the `QueryBuilder` for a COUNT query. The callable should have a signature of `function (QueryBuilder $queryBuilder): void {}`. +The class constructor requires a `Doctrine\DBAL\Query\QueryBuilder` and a callable which can be used to modify a clone of the query builder for a COUNT query. The callable should have a signature of `function (QueryBuilder $queryBuilder): QueryBuilder {}`. + +
Before Pagerfanta 4.6, a return from the callable was ignored and the query builder passed to the callable was returned. This approach is deprecated in favor of the callable returning a query builder object, be it the provided builder or a new instance. In Pagerfanta 5.0, this return will be required.
Below is an example of using the `QueryAdapter`. @@ -129,9 +131,11 @@ $query = $connection->createQueryBuilder() ->select('p.*') ->from('posts', 'p'); -$countQueryBuilderModifier = static function (QueryBuilder $queryBuilder): void { +$countQueryBuilderModifier = static function (QueryBuilder $queryBuilder): QueryBuilder { $queryBuilder->select('COUNT(DISTINCT p.id) AS total_results') ->setMaxResults(1); + + return $queryBuilder; }; $adapter = new QueryAdapter($query, $countQueryBuilderModifier); diff --git a/lib/Adapter/Doctrine/DBAL/QueryAdapter.php b/lib/Adapter/Doctrine/DBAL/QueryAdapter.php index 36feec8c..89165fd4 100644 --- a/lib/Adapter/Doctrine/DBAL/QueryAdapter.php +++ b/lib/Adapter/Doctrine/DBAL/QueryAdapter.php @@ -19,12 +19,12 @@ class QueryAdapter implements AdapterInterface /** * @var callable * - * @phpstan-var callable(QueryBuilder): void + * @phpstan-var callable(QueryBuilder): (QueryBuilder|void) */ private $countQueryBuilderModifier; /** - * @phpstan-param callable(QueryBuilder): void $countQueryBuilderModifier + * @phpstan-param callable(QueryBuilder): (QueryBuilder|void) $countQueryBuilderModifier */ public function __construct(QueryBuilder $queryBuilder, callable $countQueryBuilderModifier) { @@ -63,7 +63,13 @@ private function prepareCountQueryBuilder(): QueryBuilder $qb = clone $this->queryBuilder; $callable = $this->countQueryBuilderModifier; - $callable($qb); + $newQb = $callable($qb); + + if ($newQb instanceof QueryBuilder) { + return $newQb; + } + + trigger_deprecation('pagerfanta/dbal', '4.6', 'Not returning a "%s" from the query builder modifier in "%s" is deprecated. In 5.0, returning a query builder object will be required.'); return $qb; } diff --git a/lib/Adapter/Doctrine/DBAL/SingleTableQueryAdapter.php b/lib/Adapter/Doctrine/DBAL/SingleTableQueryAdapter.php index 8a232d65..2617cf0e 100644 --- a/lib/Adapter/Doctrine/DBAL/SingleTableQueryAdapter.php +++ b/lib/Adapter/Doctrine/DBAL/SingleTableQueryAdapter.php @@ -28,7 +28,7 @@ private function createCountQueryModifier(string $countField): \Closure { $select = $this->createSelectForCountField($countField); - return static function (QueryBuilder $queryBuilder) use ($select): void { + return static function (QueryBuilder $queryBuilder) use ($select): QueryBuilder { $queryBuilder->select($select); if (method_exists($queryBuilder, 'resetOrderBy')) { @@ -38,6 +38,8 @@ private function createCountQueryModifier(string $countField): \Closure } $queryBuilder->setMaxResults(1); + + return $queryBuilder; }; } diff --git a/lib/Adapter/Doctrine/DBAL/Tests/QueryAdapterTest.php b/lib/Adapter/Doctrine/DBAL/Tests/QueryAdapterTest.php index e6c161da..d23765b4 100644 --- a/lib/Adapter/Doctrine/DBAL/Tests/QueryAdapterTest.php +++ b/lib/Adapter/Doctrine/DBAL/Tests/QueryAdapterTest.php @@ -4,6 +4,7 @@ use Doctrine\DBAL\Query\QueryBuilder; use Pagerfanta\Doctrine\DBAL\QueryAdapter; +use PHPUnit\Framework\Attributes\Group; final class QueryAdapterTest extends DBALTestCase { @@ -17,6 +18,20 @@ protected function setUp(): void $this->qb->select('p.*')->from('posts', 'p'); } + #[Group('legacy')] + public function testAdapterReturnsNumberOfResultsWhenCallbackUsesDeprecatedNoReturnSignature(): void + { + $adapter = new QueryAdapter( + $this->qb, + static function (QueryBuilder $qb): void { + $qb->select('COUNT(DISTINCT p.id) AS total_results') + ->setMaxResults(1); + } + ); + + self::assertSame(50, $adapter->getNbResults()); + } + public function testAdapterReturnsNumberOfResults(): void { $adapter = $this->createAdapterToTestGetNbResults(); @@ -35,7 +50,7 @@ public function testResultCountStaysConsistentAfterSlicing(): void public function testGetSlice(): void { - $adapter = new QueryAdapter($this->qb, static function (QueryBuilder $qb): void {}); + $adapter = new QueryAdapter($this->qb, static fn (QueryBuilder $qb): QueryBuilder => $qb); $offset = 30; $length = 10; @@ -63,10 +78,7 @@ private function createAdapterToTestGetNbResults(): QueryAdapter { return new QueryAdapter( $this->qb, - static function (QueryBuilder $qb): void { - $qb->select('COUNT(DISTINCT p.id) AS total_results') - ->setMaxResults(1); - } + static fn (QueryBuilder $qb): QueryBuilder => $qb->select('COUNT(DISTINCT p.id) AS total_results')->setMaxResults(1), ); } } diff --git a/lib/Adapter/Doctrine/DBAL/composer.json b/lib/Adapter/Doctrine/DBAL/composer.json index 2c0e3eee..24c90ac9 100644 --- a/lib/Adapter/Doctrine/DBAL/composer.json +++ b/lib/Adapter/Doctrine/DBAL/composer.json @@ -7,7 +7,8 @@ "require": { "php": "^8.1", "doctrine/dbal": "^3.5 || ^4.0", - "pagerfanta/core": "^3.7 || ^4.0" + "pagerfanta/core": "^3.7 || ^4.0", + "symfony/deprecation-contracts": "^2.1 || ^3.0" }, "require-dev": { "phpunit/phpunit": "^10.5"