From 247647254de7fa714c9733f4c26528d3dea7a505 Mon Sep 17 00:00:00 2001 From: Farhad Safarov Date: Mon, 9 Mar 2020 15:24:39 +0300 Subject: [PATCH] refactor container check (#8) * refactor container check * no message * no message * no message --- .gitignore | 1 + .php_cs.dist | 20 ++++++ .travis.yml | 1 + README.md | 36 +++++++++- codeception.yml | 10 +-- composer.json | 1 + src/Handler/ClassHandler.php | 63 ---------------- src/Handler/ContainerHandler.php | 39 ++++++++++ src/Handler/ContainerXmlHandler.php | 71 +++++++++++++++++++ src/Issue/ServiceNotFound.php | 14 ++++ src/Plugin.php | 20 +++++- src/SymfonyContainer.php | 43 +++++++++++ tests/{ => acceptance}/_output/.gitignore | 0 tests/{ => acceptance}/_run/.gitignore | 0 .../_support/AcceptanceTester.php | 0 .../_support/_generated/.gitignore | 0 tests/{ => acceptance}/acceptance.suite.yml | 0 .../ContainerDependency.feature | 0 .../{ => acceptance}/ContainerService.feature | 0 .../acceptance/ContainerXml.feature | 59 +++++++++++++++ .../RepositoryStringShortcut.feature | 0 .../{ => acceptance}/RequestContent.feature | 0 tests/acceptance/container.xml | 9 +++ 23 files changed, 317 insertions(+), 70 deletions(-) create mode 100644 .php_cs.dist create mode 100644 src/Handler/ContainerHandler.php create mode 100644 src/Handler/ContainerXmlHandler.php create mode 100644 src/Issue/ServiceNotFound.php create mode 100644 src/SymfonyContainer.php rename tests/{ => acceptance}/_output/.gitignore (100%) rename tests/{ => acceptance}/_run/.gitignore (100%) rename tests/{ => acceptance}/_support/AcceptanceTester.php (100%) rename tests/{ => acceptance}/_support/_generated/.gitignore (100%) rename tests/{ => acceptance}/acceptance.suite.yml (100%) rename tests/acceptance/{ => acceptance}/ContainerDependency.feature (100%) rename tests/acceptance/{ => acceptance}/ContainerService.feature (100%) create mode 100644 tests/acceptance/acceptance/ContainerXml.feature rename tests/acceptance/{ => acceptance}/RepositoryStringShortcut.feature (100%) rename tests/acceptance/{ => acceptance}/RequestContent.feature (100%) create mode 100644 tests/acceptance/container.xml diff --git a/.gitignore b/.gitignore index 4a45ef5..3ecc762 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ vendor composer.lock .php_cs.cache +tests/_run diff --git a/.php_cs.dist b/.php_cs.dist new file mode 100644 index 0000000..d2d94d7 --- /dev/null +++ b/.php_cs.dist @@ -0,0 +1,20 @@ +in(__DIR__.'/src') + ->ignoreDotFiles(true) + ->ignoreVCS(true) + ->files() + ->name('*.php'); + +return PhpCsFixer\Config::create() + ->setFinder($finder) + ->setRules([ + '@Symfony' => true, + 'array_syntax' => ['syntax' => 'short'], + 'binary_operator_spaces' => ['align_double_arrow' => false], + 'no_useless_else' => true, + 'no_useless_return' => false, + 'ordered_imports' => true, + 'phpdoc_to_comment' => false, + ]); diff --git a/.travis.yml b/.travis.yml index dbb5c4b..00d6291 100644 --- a/.travis.yml +++ b/.travis.yml @@ -7,6 +7,7 @@ php: before_install: - phpenv config-rm xdebug.ini || true + - composer validate install: - composer install diff --git a/README.md b/README.md index 1fc6b4a..a3265d2 100644 --- a/README.md +++ b/README.md @@ -11,11 +11,45 @@ vendor/bin/psalm-plugin enable seferov/symfony-psalm-plugin ### Features -- Detect `ContainerInterface::get()` result type +- Detect `ContainerInterface::get()` result type. Works better if you [configure](https://github.com/seferov/symfony-psalm-plugin/#configuration) compiled container XML file. - Fixes `PossiblyInvalidArgument` for `Symfony\Component\HttpFoundation\Request::getContent`. The plugin calculates real return type by checking the given argument and marks return type as either string or resource. - Complains when `Container` is injected to a service. Use dependency-injection. +### Configuration + +If you followed installation instructions, psalm-plugin command would added plugin configuration to psalm.xml + +```xml + + + + + + + + +``` + +To be able to detect return types of services using ID (generally starts with `@` in Symfony YAML config files. Ex: `logger` service) +`containerXml` must be provided. Example: +```xml + + var/cache/dev/App_KernelDevDebugContainer.xml + +``` + +This file path may change based on your Symfony version, file structure and environment settings. +Default file for Symfony versions: +- Symfony 3: var/cache/dev/srcDevDebugProjectContainer.xml +- Symfony 4: var/cache/dev/srcApp_KernelDevDebugContainer.xml +- Symfony 5: var/cache/dev/App_KernelDevDebugContainer.xml + ### Credits - [@weirdan](https://github.com/weirdan) for [codeception psalm module](https://github.com/weirdan/codeception-psalm-module) diff --git a/codeception.yml b/codeception.yml index e51bc3b..cb671fd 100644 --- a/codeception.yml +++ b/codeception.yml @@ -1,11 +1,11 @@ namespace: Psalm\PhpUnitPlugin\Tests paths: - tests: tests - output: tests/_output - data: tests/_data - support: tests/_support - envs: tests/_envs + tests: tests/acceptance + output: tests/acceptance/_output + data: tests/acceptance/_data + support: tests/acceptance/_support + envs: tests/acceptance/_envs actor_suffix: Tester diff --git a/composer.json b/composer.json index 39ed140..fdf29a5 100644 --- a/composer.json +++ b/composer.json @@ -11,6 +11,7 @@ ], "require": { "php": "^7.1", + "ext-simplexml": "*", "vimeo/psalm": "^3.7", "symfony/framework-bundle": "^3.0 || ^4.0 || ^5.0" }, diff --git a/src/Handler/ClassHandler.php b/src/Handler/ClassHandler.php index 2950816..a98dd2c 100644 --- a/src/Handler/ClassHandler.php +++ b/src/Handler/ClassHandler.php @@ -13,7 +13,6 @@ use Psalm\Plugin\Hook\AfterClassLikeAnalysisInterface; use Psalm\Plugin\Hook\AfterMethodCallAnalysisInterface; use Psalm\StatementsSource; use Psalm\Storage\ClassLikeStorage; -use Psalm\Type\Atomic\TNamedObject; use Psalm\Type\Union; use Seferov\SymfonyPsalmPlugin\Issue\ContainerDependency; use Seferov\SymfonyPsalmPlugin\Issue\RepositoryStringShortcut; @@ -21,11 +20,6 @@ use Symfony\Component\DependencyInjection\ContainerInterface; class ClassHandler implements AfterClassLikeAnalysisInterface, AfterMethodCallAnalysisInterface { - /** - * @psalm-var array - */ - private static $classServiceMap = []; - /** * {@inheritdoc} */ @@ -62,22 +56,6 @@ class ClassHandler implements AfterClassLikeAnalysisInterface, AfterMethodCallAn Union &$return_type_candidate = null ) { switch ($declaring_method_id) { - case 'Psr\Container\ContainerInterface::get': - case 'Symfony\Component\DependencyInjection\ContainerInterface::get': - if ($return_type_candidate && $expr->args[0]->value instanceof ClassConstFetch) { - $className = (string) $expr->args[0]->value->class->getAttribute('resolvedName'); - $return_type_candidate = new Union([new TNamedObject($className)]); - } - if (!count(self::$classServiceMap)) { - self::$classServiceMap = self::loadServiceFile($codebase); - } - if ($return_type_candidate && count(self::$classServiceMap) && $expr->args[0]->value instanceof Node\Scalar\String_) { - $serviceName = (string) $expr->args[0]->value->value; - if (isset(self::$classServiceMap[$serviceName])) { - $return_type_candidate = new Union([new TNamedObject((string) self::$classServiceMap[$serviceName])]); - } - } - break; case 'Symfony\Component\HttpFoundation\Request::getcontent': if ($return_type_candidate) { $removeType = 'resource'; @@ -99,45 +77,4 @@ class ClassHandler implements AfterClassLikeAnalysisInterface, AfterMethodCallAn break; } } - - /** - * @todo don't check every time if containerXml config is not set. - * @psalm-return array - */ - private static function loadServiceFile(Codebase $codebase): array - { - $classServiceMap = []; - if (count($codebase->config->getPluginClasses())) { - foreach ($codebase->config->getPluginClasses() as $pluginClass) { - if ($pluginClass['class'] === str_replace('Handler', 'Plugin', __NAMESPACE__)) { - $simpleXmlConfig = $pluginClass['config']; - } - } - } - - if (isset($simpleXmlConfig) && $simpleXmlConfig instanceof \SimpleXMLElement) { - $serviceFilePath = (string) $simpleXmlConfig->containerXml; - if (!file_exists($serviceFilePath)) { - return []; - } - $xml = simplexml_load_file($serviceFilePath); - if (!$xml->services instanceof \SimpleXMLElement) { - return $classServiceMap; - } - $services = $xml->services; - /** @psalm-suppress MixedAssignment */ - if (count($services)) { - foreach ($services->service as $serviceObj) { - if (isset($serviceObj) && $serviceObj instanceof \SimpleXMLElement) { - $serviceAttributes = $serviceObj->attributes(); - if ($serviceAttributes && isset($serviceAttributes['id']) && isset($serviceAttributes['class'])) { - $classServiceMap[(string) $serviceAttributes['id']] = (string) $serviceAttributes['class']; - } - } - } - } - } - - return $classServiceMap; - } } diff --git a/src/Handler/ContainerHandler.php b/src/Handler/ContainerHandler.php new file mode 100644 index 0000000..a6d1150 --- /dev/null +++ b/src/Handler/ContainerHandler.php @@ -0,0 +1,39 @@ +args[0]->value instanceof ClassConstFetch) { + $className = (string) $expr->args[0]->value->class->getAttribute('resolvedName'); + $return_type_candidate = new Union([new TNamedObject($className)]); + } + } +} diff --git a/src/Handler/ContainerXmlHandler.php b/src/Handler/ContainerXmlHandler.php new file mode 100644 index 0000000..b5a75ec --- /dev/null +++ b/src/Handler/ContainerXmlHandler.php @@ -0,0 +1,71 @@ +args[0]->value instanceof String_) { + $simpleXmlConfig = null; + if (count($codebase->config->getPluginClasses())) { + foreach ($codebase->config->getPluginClasses() as $pluginClass) { + if ($pluginClass['class'] === str_replace('Handler', 'Plugin', __NAMESPACE__)) { + $simpleXmlConfig = (string) $pluginClass['config']; + } + } + } + + if (!is_string($simpleXmlConfig)) { + throw new \LogicException('This hook is registered when xml file is set'); + } + + $symfonyContainer = new SymfonyContainer($simpleXmlConfig); + $serviceId = (string) $expr->args[0]->value->value; + $serviceDefinition = $symfonyContainer->get($serviceId); + if ($serviceDefinition) { + if ($serviceDefinition->isPublic()) { + $class = $serviceDefinition->getClass(); + if ($class) { + $return_type_candidate = new Union([new TNamedObject($class)]); + } + } + // @todo: else emit "get private service" issue + } else { + IssueBuffer::accepts( + new ServiceNotFound($serviceId, new CodeLocation($statements_source, $expr->args[0]->value)), + $statements_source->getSuppressedIssues() + ); + } + } + } +} diff --git a/src/Issue/ServiceNotFound.php b/src/Issue/ServiceNotFound.php new file mode 100644 index 0000000..74740f6 --- /dev/null +++ b/src/Issue/ServiceNotFound.php @@ -0,0 +1,14 @@ +registerHooksFromClass(ClassHandler::class); + + if (isset($config->containerXml)) { + $containerXmlPath = realpath((string) $config->containerXml); + if (!$containerXmlPath) { + throw new ConfigException(sprintf('Container XML file (%s) does not exits', $containerXmlPath)); + } + + $api->registerHooksFromClass(ContainerXmlHandler::class); + } else { + $api->registerHooksFromClass(ContainerHandler::class); + } } } diff --git a/src/SymfonyContainer.php b/src/SymfonyContainer.php new file mode 100644 index 0000000..e133b0e --- /dev/null +++ b/src/SymfonyContainer.php @@ -0,0 +1,43 @@ + + */ + private $services = []; + + public function __construct(string $containerXmlPath) + { + $xml = simplexml_load_file($containerXmlPath); + if (!$xml->services instanceof \SimpleXMLElement) { + throw new ConfigException('Not a valid container xml file'); + } + + /** @psalm-var \SimpleXMLElement $service */ + foreach ($xml->services->service as $service) { + /** @psalm-var \SimpleXMLElement $serviceAttributes */ + $serviceAttributes = $service->attributes(); + $definition = new Definition((string) $serviceAttributes->class); + $definition->setPublic((bool) $serviceAttributes->public); + + $this->services[(string) $serviceAttributes->id] = $definition; + } + } + + public function get(string $id): ?Definition + { + if (isset($this->services[$id])) { + return $this->services[$id]; + } + + return null; + } +} diff --git a/tests/_output/.gitignore b/tests/acceptance/_output/.gitignore similarity index 100% rename from tests/_output/.gitignore rename to tests/acceptance/_output/.gitignore diff --git a/tests/_run/.gitignore b/tests/acceptance/_run/.gitignore similarity index 100% rename from tests/_run/.gitignore rename to tests/acceptance/_run/.gitignore diff --git a/tests/_support/AcceptanceTester.php b/tests/acceptance/_support/AcceptanceTester.php similarity index 100% rename from tests/_support/AcceptanceTester.php rename to tests/acceptance/_support/AcceptanceTester.php diff --git a/tests/_support/_generated/.gitignore b/tests/acceptance/_support/_generated/.gitignore similarity index 100% rename from tests/_support/_generated/.gitignore rename to tests/acceptance/_support/_generated/.gitignore diff --git a/tests/acceptance.suite.yml b/tests/acceptance/acceptance.suite.yml similarity index 100% rename from tests/acceptance.suite.yml rename to tests/acceptance/acceptance.suite.yml diff --git a/tests/acceptance/ContainerDependency.feature b/tests/acceptance/acceptance/ContainerDependency.feature similarity index 100% rename from tests/acceptance/ContainerDependency.feature rename to tests/acceptance/acceptance/ContainerDependency.feature diff --git a/tests/acceptance/ContainerService.feature b/tests/acceptance/acceptance/ContainerService.feature similarity index 100% rename from tests/acceptance/ContainerService.feature rename to tests/acceptance/acceptance/ContainerService.feature diff --git a/tests/acceptance/acceptance/ContainerXml.feature b/tests/acceptance/acceptance/ContainerXml.feature new file mode 100644 index 0000000..7964c1a --- /dev/null +++ b/tests/acceptance/acceptance/ContainerXml.feature @@ -0,0 +1,59 @@ +Feature: Container XML config + Detect ContainerInterface::get() result type + + Background: + Given I have the following config + """ + + + + + + + + + + ../../tests/acceptance/container.xml + + + + """ + + Scenario: Asserting psalm recognizes return type of service got via 'ContainerInterface::get() using service ID' + Given I have the following code + """ + container->get('service_container')->has('lorem'); + } + } + """ + When I run Psalm + Then I see no errors + + # todo: emit a service not found error instead of PossiblyNullReference + Scenario: Psalm emits when service ID not found in container' + Given I have the following code + """ + container->get('not_a_service')->has('lorem'); + } + } + """ + When I run Psalm + Then I see these errors + | Type | Message | + | ServiceNotFound | Service "not_a_service" not found | diff --git a/tests/acceptance/RepositoryStringShortcut.feature b/tests/acceptance/acceptance/RepositoryStringShortcut.feature similarity index 100% rename from tests/acceptance/RepositoryStringShortcut.feature rename to tests/acceptance/acceptance/RepositoryStringShortcut.feature diff --git a/tests/acceptance/RequestContent.feature b/tests/acceptance/acceptance/RequestContent.feature similarity index 100% rename from tests/acceptance/RequestContent.feature rename to tests/acceptance/acceptance/RequestContent.feature diff --git a/tests/acceptance/container.xml b/tests/acceptance/container.xml new file mode 100644 index 0000000..7ba3dde --- /dev/null +++ b/tests/acceptance/container.xml @@ -0,0 +1,9 @@ + + + + dev + + + + +