From 32281f12c7734a6e7d7cf48165845861f2a373ca Mon Sep 17 00:00:00 2001 From: Vincent Langlet Date: Sat, 24 Jul 2021 17:21:45 +0200 Subject: [PATCH] Add strict rules (#7340) * Add strict rules * Fix last issue * Add message --- composer.json | 1 + phpstan-baseline.neon | 15 +++++++++ src/Admin/AbstractAdmin.php | 20 +++++++----- src/BCLayer/BCDeprecationParameters.php | 1 + src/BCLayer/BCUserInterface.php | 1 + src/Command/GenerateObjectAclCommand.php | 11 ++----- src/Controller/CRUDController.php | 10 ++++-- src/DependencyInjection/Configuration.php | 2 -- .../Persister/SessionFilterPersister.php | 1 + .../BooleanToStringTransformer.php | 2 +- src/Menu/Matcher/Voter/AdminVoter.php | 1 + src/Menu/Provider/GroupMenuProvider.php | 32 ++++++++++++------- src/Route/AdminPoolLoader.php | 8 +++-- src/Security/Handler/AclSecurityHandler.php | 7 ++-- tests/Admin/AdminTest.php | 2 +- .../Command/GenerateObjectAclCommandTest.php | 29 ----------------- tests/Controller/CRUDControllerTest.php | 6 ++-- tests/Datagrid/DatagridTest.php | 14 +++----- tests/Route/RouteCollectionTest.php | 4 +-- 19 files changed, 82 insertions(+), 85 deletions(-) diff --git a/composer.json b/composer.json index 68f69516f0..a62a60e5ac 100644 --- a/composer.json +++ b/composer.json @@ -79,6 +79,7 @@ "phpstan/extension-installer": "^1.0", "phpstan/phpstan": "^0.12.84", "phpstan/phpstan-phpunit": "^0.12.18", + "phpstan/phpstan-strict-rules": "^0.12.10", "phpstan/phpstan-symfony": "^0.12.21", "psalm/plugin-phpunit": "^0.15.1", "psalm/plugin-symfony": "^2.0", diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 42e70ab5d9..609acae0b6 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -1,8 +1,14 @@ parameters: ignoreErrors: + - # Disallow VariableMethodCallRule and VariablePropertyFetchRule + message: '#^Variable (method call|property access)#' + path: . - # https://github.com/phpstan/phpstan-phpunit/issues/87 message: '#^Trying to mock an undefined method [a-zA-Z]*\(\) on class stdClass\.$#' path: tests/ + - # https://github.com/phpstan/phpstan-strict-rules/issues/130 + message: '#^Call to static method PHPUnit\\Framework\\Assert::.* will always evaluate to true\.$#' + path: tests/ - # BC for Symfony 4 since InputBag was introduce in Symfony 5 message: '#^Property Symfony\\Component\\HttpFoundation\\Request\:\:\$query \(Symfony\\Component\\HttpFoundation\\InputBag\) does not accept Symfony\\Component\\HttpFoundation\\ParameterBag\.$#' path: tests/Admin/AdminTest.php @@ -18,3 +24,12 @@ parameters: - # PHPStan doesn't solve type of Maker argument/option https://github.com/phpstan/phpstan-symfony/issues/167 message: '#array\|string\|true(\sgiven)?.$#' path: src/Maker/AdminMaker.php + - # `treatPhpdocAsCertain: false` should not report this https://github.com/phpstan/phpstan/issues/5333 + message: '#^Instanceof between Symfony\\Component\\Routing\\Route and Symfony\\Component\\Routing\\Route will always evaluate to true\.$#' + path: src/Route/RouteCollection.php + - # DataTransformer contravariance is not really mandatory + message: '#(reverseTransform|transform)\(\) should be contravariant#' + path: src/Form/DataTransformer + - # Generic with intersection is buggy https://github.com/phpstan/phpstan/issues/5336 + message: '#Sonata\\AdminBundle\\Datagrid\\(Datagrid|PagerInterface)#' + path: tests/Datagrid/DatagridTest.php diff --git a/src/Admin/AbstractAdmin.php b/src/Admin/AbstractAdmin.php index dfcbfe0839..cabf8531a0 100644 --- a/src/Admin/AbstractAdmin.php +++ b/src/Admin/AbstractAdmin.php @@ -2264,15 +2264,15 @@ protected function configureDefaultSortValues(array &$sortValues): void */ final protected function appendParentObject(object $object): void { - if ($this->isChild() && null !== $this->getParentAssociationMapping()) { - $parentAdmin = $this->getParent(); - $parentObject = $parentAdmin->getObject($this->getRequest()->get($parentAdmin->getIdParameter())); + if ($this->isChild()) { + $parentAssociationMapping = $this->getParentAssociationMapping(); - if (null !== $parentObject) { - $propertyAccessor = PropertyAccess::createPropertyAccessor(); - $parentAssociationMapping = $this->getParentAssociationMapping(); + if (null !== $parentAssociationMapping) { + $parentAdmin = $this->getParent(); + $parentObject = $parentAdmin->getObject($this->getRequest()->get($parentAdmin->getIdParameter())); - if (null !== $parentAssociationMapping) { + if (null !== $parentObject) { + $propertyAccessor = PropertyAccess::createPropertyAccessor(); $value = $propertyAccessor->getValue($object, $parentAssociationMapping); if (\is_array($value) || $value instanceof \ArrayAccess) { @@ -2282,8 +2282,12 @@ final protected function appendParentObject(object $object): void $propertyAccessor->setValue($object, $parentAssociationMapping, $parentObject); } } + + return; } - } elseif ($this->hasParentFieldDescription()) { + } + + if ($this->hasParentFieldDescription()) { $parentAdmin = $this->getParentFieldDescription()->getAdmin(); $parentObject = $parentAdmin->getObject($this->getRequest()->get($parentAdmin->getIdParameter())); diff --git a/src/BCLayer/BCDeprecationParameters.php b/src/BCLayer/BCDeprecationParameters.php index 406e773291..43c7284709 100644 --- a/src/BCLayer/BCDeprecationParameters.php +++ b/src/BCLayer/BCDeprecationParameters.php @@ -28,6 +28,7 @@ final class BCDeprecationParameters */ public static function forConfig(string $message, string $version): array { + // @phpstan-ignore-next-line if (method_exists(BaseNode::class, 'getDeprecation')) { return [ 'sonata-project/admin-bundle', diff --git a/src/BCLayer/BCUserInterface.php b/src/BCLayer/BCUserInterface.php index 7bab502bb6..c12af005fb 100644 --- a/src/BCLayer/BCUserInterface.php +++ b/src/BCLayer/BCUserInterface.php @@ -28,6 +28,7 @@ final class BCUserInterface */ public static function getUsername(UserInterface $user): string { + // @phpstan-ignore-next-line if (method_exists($user, 'getUserIdentifier')) { return $user->getUserIdentifier(); } diff --git a/src/Command/GenerateObjectAclCommand.php b/src/Command/GenerateObjectAclCommand.php index 479f3883d8..736f11bf8d 100644 --- a/src/Command/GenerateObjectAclCommand.php +++ b/src/Command/GenerateObjectAclCommand.php @@ -117,7 +117,7 @@ public function execute(InputInterface $input, OutputInterface $output): int } $objectOwner = $input->getOption('object_owner'); - if (!$input->getOption('step') && $objectOwner) { + if (!$input->getOption('step') && null !== $objectOwner) { $securityIdentity = new UserSecurityIdentity($objectOwner, $this->getUserModelClass($input, $output)); } @@ -128,14 +128,7 @@ public function execute(InputInterface $input, OutputInterface $output): int continue; } - $manipulator = $this->aclObjectManipulators[$manipulatorId]; - if (!$manipulator instanceof ObjectAclManipulatorInterface) { - $output->writeln(sprintf('The interface "ObjectAclManipulatorInterface" is not implemented for %s: ignoring', \get_class($manipulator))); - - continue; - } - - $manipulator->batchConfigureAcls($output, $admin, $securityIdentity); + $this->aclObjectManipulators[$manipulatorId]->batchConfigureAcls($output, $admin, $securityIdentity); } return 0; diff --git a/src/Controller/CRUDController.php b/src/Controller/CRUDController.php index 0682ee8f59..438ac593c0 100644 --- a/src/Controller/CRUDController.php +++ b/src/Controller/CRUDController.php @@ -431,7 +431,7 @@ public function batchAction(Request $request): Response $askConfirmation = $batchActions[$action]['ask_confirmation'] ?? true; - if ($askConfirmation && 'ok' !== $confirmation) { + if (true === $askConfirmation && 'ok' !== $confirmation) { $actionLabel = $batchActions[$action]['label']; $batchTranslationDomain = $batchActions[$action]['translation_domain'] ?? $this->admin->getTranslationDomain(); @@ -949,7 +949,9 @@ final protected function renderJson($data, int $status = Response::HTTP_OK, arra */ final protected function isXmlHttpRequest(Request $request): bool { - return $request->isXmlHttpRequest() || $request->get('_xml_http_request'); + return $request->isXmlHttpRequest() + || $request->request->getBoolean('_xml_http_request') + || $request->query->getBoolean('_xml_http_request'); } /** @@ -992,7 +994,9 @@ protected function getBaseTemplate(): string */ protected function handleModelManagerException(\Exception $exception): void { - if ($this->getParameter('kernel.debug')) { + $debug = $this->getParameter('kernel.debug'); + \assert(\is_bool($debug)); + if ($debug) { throw $exception; } diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index be9aaaf979..0f5579966c 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -13,7 +13,6 @@ namespace Sonata\AdminBundle\DependencyInjection; -use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition; use Symfony\Component\Config\Definition\Builder\TreeBuilder; use Symfony\Component\Config\Definition\ConfigurationInterface; @@ -36,7 +35,6 @@ public function getConfigTreeBuilder(): TreeBuilder { $treeBuilder = new TreeBuilder('sonata_admin'); $rootNode = $treeBuilder->getRootNode(); - \assert($rootNode instanceof ArrayNodeDefinition); $rootNode ->fixXmlConfig('option') diff --git a/src/Filter/Persister/SessionFilterPersister.php b/src/Filter/Persister/SessionFilterPersister.php index ca08387b54..c19d2d70ff 100644 --- a/src/Filter/Persister/SessionFilterPersister.php +++ b/src/Filter/Persister/SessionFilterPersister.php @@ -65,6 +65,7 @@ private function buildStorageKey(string $adminCode): string */ private function getSession(): SessionInterface { + // @phpstan-ignore-next-line if (method_exists($this->requestStack, 'getSession')) { return $this->requestStack->getSession(); } diff --git a/src/Form/DataTransformer/BooleanToStringTransformer.php b/src/Form/DataTransformer/BooleanToStringTransformer.php index d200165231..bc00c2ffe3 100644 --- a/src/Form/DataTransformer/BooleanToStringTransformer.php +++ b/src/Form/DataTransformer/BooleanToStringTransformer.php @@ -38,7 +38,7 @@ public function __construct(string $trueValue) */ public function transform($value): ?string { - return $value ? $this->trueValue : null; + return true === $value ? $this->trueValue : null; } /** diff --git a/src/Menu/Matcher/Voter/AdminVoter.php b/src/Menu/Matcher/Voter/AdminVoter.php index e8e85db05a..1ef4f5f539 100644 --- a/src/Menu/Matcher/Voter/AdminVoter.php +++ b/src/Menu/Matcher/Voter/AdminVoter.php @@ -75,6 +75,7 @@ public function matchItem(ItemInterface $item): ?bool */ private function getMainRequest(): ?Request { + // @phpstan-ignore-next-line if (method_exists($this->requestStack, 'getMainRequest')) { return $this->requestStack->getMainRequest(); // symfony 5.3+ } diff --git a/src/Menu/Provider/GroupMenuProvider.php b/src/Menu/Provider/GroupMenuProvider.php index 607cbab847..e4d773c9b5 100644 --- a/src/Menu/Provider/GroupMenuProvider.php +++ b/src/Menu/Provider/GroupMenuProvider.php @@ -55,18 +55,24 @@ public function __construct(FactoryInterface $menuFactory, Pool $pool, Authoriza /** * Retrieves the menu based on the group options. * - * @param mixed[] $options + * @param array $options * * @throws \InvalidArgumentException if the menu does not exists */ public function get(string $name, array $options = []): ItemInterface { + if (!isset($options['name'])) { + throw new \InvalidArgumentException('The option "name" is required.'); + } + $menuItem = $this->menuFactory->createItem($options['name']); + + if (!isset($options['group'])) { + throw new \InvalidArgumentException('The option "group" is required.'); + } /** @phpstan-var Group $group */ $group = $options['group']; - $menuItem = $this->menuFactory->createItem($options['name']); - - if (!\array_key_exists('on_top', $group) || false === $group['on_top']) { + if (false === $group['on_top']) { foreach ($group['items'] as $item) { if ($this->canGenerateMenuItem($item, $group)) { $menuItem->addChild($this->generateMenuItem($item, $group)); @@ -75,18 +81,20 @@ public function get(string $name, array $options = []): ItemInterface if (false === $menuItem->hasChildren()) { $menuItem->setDisplay(false); - } elseif ($group['keep_open'] ?? false) { + } elseif ($group['keep_open']) { $menuItem->setAttribute('class', 'keep-open'); $menuItem->setExtra('keep_open', $group['keep_open']); } - } elseif (1 === \count($group['items'])) { - if ($this->canGenerateMenuItem($group['items'][0], $group)) { - $menuItem = $this->generateMenuItem($group['items'][0], $group); - $menuItem->setExtra('on_top', $group['on_top']); - } else { - $menuItem->setDisplay(false); - } + } elseif ( + 1 === \count($group['items']) + && $this->canGenerateMenuItem($group['items'][0], $group) + ) { + $menuItem = $this->generateMenuItem($group['items'][0], $group); + $menuItem->setExtra('on_top', $group['on_top']); + } else { + $menuItem->setDisplay(false); } + $menuItem->setLabel($group['label']); return $menuItem; diff --git a/src/Route/AdminPoolLoader.php b/src/Route/AdminPoolLoader.php index bd45375d7c..48dd62911f 100644 --- a/src/Route/AdminPoolLoader.php +++ b/src/Route/AdminPoolLoader.php @@ -20,8 +20,6 @@ /** * @author Thomas Rabaix - * - * @psalm-suppress PropertyNotSetInConstructor add `parent::__construct()` call when dropping support of symfony/symfony-config < 5.3. */ final class AdminPoolLoader extends Loader { @@ -34,6 +32,12 @@ final class AdminPoolLoader extends Loader public function __construct(Pool $pool) { + // Remove this check when dropping support for support of symfony/symfony-config < 5.3. + // @phpstan-ignore-next-line + if (method_exists(parent::class, '__construct')) { + parent::__construct(); + } + $this->pool = $pool; } diff --git a/src/Security/Handler/AclSecurityHandler.php b/src/Security/Handler/AclSecurityHandler.php index 7345ca0aac..4af9cfb259 100644 --- a/src/Security/Handler/AclSecurityHandler.php +++ b/src/Security/Handler/AclSecurityHandler.php @@ -168,7 +168,6 @@ public function getObjectAcl(ObjectIdentityInterface $objectIdentity): ?MutableA { try { $acl = $this->aclProvider->findAcl($objectIdentity); - \assert($acl instanceof MutableAclInterface); } catch (AclNotFoundException $e) { return null; } @@ -248,7 +247,8 @@ public function deleteAcl(ObjectIdentityInterface $objectIdentity): void public function findClassAceIndexByRole(MutableAclInterface $acl, string $role) { foreach ($acl->getClassAces() as $index => $entry) { - if ($entry->getSecurityIdentity() instanceof RoleSecurityIdentity && $entry->getSecurityIdentity()->getRole() === $role) { + $securityIdentity = $entry->getSecurityIdentity(); + if ($securityIdentity instanceof RoleSecurityIdentity && $securityIdentity->getRole() === $role) { return $index; } } @@ -259,7 +259,8 @@ public function findClassAceIndexByRole(MutableAclInterface $acl, string $role) public function findClassAceIndexByUsername(MutableAclInterface $acl, string $username) { foreach ($acl->getClassAces() as $index => $entry) { - if ($entry->getSecurityIdentity() instanceof UserSecurityIdentity && $entry->getSecurityIdentity()->getUsername() === $username) { + $securityIdentity = $entry->getSecurityIdentity(); + if ($securityIdentity instanceof UserSecurityIdentity && $securityIdentity->getUsername() === $username) { return $index; } } diff --git a/tests/Admin/AdminTest.php b/tests/Admin/AdminTest.php index 7cdc920613..9c4ce9c148 100644 --- a/tests/Admin/AdminTest.php +++ b/tests/Admin/AdminTest.php @@ -1611,7 +1611,7 @@ public function testGetFilterFieldDescription(): void $datagridBuilder = $this->createMock(DatagridBuilderInterface::class); $datagridBuilder->expects(self::once()) ->method('getBaseDatagrid') - ->with($this->identicalTo($modelAdmin)) + ->with(self::identicalTo($modelAdmin)) ->willReturn($datagrid); $datagridBuilder->expects(self::exactly(3)) diff --git a/tests/Command/GenerateObjectAclCommandTest.php b/tests/Command/GenerateObjectAclCommandTest.php index 1b71314719..2e371ef38f 100644 --- a/tests/Command/GenerateObjectAclCommandTest.php +++ b/tests/Command/GenerateObjectAclCommandTest.php @@ -99,35 +99,6 @@ public function testExecuteWithManipulatorNotFound(): void self::assertMatchesRegularExpression('/Admin class is using a manager type that has no manipulator implemented : ignoring/', $commandTester->getDisplay()); } - /** - * @psalm-suppress InvalidArgument - */ - public function testExecuteWithManipulatorNotObjectAclManipulatorInterface(): void - { - $admin = $this->createStub(AbstractAdmin::class); - $container = new Container(); - $container->set('acme.admin.foo', $admin); - $pool = new Pool($container, ['acme.admin.foo']); - - $admin->setManagerType('bar'); - - $aclObjectManipulators = [ - 'sonata.admin.manipulator.acl.object.bar' => new \stdClass(), - ]; - - // @phpstan-ignore-next-line - $command = new GenerateObjectAclCommand($pool, $aclObjectManipulators); - - $application = new Application(); - $application->add($command); - - $command = $application->find('sonata:admin:generate-object-acl'); - $commandTester = new CommandTester($command); - $commandTester->execute(['command' => $command->getName()]); - - self::assertMatchesRegularExpression('/The interface "ObjectAclManipulatorInterface" is not implemented for/', $commandTester->getDisplay()); - } - public function testExecuteWithManipulator(): void { $admin = $this->createStub(AbstractAdmin::class); diff --git a/tests/Controller/CRUDControllerTest.php b/tests/Controller/CRUDControllerTest.php index 38c2eaec1a..b2703189a2 100644 --- a/tests/Controller/CRUDControllerTest.php +++ b/tests/Controller/CRUDControllerTest.php @@ -350,7 +350,7 @@ public function testRenderJsonAjax(): void { $data = ['example' => '123', 'foo' => 'bar']; - $this->request->attributes->set('_xml_http_request', true); + $this->request->query->set('_xml_http_request', true); $this->request->headers->set('Content-Type', 'multipart/form-data'); $response = $this->protectedTestedMethods['renderJson']->invoke($this->controller, $data, 200, [], $this->request); @@ -369,7 +369,7 @@ public function testIsXmlHttpRequest(): void $this->request->headers->remove('X-Requested-With'); self::assertFalse($this->protectedTestedMethods['isXmlHttpRequest']->invoke($this->controller, $this->request)); - $this->request->attributes->set('_xml_http_request', true); + $this->request->query->set('_xml_http_request', true); self::assertTrue($this->protectedTestedMethods['isXmlHttpRequest']->invoke($this->controller, $this->request)); } @@ -422,7 +422,7 @@ public function testGetBaseTemplate(): void $this->protectedTestedMethods['getBaseTemplate']->invoke($this->controller) ); - $this->request->attributes->set('_xml_http_request', true); + $this->request->request->set('_xml_http_request', true); self::assertSame( '@SonataAdmin/ajax_layout.html.twig', $this->protectedTestedMethods['getBaseTemplate']->invoke($this->controller) diff --git a/tests/Datagrid/DatagridTest.php b/tests/Datagrid/DatagridTest.php index 8651651c87..680d27489d 100644 --- a/tests/Datagrid/DatagridTest.php +++ b/tests/Datagrid/DatagridTest.php @@ -14,6 +14,7 @@ namespace Sonata\AdminBundle\Tests\Datagrid; use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\MockObject\Stub; use PHPUnit\Framework\TestCase; use Sonata\AdminBundle\Datagrid\Datagrid; use Sonata\AdminBundle\Datagrid\DatagridInterface; @@ -35,17 +36,17 @@ final class DatagridTest extends TestCase { /** - * @var Datagrid + * @var Datagrid */ private $datagrid; /** - * @var PagerInterface&MockObject + * @var PagerInterface&MockObject */ private $pager; /** - * @var ProxyQueryInterface + * @var ProxyQueryInterface&Stub */ private $query; @@ -62,15 +63,10 @@ final class DatagridTest extends TestCase protected function setUp(): void { $this->query = $this->createStub(ProxyQueryInterface::class); - \assert($this->query instanceof ProxyQueryInterface); // https://github.com/vimeo/psalm/issues/5818 - $this->columns = new FieldDescriptionCollection(); $this->pager = $this->createMock(PagerInterface::class); $this->formBuilder = Forms::createFormFactoryBuilder()->getFormFactory()->createBuilder(); - - $values = []; - - $this->datagrid = new Datagrid($this->query, $this->columns, $this->pager, $this->formBuilder, $values); + $this->datagrid = new Datagrid($this->query, $this->columns, $this->pager, $this->formBuilder, []); } public function testGetPager(): void diff --git a/tests/Route/RouteCollectionTest.php b/tests/Route/RouteCollectionTest.php index 9583f11b2e..442b2ee3c0 100644 --- a/tests/Route/RouteCollectionTest.php +++ b/tests/Route/RouteCollectionTest.php @@ -179,9 +179,7 @@ public function testRouteWithAllConstructorParameters(): void self::assertArrayHasKey('debug', $route->getOptions()); self::assertSame($host, $route->getHost()); self::assertSame($methods, $route->getMethods()); - if (method_exists($route, 'getCondition')) { - self::assertSame($condition, $route->getCondition()); - } + self::assertSame($condition, $route->getCondition()); } public function testRouteControllerService(): void