From 193c3abf0e5f7a7e2699e7c7ac37c1e713d79ef8 Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Sun, 20 Feb 2022 14:09:05 +0100 Subject: [PATCH 1/3] Bring `FilterCollection` to a "clean" state after hash computation (#9523) Co-authored-by: Alexander M. Turek --- lib/Doctrine/ORM/Query/FilterCollection.php | 9 ++++--- phpstan-baseline.neon | 5 ---- .../Tests/ORM/Query/FilterCollectionTest.php | 25 ++++++++++++++++--- 3 files changed, 27 insertions(+), 12 deletions(-) diff --git a/lib/Doctrine/ORM/Query/FilterCollection.php b/lib/Doctrine/ORM/Query/FilterCollection.php index 7e9ac71bab9..87471cf1117 100644 --- a/lib/Doctrine/ORM/Query/FilterCollection.php +++ b/lib/Doctrine/ORM/Query/FilterCollection.php @@ -97,8 +97,7 @@ public function enable($name) // Keep the enabled filters sorted for the hash ksort($this->enabledFilters); - // Now the filter collection is dirty - $this->filtersState = self::FILTERS_STATE_DIRTY; + $this->setFiltersStateDirty(); } return $this->enabledFilters[$name]; @@ -120,8 +119,7 @@ public function disable($name) unset($this->enabledFilters[$name]); - // Now the filter collection is dirty - $this->filtersState = self::FILTERS_STATE_DIRTY; + $this->setFiltersStateDirty(); return $filter; } @@ -194,6 +192,9 @@ public function getHash() $filterHash .= $name . $filter; } + $this->filterHash = $filterHash; + $this->filtersState = self::FILTERS_STATE_CLEAN; + return $filterHash; } diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index c35eafef735..3fa8ee23057 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -875,11 +875,6 @@ parameters: count: 1 path: lib/Doctrine/ORM/Query/Expr/Select.php - - - message: "#^Property Doctrine\\\\ORM\\\\Query\\\\FilterCollection\\:\\:\\$filterHash is never written, only read\\.$#" - count: 1 - path: lib/Doctrine/ORM/Query/FilterCollection.php - - message: "#^Else branch is unreachable because ternary operator condition is always true\\.$#" count: 3 diff --git a/tests/Doctrine/Tests/ORM/Query/FilterCollectionTest.php b/tests/Doctrine/Tests/ORM/Query/FilterCollectionTest.php index 304c31d169d..b65d857e1c3 100644 --- a/tests/Doctrine/Tests/ORM/Query/FilterCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/Query/FilterCollectionTest.php @@ -4,17 +4,18 @@ namespace Doctrine\Tests\ORM\Query; -use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\Query\Filter\SQLFilter; +use Doctrine\Tests\Mocks\EntityManagerMock; use Doctrine\Tests\OrmTestCase; +use InvalidArgumentException; /** * Test case for FilterCollection */ class FilterCollectionTest extends OrmTestCase { - /** @var EntityManagerInterface */ + /** @var EntityManagerMock */ private $em; protected function setUp(): void @@ -64,7 +65,7 @@ public function testIsEnabled(): void public function testGetFilterInvalidArgument(): void { - $this->expectException('InvalidArgumentException'); + $this->expectException(InvalidArgumentException::class); $filterCollection = $this->em->getFilters(); $filterCollection->getFilter('testFilter'); } @@ -76,6 +77,24 @@ public function testGetFilter(): void self::assertInstanceOf(MyFilter::class, $filterCollection->getFilter('testFilter')); } + + public function testHashing(): void + { + $filterCollection = $this->em->getFilters(); + + self::assertTrue($filterCollection->isClean()); + + $oldHash = $filterCollection->getHash(); + $filterCollection->enable('testFilter'); + + self::assertFalse($filterCollection->isClean()); + + $hash = $filterCollection->getHash(); + + self::assertNotSame($oldHash, $hash); + self::assertTrue($filterCollection->isClean()); + self::assertSame($hash, $filterCollection->getHash()); + } } class MyFilter extends SQLFilter From 08eaba44ca081311495b4c153a9fab383863a06e Mon Sep 17 00:00:00 2001 From: "Alexander M. Turek" Date: Sun, 20 Feb 2022 21:09:41 +0100 Subject: [PATCH 2/3] Fix more types on EntityRepository and FilterCollection (#9525) --- lib/Doctrine/ORM/Configuration.php | 4 ++- lib/Doctrine/ORM/EntityRepository.php | 9 +++++- lib/Doctrine/ORM/Query/FilterCollection.php | 21 ++++++++++--- psalm-baseline.xml | 30 ++----------------- .../Tests/ORM/Query/FilterCollectionTest.php | 12 ++++++++ .../DefaultRepositoryFactoryTest.php | 16 ++++++---- 6 files changed, 52 insertions(+), 40 deletions(-) diff --git a/lib/Doctrine/ORM/Configuration.php b/lib/Doctrine/ORM/Configuration.php index b17eefee64f..aacf6053308 100644 --- a/lib/Doctrine/ORM/Configuration.php +++ b/lib/Doctrine/ORM/Configuration.php @@ -34,6 +34,7 @@ use Doctrine\ORM\Mapping\EntityListenerResolver; use Doctrine\ORM\Mapping\NamingStrategy; use Doctrine\ORM\Mapping\QuoteStrategy; +use Doctrine\ORM\Query\Filter\SQLFilter; use Doctrine\ORM\Query\ResultSetMapping; use Doctrine\ORM\Repository\DefaultRepositoryFactory; use Doctrine\ORM\Repository\RepositoryFactory; @@ -804,6 +805,7 @@ public function getClassMetadataFactoryName() * * @param string $name The name of the filter. * @param string $className The class name of the filter. + * @psalm-param class-string $className * * @return void */ @@ -819,7 +821,7 @@ public function addFilter($name, $className) * * @return string|null The class name of the filter, or null if it is not * defined. - * @psalm-return ?class-string + * @psalm-return class-string|null */ public function getFilterClassName($name) { diff --git a/lib/Doctrine/ORM/EntityRepository.php b/lib/Doctrine/ORM/EntityRepository.php index 0ba6de28b1d..caf8b4527bf 100644 --- a/lib/Doctrine/ORM/EntityRepository.php +++ b/lib/Doctrine/ORM/EntityRepository.php @@ -40,6 +40,7 @@ class EntityRepository implements ObjectRepository, Selectable * @internal This property will be private in 3.0, call {@see getEntityName()} instead. * * @var string + * @psalm-var class-string */ protected $_entityName; @@ -54,12 +55,16 @@ class EntityRepository implements ObjectRepository, Selectable * @internal This property will be private in 3.0, call {@see getClassMetadata()} instead. * * @var ClassMetadata + * @psalm-var ClassMetadata */ protected $_class; /** @var Inflector|null */ private static $inflector; + /** + * @psalm-param ClassMetadata $class + */ public function __construct(EntityManagerInterface $em, ClassMetadata $class) { $this->_entityName = $class->name; @@ -276,6 +281,7 @@ public function __call($method, $arguments) /** * @return string + * @psalm-return class-string */ protected function getEntityName() { @@ -283,7 +289,7 @@ protected function getEntityName() } /** - * @return string + * {@inheritdoc} */ public function getClassName() { @@ -300,6 +306,7 @@ protected function getEntityManager() /** * @return ClassMetadata + * @psalm-return ClassMetadata */ protected function getClassMetadata() { diff --git a/lib/Doctrine/ORM/Query/FilterCollection.php b/lib/Doctrine/ORM/Query/FilterCollection.php index 87471cf1117..01c7e2c61c1 100644 --- a/lib/Doctrine/ORM/Query/FilterCollection.php +++ b/lib/Doctrine/ORM/Query/FilterCollection.php @@ -47,13 +47,23 @@ class FilterCollection * Instances of enabled filters. * * @var SQLFilter[] + * @psalm-var array */ private $enabledFilters = []; - /** @var string The filter hash from the last time the query was parsed. */ - private $filterHash; + /** + * The filter hash from the last time the query was parsed. + * + * @var string + */ + private $filterHash = ''; - /** @var int The current state of this filter. */ + /** + * The current state of this filter. + * + * @var int + * @psalm-var self::FILTERS_STATE_* + */ private $filtersState = self::FILTERS_STATE_CLEAN; public function __construct(EntityManagerInterface $em) @@ -66,6 +76,7 @@ public function __construct(EntityManagerInterface $em) * Gets all the enabled filters. * * @return SQLFilter[] The enabled filters. + * @psalm-return array */ public function getEnabledFilters() { @@ -167,7 +178,9 @@ public function isEnabled($name) } /** - * @return bool True, if the filter collection is clean. + * Checks if the filter collection is clean. + * + * @return bool */ public function isClean() { diff --git a/psalm-baseline.xml b/psalm-baseline.xml index f927695797b..63f622c0b3b 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -353,26 +353,14 @@ - - $this->_entityName - $this->_entityName - $this->_entityName - $this->_entityName - $this->_entityName - - + $persister->load($criteria, null, null, [], null, 1, $orderBy) - $this->_em->find($this->_entityName, $id, $lockMode, $lockVersion) new LazyCriteriaCollection($persister, $criteria) - - ?T + ?T AbstractLazyCollection<int, T>&Selectable<int, T> - - string - find @@ -2163,20 +2151,6 @@ $this->parameters - - - $this->enabledFilters[$name] - - - SQLFilter - - - $filterHash - - - $this->enabledFilters - - $stringPattern diff --git a/tests/Doctrine/Tests/ORM/Query/FilterCollectionTest.php b/tests/Doctrine/Tests/ORM/Query/FilterCollectionTest.php index b65d857e1c3..19b509b4e54 100644 --- a/tests/Doctrine/Tests/ORM/Query/FilterCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/Query/FilterCollectionTest.php @@ -85,6 +85,12 @@ public function testHashing(): void self::assertTrue($filterCollection->isClean()); $oldHash = $filterCollection->getHash(); + $filterCollection->setFiltersStateDirty(); + + self::assertFalse($filterCollection->isClean()); + self::assertSame($oldHash, $filterCollection->getHash()); + self::assertTrue($filterCollection->isClean()); + $filterCollection->enable('testFilter'); self::assertFalse($filterCollection->isClean()); @@ -94,6 +100,12 @@ public function testHashing(): void self::assertNotSame($oldHash, $hash); self::assertTrue($filterCollection->isClean()); self::assertSame($hash, $filterCollection->getHash()); + + $filterCollection->disable('testFilter'); + + self::assertFalse($filterCollection->isClean()); + self::assertSame($oldHash, $filterCollection->getHash()); + self::assertTrue($filterCollection->isClean()); } } diff --git a/tests/Doctrine/Tests/ORM/Repository/DefaultRepositoryFactoryTest.php b/tests/Doctrine/Tests/ORM/Repository/DefaultRepositoryFactoryTest.php index 052d9b8511c..dd42ac1c42b 100644 --- a/tests/Doctrine/Tests/ORM/Repository/DefaultRepositoryFactoryTest.php +++ b/tests/Doctrine/Tests/ORM/Repository/DefaultRepositoryFactoryTest.php @@ -10,6 +10,7 @@ use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\Repository\DefaultRepositoryFactory; use Doctrine\Tests\Models\DDC753\DDC753DefaultRepository; +use Doctrine\Tests\Models\DDC753\DDC753EntityWithDefaultCustomRepository; use Doctrine\Tests\Models\DDC869\DDC869PaymentRepository; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -70,7 +71,7 @@ public function testCreatedRepositoriesAreCached(): void public function testCreatesRepositoryFromCustomClassMetadata(): void { - $customMetadata = $this->buildClassMetadata(__DIR__); + $customMetadata = $this->buildClassMetadata(DDC753EntityWithDefaultCustomRepository::class); $customMetadata->customRepositoryClassName = DDC753DefaultRepository::class; $this->entityManager @@ -107,12 +108,18 @@ public function testCachesDistinctRepositoriesPerDistinctEntityManager(): void } /** + * @psalm-param class-string $className + * * @return ClassMetadata&MockObject + * @psalm-return ClassMetadata&MockObject + * + * @template TEntity of object */ private function buildClassMetadata(string $className): ClassMetadata { $metadata = $this->createMock(ClassMetadata::class); - $metadata->expects(self::any())->method('getName')->will(self::returnValue($className)); + $metadata->method('getName')->willReturn($className); + $metadata->name = $className; $metadata->customRepositoryClassName = null; @@ -125,10 +132,7 @@ private function buildClassMetadata(string $className): ClassMetadata private function createEntityManager(): EntityManagerInterface { $entityManager = $this->createMock(EntityManagerInterface::class); - - $entityManager->expects(self::any()) - ->method('getConfiguration') - ->will(self::returnValue($this->configuration)); + $entityManager->method('getConfiguration')->willReturn($this->configuration); return $entityManager; } From 65f48e0ecd58345296afca351be77b022b96c04e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Tue, 22 Feb 2022 14:00:04 +0100 Subject: [PATCH 3/3] Drop minor version number We should make it explicit that we mean to test with whatever is the latest 3.x --- .github/workflows/continuous-integration.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index b32944190ac..a2b7cdd0cb6 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -30,7 +30,7 @@ jobs: - php-version: "8.0" dbal-version: "2.13" - php-version: "8.1" - dbal-version: "3.2@dev" + dbal-version: "3@dev" steps: - name: "Checkout"