From 2ee2ae1f5efc42df14fc961d70e4127dd6f814ba Mon Sep 17 00:00:00 2001 From: Farhad Safarov Date: Wed, 5 Jun 2024 17:43:24 +0300 Subject: [PATCH] Fix & improve tests (#342) --- composer.json | 4 +- src/Plugin.php | 40 +++++------ .../PropertyAccessorInterface.stubphp | 8 +-- .../Core/Authorization/Voter/Voter.stubphp | 42 ----------- .../acceptance/ContainerService.feature | 47 ++++++------- .../acceptance/ContainerXml.feature | 51 ++++++-------- tests/acceptance/acceptance/HeaderBag.feature | 25 +++---- .../acceptance/NamingConventions.feature | 69 +++++++------------ .../PropertyAccessorInterface.feature | 30 ++++---- tests/acceptance/acceptance/Voter.feature | 40 ----------- .../acceptance/forms/AbstractType.feature | 2 +- .../forms/AbstractTypeExtension.feature | 2 +- .../forms/FormBuilterInterface.feature | 2 +- .../forms/FormConfigBuilterInterface.feature | 2 +- .../forms/FormConfigInterface.feature | 2 +- .../acceptance/forms/FormView.feature | 2 +- 16 files changed, 126 insertions(+), 242 deletions(-) delete mode 100644 src/Stubs/common/Component/Security/Core/Authorization/Voter/Voter.stubphp delete mode 100644 tests/acceptance/acceptance/Voter.feature diff --git a/composer.json b/composer.json index d30e15d9..9b77764b 100644 --- a/composer.json +++ b/composer.json @@ -16,14 +16,14 @@ "vimeo/psalm": "^5.24" }, "require-dev": { - "symfony/form": "^5.0 || ^6.0 || ^7.0", "doctrine/annotations": "^1.8|^2", "doctrine/orm": "^2.9", "phpunit/phpunit": "~7.5 || ~9.5", "symfony/cache-contracts": "^1.0 || ^2.0", "symfony/console": "*", + "symfony/form": "^5.0 || ^6.0 || ^7.0", "symfony/messenger": "^5.0 || ^6.0 || ^7.0", - "symfony/security-guard": "*", + "symfony/security-core": "*", "symfony/serializer": "^5.0 || ^6.0 || ^7.0", "symfony/validator": "*", "twig/twig": "^2.10 || ^3.0", diff --git a/src/Plugin.php b/src/Plugin.php index d329c741..ce9c5431 100644 --- a/src/Plugin.php +++ b/src/Plugin.php @@ -29,7 +29,7 @@ */ class Plugin implements PluginEntryPointInterface { - public function __invoke(RegistrationInterface $api, ?\SimpleXMLElement $config = null): void + public function __invoke(RegistrationInterface $registration, ?\SimpleXMLElement $config = null): void { require_once __DIR__.'/Handler/HeaderBagHandler.php'; require_once __DIR__.'/Handler/ContainerHandler.php'; @@ -39,13 +39,13 @@ public function __invoke(RegistrationInterface $api, ?\SimpleXMLElement $config require_once __DIR__.'/Handler/DoctrineQueryBuilderHandler.php'; require_once __DIR__.'/Provider/FormGetErrorsReturnTypeProvider.php'; - $api->registerHooksFromClass(HeaderBagHandler::class); - $api->registerHooksFromClass(ConsoleHandler::class); - $api->registerHooksFromClass(ContainerDependencyHandler::class); - $api->registerHooksFromClass(RequiredSetterHandler::class); + $registration->registerHooksFromClass(HeaderBagHandler::class); + $registration->registerHooksFromClass(ConsoleHandler::class); + $registration->registerHooksFromClass(ContainerDependencyHandler::class); + $registration->registerHooksFromClass(RequiredSetterHandler::class); if (class_exists(\Doctrine\ORM\QueryBuilder::class)) { - $api->registerHooksFromClass(DoctrineQueryBuilderHandler::class); + $registration->registerHooksFromClass(DoctrineQueryBuilderHandler::class); } if (class_exists(AnnotationRegistry::class)) { @@ -54,10 +54,10 @@ public function __invoke(RegistrationInterface $api, ?\SimpleXMLElement $config /** @psalm-suppress DeprecatedMethod */ AnnotationRegistry::registerLoader('class_exists'); } - $api->registerHooksFromClass(DoctrineRepositoryHandler::class); + $registration->registerHooksFromClass(DoctrineRepositoryHandler::class); require_once __DIR__.'/Handler/AnnotationHandler.php'; - $api->registerHooksFromClass(AnnotationHandler::class); + $registration->registerHooksFromClass(AnnotationHandler::class); } if (isset($config->containerXml)) { @@ -83,14 +83,14 @@ public function __invoke(RegistrationInterface $api, ?\SimpleXMLElement $config require_once __DIR__.'/Handler/ParameterBagHandler.php'; ParameterBagHandler::init($containerMeta); - $api->registerHooksFromClass(ParameterBagHandler::class); + $registration->registerHooksFromClass(ParameterBagHandler::class); } - $api->registerHooksFromClass(ContainerHandler::class); + $registration->registerHooksFromClass(ContainerHandler::class); - $this->addStubs($api, __DIR__.'/Stubs/common'); - $this->addStubs($api, __DIR__.'/Stubs/'.Kernel::MAJOR_VERSION); - $this->addStubs($api, __DIR__.'/Stubs/php'); + $this->addStubs($registration, __DIR__.'/Stubs/common'); + $this->addStubs($registration, __DIR__.'/Stubs/'.Kernel::MAJOR_VERSION); + $this->addStubs($registration, __DIR__.'/Stubs/php'); if (isset($config->twigCachePath)) { $twig_cache_path = getcwd().DIRECTORY_SEPARATOR.ltrim((string) $config->twigCachePath, DIRECTORY_SEPARATOR); @@ -99,15 +99,15 @@ public function __invoke(RegistrationInterface $api, ?\SimpleXMLElement $config } require_once __DIR__.'/Twig/CachedTemplatesTainter.php'; - $api->registerHooksFromClass(CachedTemplatesTainter::class); + $registration->registerHooksFromClass(CachedTemplatesTainter::class); require_once __DIR__.'/Twig/CachedTemplatesMapping.php'; - $api->registerHooksFromClass(CachedTemplatesMapping::class); + $registration->registerHooksFromClass(CachedTemplatesMapping::class); CachedTemplatesMapping::setCachePath($twig_cache_path); } require_once __DIR__.'/Twig/AnalyzedTemplatesTainter.php'; - $api->registerHooksFromClass(AnalyzedTemplatesTainter::class); + $registration->registerHooksFromClass(AnalyzedTemplatesTainter::class); if (isset($config->twigRootPath)) { $twig_root_path = trim((string) $config->twigRootPath, DIRECTORY_SEPARATOR); @@ -119,20 +119,20 @@ public function __invoke(RegistrationInterface $api, ?\SimpleXMLElement $config TemplateFileAnalyzer::setTemplateRootPath($twig_root_path); } - $api->registerHooksFromClass(FormGetErrorsReturnTypeProvider::class); + $registration->registerHooksFromClass(FormGetErrorsReturnTypeProvider::class); } - private function addStubs(RegistrationInterface $api, string $path): void + private function addStubs(RegistrationInterface $registration, string $path): void { if (!is_dir($path)) { - // e.g. looking for stubs for version 3, but there aren't any at time of writing, so don't try and load them. + // e.g., looking for stubs for version 3, but there aren't any at time of writing, so don't try and load them. return; } $a = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($path)); foreach ($a as $file) { if (!$file->isDir()) { - $api->addStubFile($file->getPathname()); + $registration->addStubFile($file->getPathname()); } } } diff --git a/src/Stubs/common/Component/PropertyAccess/PropertyAccessorInterface.stubphp b/src/Stubs/common/Component/PropertyAccess/PropertyAccessorInterface.stubphp index db8bd358..607809e5 100644 --- a/src/Stubs/common/Component/PropertyAccess/PropertyAccessorInterface.stubphp +++ b/src/Stubs/common/Component/PropertyAccess/PropertyAccessorInterface.stubphp @@ -11,7 +11,7 @@ interface PropertyAccessorInterface * @psalm-param mixed $value * @psalm-param-out T $objectOrArray */ - public function setValue(&$objectOrArray, $propertyPath, $value); + public function setValue(object|array &$objectOrArray, string|PropertyPathInterface $propertyPath, mixed $value); /** * @param object|array $objectOrArray @@ -19,7 +19,7 @@ interface PropertyAccessorInterface * * @return mixed */ - public function getValue($objectOrArray, $propertyPath); + public function getValue(object|array $objectOrArray, string|PropertyPathInterface $propertyPath): mixed; /** * @param object|array $objectOrArray @@ -27,7 +27,7 @@ interface PropertyAccessorInterface * * @return bool */ - public function isWritable($objectOrArray, $propertyPath); + public function isWritable(object|array $objectOrArray, string|PropertyPathInterface $propertyPath): bool; /** * @param object|array $objectOrArray @@ -35,5 +35,5 @@ interface PropertyAccessorInterface * * @return bool */ - public function isReadable($objectOrArray, $propertyPath); + public function isReadable(object|array $objectOrArray, string|PropertyPathInterface $propertyPath): bool; } diff --git a/src/Stubs/common/Component/Security/Core/Authorization/Voter/Voter.stubphp b/src/Stubs/common/Component/Security/Core/Authorization/Voter/Voter.stubphp deleted file mode 100644 index 57d88416..00000000 --- a/src/Stubs/common/Component/Security/Core/Authorization/Voter/Voter.stubphp +++ /dev/null @@ -1,42 +0,0 @@ -container->get(SomeService::class)->do(); + $container->get(SomeService::class)->do(); } } """ @@ -30,19 +33,11 @@ Feature: Container service Scenario: Asserting psalm recognizes return type of service got via 'ContainerInterface::get()'. Given I have the following code """ - container->get(SomeService::class)->nosuchmethod(); + $container->get(SomeService::class)->nosuchmethod(); } } """ @@ -55,18 +50,16 @@ Feature: Container service Scenario: Container get(self::class) should not crash Given I have the following code """ - container->get(self::class)->index(); + $container->get(self::class)->index(); } } """ When I run Psalm Then I see these errors - | Type | Message | - | MissingReturnType | Method SomeController::index does not have a return type, expecting void | + | Type | Message | + | MixedMethodCall | Cannot determine the type of the object on the left hand side of this expression | + And I see no other errors diff --git a/tests/acceptance/acceptance/ContainerXml.feature b/tests/acceptance/acceptance/ContainerXml.feature index 8c4f8ab7..8021a8e1 100644 --- a/tests/acceptance/acceptance/ContainerXml.feature +++ b/tests/acceptance/acceptance/ContainerXml.feature @@ -7,19 +7,22 @@ Feature: Container XML config """ ../../tests/acceptance/container.xml """ + And I have the following code preamble + """ + container->get('service_container')->has('lorem'); + return $container->get('service_container')->has('lorem'); } } """ @@ -29,15 +32,11 @@ Feature: Container XML config Scenario: Psalm emits when service ID not found in container' Given I have the following code """ - container->get('not_a_service')->has('lorem'); + $container->get('not_a_service')->has('lorem'); } } """ @@ -49,18 +48,12 @@ Feature: Container XML config Scenario: Using service both via alias and class const Given I have the following code """ - container->get('http_kernel'); - $this->container->get(HttpKernelInterface::class); + $container->get('http_kernel'); + $container->get(HttpKernelInterface::class); } } """ @@ -70,15 +63,11 @@ Feature: Container XML config Scenario: Using private service Given I have the following code """ - container->get('private_service'); + $container->get('private_service'); } } """ diff --git a/tests/acceptance/acceptance/HeaderBag.feature b/tests/acceptance/acceptance/HeaderBag.feature index 157278c0..ae1a753c 100644 --- a/tests/acceptance/acceptance/HeaderBag.feature +++ b/tests/acceptance/acceptance/HeaderBag.feature @@ -2,7 +2,7 @@ Feature: Header get Background: - Given I have issue handler "UnusedFunctionCall" suppressed + Given I have issue handler "UnusedVariable" suppressed And I have Symfony plugin enabled And I have the following code preamble """ @@ -11,24 +11,23 @@ Feature: Header get use Symfony\Component\HttpFoundation\Request; """ - Scenario: HeaderBag get method return type should return `?string` (unless third argument is provided for < Sf4.4) + Scenario: HeaderBag get method return type should return `?string` Given I have the following code """ class App { public function index(Request $request): void { - $string = $request->headers->get('nullable_string'); - if (!$string) { - return; - } - - trim($string); + /** @psalm-trace $nullableString */ + $nullableString = $request->headers->get('nullable_string'); } } """ When I run Psalm - Then I see no errors + Then I see these errors + | Type | Message | + | Trace | $nullableString: null\|string | + And I see no other errors Scenario: HeaderBag get method return type should return `string` if default value is provided with string Given I have the following code @@ -37,11 +36,13 @@ Feature: Header get { public function index(Request $request): void { + /** @psalm-trace $string */ $string = $request->headers->get('string', 'string'); - - trim($string); } } """ When I run Psalm - Then I see no errors + Then I see these errors + | Type | Message | + | Trace | $string: string | + And I see no other errors diff --git a/tests/acceptance/acceptance/NamingConventions.feature b/tests/acceptance/acceptance/NamingConventions.feature index a376170f..928149fa 100644 --- a/tests/acceptance/acceptance/NamingConventions.feature +++ b/tests/acceptance/acceptance/NamingConventions.feature @@ -7,19 +7,22 @@ Feature: Naming conventions """ ../../tests/acceptance/container.xml """ + And I have the following code preamble + """ + container->get('service_container')->has('lorem'); + return $container->get('service_container')->has('lorem'); } } """ @@ -29,15 +32,11 @@ Feature: Naming conventions Scenario: Detects service naming convention violation Given I have the following code """ - container->get('wronglyNamedService'); + $container->get('wronglyNamedService'); } } """ @@ -50,17 +49,11 @@ Feature: Naming conventions Scenario: No service naming convention violation when using FQCNs Given I have the following code """ - container->get(HttpKernelInterface::class); + $container->get(HttpKernelInterface::class); } } """ @@ -70,15 +63,11 @@ Feature: Naming conventions Scenario: No naming convention violation for parameter Given I have the following code """ - container->getParameter('kernel.cache_dir'); + $container->getParameter('kernel.cache_dir'); } } """ @@ -88,15 +77,11 @@ Feature: Naming conventions Scenario: Detects parameter naming convention violation Given I have the following code """ - container->getParameter('wronglyNamedParameter'); + $container->getParameter('wronglyNamedParameter'); } } """ @@ -109,15 +94,11 @@ Feature: Naming conventions Scenario: No parameter naming convention violation when using environment variables Given I have the following code """ - container->getParameter('env(SOME_ENV_VAR)'); + $container->getParameter('env(SOME_ENV_VAR)'); } } """ diff --git a/tests/acceptance/acceptance/PropertyAccessorInterface.feature b/tests/acceptance/acceptance/PropertyAccessorInterface.feature index 79499b96..8503fe62 100644 --- a/tests/acceptance/acceptance/PropertyAccessorInterface.feature +++ b/tests/acceptance/acceptance/PropertyAccessorInterface.feature @@ -9,6 +9,7 @@ Feature: PropertyAccessorInterface setValue($company, 'name', 'Acme v2'); - array_key_exists('name', $company); + /** @psalm-trace $company */ """ When I run Psalm - Then I see no errors + Then I see these errors + | Type | Message | + | Trace | $company: array | + And I see no other errors Scenario: Set value keeps object instance if an object is passed Given I have the following code @@ -36,28 +40,26 @@ Feature: PropertyAccessorInterface $propertyAccessor->setValue($company, 'name', 'Acme v2'); - echo $company->name; + /** @psalm-trace $company */ """ When I run Psalm - Then I see no errors + Then I see these errors + | Type | Message | + | Trace | $company: Company | + And I see no other errors Scenario: Set value does not modify the propertyAccessor variable Given I have the following code """ - use Symfony\Component\PropertyAccess\PropertyAccessorInterface; - class Company { - /** - * @var PropertyAccessorInterface - */ - private $propertyAccessor; - - public function __construct(PropertyAccessorInterface $propertyAccessor) { - $this->propertyAccessor = $propertyAccessor; + public function __construct( + private PropertyAccessorInterface $propertyAccessor, + ) { } - public function doThings(Company $thing): void { + public function doThings(Company $thing): void + { $this->propertyAccessor->setValue($thing, 'foo', 'bar'); $this->propertyAccessor->setValue($thing, 'foo', 'bar'); } diff --git a/tests/acceptance/acceptance/Voter.feature b/tests/acceptance/acceptance/Voter.feature deleted file mode 100644 index 2dca842d..00000000 --- a/tests/acceptance/acceptance/Voter.feature +++ /dev/null @@ -1,40 +0,0 @@ -@symfony-common -Feature: Voter abstract class - - Background: - Given I have Symfony plugin enabled - And I have the following code preamble - """ -