From 3b7fb6c4ee89dbaba18732872bad91aaa51bd36a Mon Sep 17 00:00:00 2001 From: David Buchmann Date: Sat, 4 May 2024 22:25:56 +0200 Subject: [PATCH] strict typing --- CHANGELOG.md | 1 + .../Compiler/AddPublishedVotersPass.php | 2 +- src/Templating/Helper/Cmf.php | 148 ++++++++---------- src/Translatable/TranslatableInterface.php | 6 +- .../CmfCoreExtensionTest.php | 5 +- .../Templating/Helper/CmfHierarchyTest.php | 31 ++-- tests/Functional/Twig/ServiceTest.php | 2 +- .../Unit/Twig/Extension/CmfExtensionTest.php | 7 +- 8 files changed, 82 insertions(+), 120 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40fb2b3..6c2ab0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ Changelog * Drop support for old Symfony versions * Supprt PHP 8.1 - 8.3 * Drop support for old PHP versions +* TranslatableInterface now uses typehints, adjust your implementations accordingly. * Use DateTimeInterface instead of DateTime. * Adjust to doctrine and twig BC breaks. If you extended classes or customized services, check for old `Twig_*` classes or `Doctrine\Common\Persistence` namespace. diff --git a/src/DependencyInjection/Compiler/AddPublishedVotersPass.php b/src/DependencyInjection/Compiler/AddPublishedVotersPass.php index 08846dc..8484861 100644 --- a/src/DependencyInjection/Compiler/AddPublishedVotersPass.php +++ b/src/DependencyInjection/Compiler/AddPublishedVotersPass.php @@ -39,7 +39,7 @@ public function process(ContainerBuilder $container): void $voters = new \SplPriorityQueue(); foreach ($container->findTaggedServiceIds('cmf_published_voter') as $id => $attributes) { - $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0; + $priority = $attributes[0]['priority'] ?? 0; $voters->insert(new Reference($id), $priority); } diff --git a/src/Templating/Helper/Cmf.php b/src/Templating/Helper/Cmf.php index 87c1e6e..30bd265 100644 --- a/src/Templating/Helper/Cmf.php +++ b/src/Templating/Helper/Cmf.php @@ -176,13 +176,11 @@ private function getDocument($document, ?bool $ignoreRole = false, ?string $clas } /** - * @param array $paths list of paths - * @param int|bool $limit int limit or false - * @param string|bool $offset string node name to which to skip to or false - * @param bool|null $ignoreRole if the role should be ignored or null if publish workflow should be ignored - * @param string|null $class class name to filter on + * @param string|false $offset string node name to which to skip to or false to start from the beginning + * @param bool|null $ignoreRole if the role should be ignored or null if publish workflow should be ignored + * @param string|null $class class name to filter on */ - public function findMany(array $paths = [], $limit = false, $offset = false, ?bool $ignoreRole = false, ?string $class = null): array + public function findMany(array $paths = [], int|false $limit = false, string|false $offset = false, ?bool $ignoreRole = false, ?string $class = null): array { if ($offset) { $paths = \array_slice($paths, $offset); @@ -212,16 +210,14 @@ public function findMany(array $paths = [], $limit = false, $offset = false, ?bo * * If you need the bypass role, you will have a firewall configured and can * simply use {{ is_granted('VIEW', document) }} - * - * @param ?object $document */ - public function isPublished($document): bool + public function isPublished(?object $document): bool { if (null === $this->publishWorkflowChecker) { throw new InvalidConfigurationException('You can not check for publication as the publish workflow is not enabled.'); } - if (empty($document)) { + if (null === $document) { return false; } @@ -231,17 +227,15 @@ public function isPublished($document): bool /** * Get the locales of the document. * - * @param string|object $document Document instance or path - * * @return string[] */ - public function getLocalesFor($document, bool $includeFallbacks = false): array + public function getLocalesFor(object|string|null $document, bool $includeFallbacks = false): array { if (\is_string($document)) { $document = $this->getDm()->find(null, $document); } - if (empty($document)) { + if (null === $document) { return []; } @@ -255,18 +249,18 @@ public function getLocalesFor($document, bool $includeFallbacks = false): array } /** - * @param string|object $parent parent path/document + * @param object|string $parent parent path/document * * @return bool|object|null child or null if the child cannot be found * or false if the parent is not managed by * the configured document manager */ - public function getChild($parent, string $name): object|bool|null + public function getChild(object|string $parent, string $name): object|bool|null { if (\is_object($parent)) { try { $parent = $this->getDm()->getUnitOfWork()->getDocumentId($parent); - } catch (\Exception $e) { + } catch (\Exception) { return false; } } @@ -277,17 +271,16 @@ public function getChild($parent, string $name): object|bool|null /** * Gets child documents. * - * @param string|object $parent parent id or document - * @param int|bool $limit maximum number of children to get or - * false for no limit - * @param string|bool $offset node name to which to skip to or false - * @param string|null $filter child name filter (optional) - * @param bool|null $ignoreRole whether the role should be ignored or - * null if publish workflow should be - * ignored (defaults to false) - * @param string|null $class class name to filter on (optional) + * @param int|false $limit maximum number of children to get or + * false for no limit + * @param string|false $offset node name to which to skip to or false + * @param string|null $filter child name filter (optional) + * @param bool|null $ignoreRole whether the role should be ignored or + * null if publish workflow should be + * ignored (defaults to false) + * @param string|null $class class name to filter on (optional) */ - public function getChildren($parent, $limit = false, $offset = false, $filter = null, $ignoreRole = false, $class = null): array + public function getChildren(object|string|null $parent, int|false $limit = false, string|false $offset = false, ?string $filter = null, ?bool $ignoreRole = false, ?string $class = null): array { if (empty($parent)) { return []; @@ -345,19 +338,18 @@ public function getChildren($parent, $limit = false, $offset = false, $filter = * * This has the same semantics as the isLinkable method. * - * @param string|object $parent parent path/document - * @param int|bool $limit limit or false for no limit - * @param string|bool $offset node name to which to skip to or false - * to not skip any elements - * @param string|null $filter child name filter - * @param bool|null $ignoreRole whether the role should be ignored or - * null if publish workflow should be - * ignored (defaults to false) - * @param string|null $class class name to filter on + * @param int|false $limit limit or false for no limit + * @param string|false $offset node name to which to skip to or false + * to not skip any elements + * @param string|null $filter child name filter + * @param bool|null $ignoreRole whether the role should be ignored or + * null if publish workflow should be + * ignored (defaults to false) + * @param string|null $class class name to filter on * * @see isLinkable */ - public function getLinkableChildren($parent, $limit = false, $offset = false, ?string $filter = null, ?bool $ignoreRole = false, ?string $class = null): array + public function getLinkableChildren(object|string|null $parent, int|false $limit = false, string|false $offset = false, ?string $filter = null, ?bool $ignoreRole = false, ?string $class = null): array { $children = $this->getChildren($parent, $limit, $offset, $filter, $ignoreRole, $class); foreach ($children as $key => $value) { @@ -399,18 +391,19 @@ public function isLinkable(mixed $document): bool * @param string[] $children * @param ?int $depth */ - private function getChildrenPaths(string $path, array &$children, ?int $depth): void + private function getChildrenPaths(?string $path, array &$children, ?int $depth): void { - if (null !== $depth) { - if ($depth-- < 1) { - return; - } + if (null === $path) { + return; + } + if ((null !== $depth) && $depth-- < 1) { + return; } $node = $this->getDm()->getPhpcrSession()->getNode($path); $names = (array) $node->getNodeNames(); foreach ($names as $name) { - if (0 === strpos($name, 'phpcr_locale:')) { + if (str_starts_with($name, 'phpcr_locale:')) { continue; } @@ -420,15 +413,14 @@ private function getChildrenPaths(string $path, array &$children, ?int $depth): } /** - * @param string|object $parent parent path/document - * @param int|null $depth null denotes no limit, depth of 1 means - * direct children only + * @param int|null $depth null denotes no limit, depth of 1 means + * direct children only * * @return string[] */ - public function getDescendants($parent, ?int $depth = null): array + public function getDescendants(object|string|null $parent, ?int $depth = null): array { - if (empty($parent)) { + if ('' === $parent) { return []; } @@ -443,10 +435,8 @@ public function getDescendants($parent, ?int $depth = null): array /** * Check children for a possible following document. - * - * @return object|null */ - private function checkChildren(array $childNames, string $path, ?bool $ignoreRole = false, ?string $class = null): object|string|null + private function checkChildren(array $childNames, string $path, ?bool $ignoreRole = false, ?string $class = null): object|null { foreach ($childNames as $name) { if (str_starts_with($name, 'phpcr_locale:')) { @@ -465,10 +455,8 @@ private function checkChildren(array $childNames, string $path, ?bool $ignoreRol /** * Traverse the depth to find previous documents. - * - * @return object|null */ - private function traversePrevDepth(?int $depth, int $anchorDepth, array $childNames, string $path, bool $ignoreRole, ?string $class): object|string|null + private function traversePrevDepth(?int $depth, int $anchorDepth, array $childNames, string $path, bool $ignoreRole, ?string $class): object|null { foreach ($childNames as $childName) { $childPath = "$path/$childName"; @@ -496,15 +484,13 @@ private function traversePrevDepth(?int $depth, int $anchorDepth, array $childNa /** * Search for a previous document. * - * @param string|object $path document instance or path from which to search - * @param string|object $anchor document instance or path which serves as an anchor from which to flatten the hierarchy + * @param object|string $path document instance or path from which to search + * @param object|string $anchor document instance or path which serves as an anchor from which to flatten the hierarchy * @param int|null $depth depth up to which to traverse down the tree when an anchor is provided * @param bool $ignoreRole if to ignore the role * @param string|null $class the class to filter by - * - * @return object|null */ - private function searchDepthPrev($path, $anchor, ?int $depth = null, ?bool $ignoreRole = false, ?string $class = null): object|string|null + private function searchDepthPrev(object|string $path, object|string $anchor, ?int $depth = null, ?bool $ignoreRole = false, ?string $class = null): object|null { if (\is_object($path)) { $path = $this->getDm()->getUnitOfWork()->getDocumentId($path); @@ -520,7 +506,7 @@ private function searchDepthPrev($path, $anchor, ?int $depth = null, ?bool $igno $anchor = $this->getDm()->getUnitOfWork()->getDocumentId($anchor); } - if (0 !== strpos($path, $anchor)) { + if (!str_starts_with($path, $anchor)) { throw new \RuntimeException("The anchor path '$anchor' is not a parent of the current path '$path'."); } @@ -573,13 +559,13 @@ private function searchDepthPrev($path, $anchor, ?int $depth = null, ?bool $igno /** * Search for a next document. * - * @param string|object $path document instance or path from which to search - * @param string|object $anchor document instance or path which serves as an anchor from which to flatten the hierarchy + * @param object|string $path document instance or path from which to search + * @param object|string $anchor document instance or path which serves as an anchor from which to flatten the hierarchy * @param int|null $depth depth up to which to traverse down the tree when an anchor is provided * @param bool $ignoreRole if to ignore the role * @param string|null $class the class to filter by */ - private function searchDepthNext($path, $anchor, ?int $depth = null, ?bool $ignoreRole = false, ?string $class = null): object|string|null + private function searchDepthNext(object|string $path, object|string $anchor, ?int $depth = null, ?bool $ignoreRole = false, ?string $class = null): object|null { if (\is_object($path)) { $path = $this->getDm()->getUnitOfWork()->getDocumentId($path); @@ -649,10 +635,8 @@ private function searchDepthNext($path, $anchor, ?int $depth = null, ?bool $igno * @param bool $reverse if to traverse back * @param bool $ignoreRole if to ignore the role * @param string|null $class the class to filter by - * - * @return object|null */ - private function search($path, ?bool $reverse = false, ?bool $ignoreRole = false, ?string $class = null): object|string|null + private function search($path, ?bool $reverse = false, ?bool $ignoreRole = false, ?string $class = null): object|null { if (\is_object($path)) { $path = $this->getDm()->getUnitOfWork()->getDocumentId($path); @@ -678,15 +662,13 @@ private function search($path, ?bool $reverse = false, ?bool $ignoreRole = false /** * Gets the previous document. * - * @param string|object $current document instance or path from which to search - * @param string|object|null $anchor document instance or path which serves as an anchor from which to flatten the hierarchy + * @param object|string|null $current document instance or path from which to search + * @param object|string|null $anchor document instance or path which serves as an anchor from which to flatten the hierarchy * @param int|null $depth depth up to which to traverse down the tree when an anchor is provided * @param bool $ignoreRole if to ignore the role * @param string|null $class the class to filter by - * - * @return object|null */ - public function getPrev($current, $anchor = null, ?int $depth = null, ?bool $ignoreRole = false, ?string $class = null): object|string|null + public function getPrev(object|string|null $current, object|string|null $anchor = null, ?int $depth = null, ?bool $ignoreRole = false, ?string $class = null): object|null { if ($anchor) { return $this->searchDepthPrev($current, $anchor, $depth, $ignoreRole, $class); @@ -698,15 +680,13 @@ public function getPrev($current, $anchor = null, ?int $depth = null, ?bool $ign /** * Gets the next document. * - * @param string|object $current document instance or path from which to search - * @param string|object|null $anchor document instance or path which serves as an anchor from which to flatten the hierarchy + * @param object|string|null $current document instance or path from which to search + * @param object|string|null $anchor document instance or path which serves as an anchor from which to flatten the hierarchy * @param int|null $depth depth up to which to traverse down the tree when an anchor is provided * @param bool $ignoreRole if to ignore the role * @param string|null $class the class to filter by - * - * @return object|null */ - public function getNext($current, $anchor = null, ?int $depth = null, ?bool $ignoreRole = false, ?string $class = null): object|string|null + public function getNext(object|string|null $current, object|string|null $anchor = null, ?int $depth = null, ?bool $ignoreRole = false, ?string $class = null): object|null { if ($anchor) { return $this->searchDepthNext($current, $anchor, $depth, $ignoreRole, $class); @@ -720,9 +700,9 @@ public function getNext($current, $anchor = null, ?int $depth = null, ?bool $ign * * This has the same semantics as the isLinkable method. * - * @param string|object $current Document instance or path from + * @param object|string|null $current Document instance or path from * which to search - * @param string|object|null $anchor Document instance or path which + * @param object|string|null $anchor Document instance or path which * serves as an anchor from which to * flatten the hierarchy * @param int|null $depth Depth up to which to traverse down @@ -730,11 +710,9 @@ public function getNext($current, $anchor = null, ?int $depth = null, ?bool $ign * provided * @param bool $ignoreRole Whether to ignore the role, * - * @return object|null - * * @see isLinkable */ - public function getPrevLinkable($current, $anchor = null, ?int $depth = null, ?bool $ignoreRole = false): object|string|null + public function getPrevLinkable(object|string|null $current, object|string $anchor = null, ?int $depth = null, ?bool $ignoreRole = false): object|null { while ($candidate = $this->getPrev($current, $anchor, $depth, $ignoreRole)) { if ($this->isLinkable($candidate)) { @@ -752,9 +730,9 @@ public function getPrevLinkable($current, $anchor = null, ?int $depth = null, ?b * * This has the same semantics as the isLinkable method. * - * @param string|object $current Document instance or path from + * @param object|string|null $current Document instance or path from * which to search - * @param string|object|null $anchor Document instance or path which + * @param object|string|null $anchor Document instance or path which * serves as an anchor from which to * flatten the hierarchy * @param int|null $depth Depth up to which to traverse down @@ -762,11 +740,9 @@ public function getPrevLinkable($current, $anchor = null, ?int $depth = null, ?b * provided * @param bool $ignoreRole Whether to ignore the role * - * @return object|null - * * @see isLinkable */ - public function getNextLinkable($current, $anchor = null, ?int $depth = null, ?bool $ignoreRole = false): object|string|null + public function getNextLinkable(object|string|null $current, object|string|null $anchor = null, ?int $depth = null, ?bool $ignoreRole = false): object|null { while ($candidate = $this->getNext($current, $anchor, $depth, $ignoreRole)) { if ($this->isLinkable($candidate)) { diff --git a/src/Translatable/TranslatableInterface.php b/src/Translatable/TranslatableInterface.php index f3de324..c457991 100644 --- a/src/Translatable/TranslatableInterface.php +++ b/src/Translatable/TranslatableInterface.php @@ -23,11 +23,11 @@ interface TranslatableInterface * @return string|bool The locale of this model or false if * translations are disabled in this project */ - public function getLocale(); + public function getLocale(): bool|string; /** - * @param string|bool $locale The local for this model, or false if + * @param bool|string $locale The local for this model, or false if * translations are disabled in this project */ - public function setLocale($locale); + public function setLocale(bool|string $locale): void; } diff --git a/tests/Functional/DependencyInjection/CmfCoreExtensionTest.php b/tests/Functional/DependencyInjection/CmfCoreExtensionTest.php index 8117015..2b9b7b9 100644 --- a/tests/Functional/DependencyInjection/CmfCoreExtensionTest.php +++ b/tests/Functional/DependencyInjection/CmfCoreExtensionTest.php @@ -18,10 +18,7 @@ class CmfCoreExtensionTest extends TestCase { - /** - * @var ContainerBuilder - */ - private $container; + private ContainerBuilder $container; protected function setUp(): void { diff --git a/tests/Functional/Templating/Helper/CmfHierarchyTest.php b/tests/Functional/Templating/Helper/CmfHierarchyTest.php index 1c24d26..1f96e0a 100644 --- a/tests/Functional/Templating/Helper/CmfHierarchyTest.php +++ b/tests/Functional/Templating/Helper/CmfHierarchyTest.php @@ -21,15 +21,8 @@ class CmfHierarchyTest extends BaseTestCase { - /** - * @var AuthorizationCheckerInterface|MockObject - */ - private $publishWorkflowChecker; - - /** - * @var Cmf - */ - private $helper; + private MockObject&AuthorizationCheckerInterface $publishWorkflowChecker; + private Cmf $helper; public function setUp(): void { @@ -39,14 +32,14 @@ public function setUp(): void $this->publishWorkflowChecker = $this->createMock(AuthorizationCheckerInterface::class); $this->publishWorkflowChecker ->method('isGranted') - ->will($this->returnValue(true)) + ->willReturn(true) ; $this->helper = new Cmf($this->publishWorkflowChecker); $this->helper->setDoctrineRegistry($dbManager->getRegistry(), 'default'); } - public function testGetDescendants() + public function testGetDescendants(): void { $this->assertEquals([], $this->helper->getDescendants(null)); @@ -60,7 +53,7 @@ public function testGetDescendants() /** * @dataProvider getPrevData */ - public function testGetPrev($expected, $path, $anchor = null, $depth = null, $class = 'Doctrine\ODM\PHPCR\Document\Generic') + public function testGetPrev(?string $expected, ?string $path, ?string $anchor = null, ?int $depth = null, string $class = Generic::class): void { $prev = $this->helper->getPrev($path, $anchor, $depth); if (null === $expected) { @@ -71,7 +64,7 @@ public function testGetPrev($expected, $path, $anchor = null, $depth = null, $cl } } - public static function getPrevData() + public static function getPrevData(): array { return [ [null, null], @@ -99,7 +92,7 @@ public static function getPrevData() /** * @dataProvider getNextData */ - public function testGetNext($expected, $path, $anchor = null, $depth = null, $class = Generic::class) + public function testGetNext(?string $expected, ?string $path, ?string $anchor = null, ?int $depth = null, string $class = Generic::class): void { $next = $this->helper->getNext($path, $anchor, $depth); if (null === $expected) { @@ -110,7 +103,7 @@ public function testGetNext($expected, $path, $anchor = null, $depth = null, $cl } } - public static function getNextData() + public static function getNextData(): array { return [ [null, null], @@ -139,7 +132,7 @@ public static function getNextData() /** * @dataProvider getPrevLinkableData */ - public function testGetPrevLinkable($expected, $path, $anchor = null, $depth = null) + public function testGetPrevLinkable(?string $expected, ?string $path, ?string $anchor = null, ?int $depth = null): void { $prev = $this->helper->getPrevLinkable($path, $anchor, $depth); if (null === $expected) { @@ -150,7 +143,7 @@ public function testGetPrevLinkable($expected, $path, $anchor = null, $depth = n } } - public static function getPrevLinkableData() + public static function getPrevLinkableData(): array { // TODO: expand test case return [ @@ -164,7 +157,7 @@ public static function getPrevLinkableData() /** * @dataProvider getNextLinkableData */ - public function testGetNextLinkable($expected, $path, $anchor = null, $depth = null) + public function testGetNextLinkable(?string $expected, ?string $path, ?string $anchor = null, ?int $depth = null): void { $next = $this->helper->getNextLinkable($path, $anchor, $depth); if (null === $expected) { @@ -175,7 +168,7 @@ public function testGetNextLinkable($expected, $path, $anchor = null, $depth = n } } - public static function getNextLinkableData() + public static function getNextLinkableData(): array { // TODO: expand test case return [ diff --git a/tests/Functional/Twig/ServiceTest.php b/tests/Functional/Twig/ServiceTest.php index 4a0a7f4..23998cc 100644 --- a/tests/Functional/Twig/ServiceTest.php +++ b/tests/Functional/Twig/ServiceTest.php @@ -16,7 +16,7 @@ class ServiceTest extends BaseTestCase { - public function testContainer() + public function testContainer(): void { /** @var \Twig\Environment $twig */ $twig = $this->getContainer()->get('test.service_container')->get('twig'); diff --git a/tests/Unit/Twig/Extension/CmfExtensionTest.php b/tests/Unit/Twig/Extension/CmfExtensionTest.php index 7743f8a..72bdf34 100644 --- a/tests/Unit/Twig/Extension/CmfExtensionTest.php +++ b/tests/Unit/Twig/Extension/CmfExtensionTest.php @@ -15,13 +15,10 @@ use PHPUnit\Framework\TestCase; use Symfony\Cmf\Bundle\CoreBundle\Templating\Helper\Cmf; use Symfony\Cmf\Bundle\CoreBundle\Twig\Extension\CmfExtension; -use Twig\Environment; -use Twig\Loader\ArrayLoader; class CmfExtensionTest extends TestCase { private Cmf&MockObject $cmfHelper; - private Environment $env; private CmfExtension $cmfExtension; public function setUp(): void @@ -29,8 +26,6 @@ public function setUp(): void $this->cmfHelper = $this->createMock(Cmf::class); $this->cmfExtension = new CmfExtension($this->cmfHelper); - $this->env = new Environment(new ArrayLoader([])); - $this->env->addExtension($this->cmfExtension); } /** @@ -54,7 +49,7 @@ public function testFunctions($methodName, array $methodArguments, $helperMethod public function getFunctionsData(): array { return [ - ['isPublished', ['document1']], + ['isPublished', [$this]], ['isLinkable', ['document1']], ['getChild', ['parent', 'name']], ['getChildren', ['parent', true], 'getChildren', ['parent', true, false, null, false, null]],