From 85ff969f30ee4b75f5c5ed818ea45965ccbf2f6b Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Wed, 1 May 2019 12:41:20 +0200 Subject: [PATCH 1/2] [CodingStyle] Fix ImportFullyQualifiedNamesRector for multiple same-ending doc types --- .../Fixture/many_imports.php.inc | 97 +++++++++++++++++++ .../ImportFullyQualifiedNamesRectorTest.php | 3 + 2 files changed, 100 insertions(+) create mode 100644 packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Fixture/many_imports.php.inc diff --git a/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Fixture/many_imports.php.inc b/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Fixture/many_imports.php.inc new file mode 100644 index 000000000000..b4506651dce4 --- /dev/null +++ b/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Fixture/many_imports.php.inc @@ -0,0 +1,97 @@ +registry = $registry; + } + + public function clear(array $stockrooms): void + { + /** @var \Doctrine\DBAL\Connection $connection */ + $connection = $this->registry->getConnection(); + $query = $connection->createQueryBuilder(); + + $query->delete('elasticr_stocks') + ->andWhere('stockroom_id IN (:id)') + ->setParameter('id', $stockrooms, Connection::PARAM_INT_ARRAY); + + $query->execute(); + } + + /** + * @throws \Doctrine\DBAL\DBALException + */ + public function add(InventoryItems $items): void + { + /** @var \Doctrine\DBAL\Connection $connection */ + $connection = $this->registry->getConnection(); + + $counter = 0; + $inserts = []; + + foreach ($items->items() as $item) { + foreach ($item->stocks() as $stock) { + $index = $counter / 1000; + + $inserts[$index]['values'][] = '(:uuid'.$counter.', :stockroom'.$counter.', :quantity'.$counter.')'; + + $inserts[$index]['parameters'][':uuid'.$counter] = $item->uuid(); + $inserts[$index]['parameters'][':stockroom'.$counter] = $stock->stockroom()->id(); + $inserts[$index]['parameters'][':quantity'.$counter] = $stock->quantity(); + + ++$counter; + } + } + + foreach ($inserts as $insert) { + $query = 'INSERT INTO elasticr_stocks (uuid, stockroom_id, quantity) VALUES'.implode(',', $insert['values']); + + $connection->executeUpdate($query, $insert['parameters']); + } + } + + public function filter(Stock\Query $query): InventoryItems + { + if (!($query instanceof Stock\Querying\Query)) { + throw new \InvalidArgumentException('Wrong query provided'); + } + + /** @var Querying\Query $query */ + $builder = $this->createBuilder(); + $query->build($builder); + + /** @var \Elasticr\Inventory\Doctrine\Stock[] $stocks */ + $stocks = $builder->getQuery() + ->useQueryCache(true) + ->useResultCache(true) + ->getResult(); + + $inventoryItems = new InventoryItems(); + + foreach ($stocks as $stock) { + $inventoryItems->add($stock->uuid->toString(), $stock->stockroom, $stock->quantity); + } + + return $inventoryItems; + } + + private function createBuilder(): QueryBuilder + { + /** @var \Doctrine\ORM\EntityManager $manager */ + $manager = $this->registry->getManager(); + + return $manager->createQueryBuilder(); + } +} diff --git a/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/ImportFullyQualifiedNamesRectorTest.php b/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/ImportFullyQualifiedNamesRectorTest.php index 7f6eb112edcb..f7d0b2e84b88 100644 --- a/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/ImportFullyQualifiedNamesRectorTest.php +++ b/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/ImportFullyQualifiedNamesRectorTest.php @@ -25,6 +25,9 @@ public function test(): void __DIR__ . '/Fixture/doc_combined.php.inc', __DIR__ . '/Fixture/conflicting_endings.php.inc', __DIR__ . '/Fixture/already_class_name_in_param_doc.php.inc', + + // buggy + __DIR__ . '/Fixture/many_imports.php.inc', ]); } From c5fc3318576c25c2fd586dd20b2c9e93f80faeed Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Wed, 1 May 2019 13:15:18 +0200 Subject: [PATCH 2/2] add ImportsInClassCollection --- packages/CodingStyle/config/config.yaml | 8 ++ .../src/Imports/ImportsInClassCollection.php | 60 +++++++++++ .../CodingStyle/src/Naming/ClassNaming.php | 13 +++ .../ImportFullyQualifiedNamesRector.php | 72 ++++++++------ .../Fixture/keep_same_end.php.inc | 28 +++++- .../Fixture/many_imports.php.inc | 99 ++++--------------- .../Source/Querying/Query.php | 8 ++ .../Source/Stock/Query.php | 8 ++ .../NodeAnalyzer/DocBlockManipulator.php | 32 +++--- 9 files changed, 206 insertions(+), 122 deletions(-) create mode 100644 packages/CodingStyle/config/config.yaml create mode 100644 packages/CodingStyle/src/Imports/ImportsInClassCollection.php create mode 100644 packages/CodingStyle/src/Naming/ClassNaming.php create mode 100644 packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Source/Querying/Query.php create mode 100644 packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Source/Stock/Query.php diff --git a/packages/CodingStyle/config/config.yaml b/packages/CodingStyle/config/config.yaml new file mode 100644 index 000000000000..73df5362df1f --- /dev/null +++ b/packages/CodingStyle/config/config.yaml @@ -0,0 +1,8 @@ +services: + _defaults: + autowire: true + public: true + + Rector\CodingStyle\: + resource: '../src' + exclude: '../src/{Rector/**/*Rector.php}' diff --git a/packages/CodingStyle/src/Imports/ImportsInClassCollection.php b/packages/CodingStyle/src/Imports/ImportsInClassCollection.php new file mode 100644 index 000000000000..b2ed9a9c99e3 --- /dev/null +++ b/packages/CodingStyle/src/Imports/ImportsInClassCollection.php @@ -0,0 +1,60 @@ +classNaming = $classNaming; + } + + public function addImport(string $import): void + { + $this->importsInClass[] = $import; + } + + public function hasImport(string $import): bool + { + return in_array($import, $this->importsInClass, true); + } + + public function canImportBeAdded(string $import): bool + { + $shortImport = $this->classNaming->getShortName($import); + + foreach ($this->importsInClass as $importsInClass) { + $shortImportInClass = $this->classNaming->getShortName($importsInClass); + if ($importsInClass !== $import && $shortImportInClass === $shortImport) { + return true; + } + } + + return false; + } + + public function reset(): void + { + $this->importsInClass = []; + } + + /** + * @return string[] + */ + public function get(): array + { + return $this->importsInClass; + } +} diff --git a/packages/CodingStyle/src/Naming/ClassNaming.php b/packages/CodingStyle/src/Naming/ClassNaming.php new file mode 100644 index 000000000000..5efafaba532f --- /dev/null +++ b/packages/CodingStyle/src/Naming/ClassNaming.php @@ -0,0 +1,13 @@ +callableNodeTraverser = $callableNodeTraverser; $this->docBlockManipulator = $docBlockManipulator; + $this->importsInClassCollection = $importsInClassCollection; + $this->classNaming = $classNaming; } public function getDefinition(): RectorDefinition @@ -91,8 +104,9 @@ public function getNodeTypes(): array */ public function refactor(Node $node): ?Node { - $this->alreadyImportedUses = []; $this->newUseStatements = []; + $this->importsInClassCollection->reset(); + $this->docBlockManipulator->resetImportedNames(); /** @var Class_|null $class */ $class = $this->betterNodeFinder->findFirstInstanceOf($node, Class_::class); @@ -110,6 +124,17 @@ public function refactor(Node $node): ?Node private function resolveAlreadyImportedUses(Namespace_ $namespace): void { + /** @var Class_ $class */ + $class = $this->betterNodeFinder->findFirstInstanceOf($namespace->stmts, Class_::class); + + // add class itself + $className = $this->getName($class); + if ($className === null) { + return; + } + + $this->importsInClassCollection->addImport($className); + /** @var Use_[] $uses */ $uses = $this->betterNodeFinder->find($namespace->stmts, function (Node $node) { if (! $node instanceof Use_) { @@ -132,7 +157,7 @@ private function resolveAlreadyImportedUses(Namespace_ $namespace): void $this->aliasedUses[] = $name; } - $this->alreadyImportedUses[] = $name; + $this->importsInClassCollection->addImport($name); } } @@ -145,7 +170,7 @@ private function resolveAlreadyImportedUses(Namespace_ $namespace): void return; } - $this->alreadyImportedUses[] = $className; + $this->importsInClassCollection->addImport($className); } /** @@ -161,14 +186,14 @@ private function addNewUseStatements(Namespace_ $namespace, array $newUseStateme foreach ($newUseStatements as $newUseStatement) { // already imported in previous cycle - if (in_array($newUseStatement, $this->alreadyImportedUses, true)) { + if ($this->importsInClassCollection->hasImport($newUseStatement)) { continue; } $useUse = new UseUse(new Name($newUseStatement)); $newUses[] = new Use_([$useUse]); - $this->alreadyImportedUses[] = $newUseStatement; + $this->importsInClassCollection->addImport($newUseStatement); } $namespace->stmts = array_merge($newUses, $namespace->stmts); @@ -179,6 +204,7 @@ private function addNewUseStatements(Namespace_ $namespace, array $newUseStateme */ private function importNamesAndCollectNewUseStatements(Class_ $class): array { + // probably anonymous class if ($class->name === null) { return []; } @@ -213,7 +239,7 @@ private function importNamesAndCollectNewUseStatements(Class_ $class): array return null; } - $shortName = $this->getShortName($fullyQualifiedName); + $shortName = $this->classNaming->getShortName($fullyQualifiedName); if (isset($this->newUseStatements[$shortName])) { if ($fullyQualifiedName === $this->newUseStatements[$shortName]) { return new Name($shortName); @@ -222,7 +248,7 @@ private function importNamesAndCollectNewUseStatements(Class_ $class): array return null; } - if (! in_array($fullyQualifiedName, $this->alreadyImportedUses, true)) { + if (! $this->importsInClassCollection->hasImport($fullyQualifiedName)) { $this->newUseStatements[$shortName] = $fullyQualifiedName; } @@ -237,18 +263,13 @@ private function importNamesAndCollectNewUseStatements(Class_ $class): array // for doc blocks $this->callableNodeTraverser->traverseNodesWithCallable([$class], function (Node $node): void { - $importedDocUseStatements = $this->docBlockManipulator->importNames($node, $this->alreadyImportedUses); + $importedDocUseStatements = $this->docBlockManipulator->importNames($node); $this->newUseStatements = array_merge($this->newUseStatements, $importedDocUseStatements); }); return $this->newUseStatements; } - private function getShortName(string $fullyQualifiedName): string - { - return Strings::after($fullyQualifiedName, '\\', -1) ?: $fullyQualifiedName; - } - // 1. name is fully qualified → import it private function shouldSkipName(string $fullyQualifiedName): bool { @@ -257,20 +278,13 @@ private function shouldSkipName(string $fullyQualifiedName): bool return true; } - $shortName = $this->getShortName($fullyQualifiedName); + $shortName = $this->classNaming->getShortName($fullyQualifiedName); // nothing to change if ($shortName === $fullyQualifiedName) { return true; } - foreach ($this->alreadyImportedUses as $alreadyImportedUse) { - $shortAlreadyImportedUsed = $this->getShortName($alreadyImportedUse); - if ($alreadyImportedUse !== $fullyQualifiedName && $shortAlreadyImportedUsed === $shortName) { - return true; - } - } - - return false; + return $this->importsInClassCollection->canImportBeAdded($fullyQualifiedName); } } diff --git a/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Fixture/keep_same_end.php.inc b/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Fixture/keep_same_end.php.inc index badb08a7d529..9d4f19f59282 100644 --- a/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Fixture/keep_same_end.php.inc +++ b/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Fixture/keep_same_end.php.inc @@ -7,9 +7,33 @@ use Some\Trait_; final class SameEnd { /** - * @param Another\Trait_ $phpStanScopeFactory + * @param \Some\Trait_ $firstTrait + * @param Another\Trait_ $secondTrait + * @param \Some\Trait_ $thirdTrait */ - public function __construct(Another\Trait_ $phpStanScopeFactory) + public function __construct(\Some\Trait_ $firstTrait, Another\Trait_ $secondTrait, \Some\Trait_ $thirdTrait) { } } + +?> +----- + diff --git a/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Fixture/many_imports.php.inc b/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Fixture/many_imports.php.inc index b4506651dce4..1a5c6aa5d9a8 100644 --- a/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Fixture/many_imports.php.inc +++ b/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Fixture/many_imports.php.inc @@ -1,97 +1,38 @@ registry = $registry; - } - - public function clear(array $stockrooms): void + public function filter(Stock\Query $query) { - /** @var \Doctrine\DBAL\Connection $connection */ - $connection = $this->registry->getConnection(); - $query = $connection->createQueryBuilder(); - - $query->delete('elasticr_stocks') - ->andWhere('stockroom_id IN (:id)') - ->setParameter('id', $stockrooms, Connection::PARAM_INT_ARRAY); + /** @var Stock\Query $query */ + $query = 5; - $query->execute(); + /** @var Querying\Query $query */ + $builder = $this->createBuilder(); + $query->build($builder); } +} - /** - * @throws \Doctrine\DBAL\DBALException - */ - public function add(InventoryItems $items): void - { - /** @var \Doctrine\DBAL\Connection $connection */ - $connection = $this->registry->getConnection(); - - $counter = 0; - $inserts = []; - - foreach ($items->items() as $item) { - foreach ($item->stocks() as $stock) { - $index = $counter / 1000; - - $inserts[$index]['values'][] = '(:uuid'.$counter.', :stockroom'.$counter.', :quantity'.$counter.')'; - - $inserts[$index]['parameters'][':uuid'.$counter] = $item->uuid(); - $inserts[$index]['parameters'][':stockroom'.$counter] = $stock->stockroom()->id(); - $inserts[$index]['parameters'][':quantity'.$counter] = $stock->quantity(); - - ++$counter; - } - } - - foreach ($inserts as $insert) { - $query = 'INSERT INTO elasticr_stocks (uuid, stockroom_id, quantity) VALUES'.implode(',', $insert['values']); +?> +----- +executeUpdate($query, $insert['parameters']); - } - } +namespace Rector\CodingStyle\Tests\Rector\Namespace_\ImportFullyQualifiedNamesRector\Source; - public function filter(Stock\Query $query): InventoryItems +use Rector\CodingStyle\Tests\Rector\Namespace_\ImportFullyQualifiedNamesRector\Source\Stock\Query; +final class StockRepository +{ + public function filter(Query $query) { - if (!($query instanceof Stock\Querying\Query)) { - throw new \InvalidArgumentException('Wrong query provided'); - } + /** @var Query $query */ + $query = 5; /** @var Querying\Query $query */ $builder = $this->createBuilder(); $query->build($builder); - - /** @var \Elasticr\Inventory\Doctrine\Stock[] $stocks */ - $stocks = $builder->getQuery() - ->useQueryCache(true) - ->useResultCache(true) - ->getResult(); - - $inventoryItems = new InventoryItems(); - - foreach ($stocks as $stock) { - $inventoryItems->add($stock->uuid->toString(), $stock->stockroom, $stock->quantity); - } - - return $inventoryItems; - } - - private function createBuilder(): QueryBuilder - { - /** @var \Doctrine\ORM\EntityManager $manager */ - $manager = $this->registry->getManager(); - - return $manager->createQueryBuilder(); } } + +?> diff --git a/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Source/Querying/Query.php b/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Source/Querying/Query.php new file mode 100644 index 000000000000..24474f49108e --- /dev/null +++ b/packages/CodingStyle/tests/Rector/Namespace_/ImportFullyQualifiedNamesRector/Source/Querying/Query.php @@ -0,0 +1,8 @@ +phpDocInfoFactory = $phpDocInfoFactory; $this->phpDocInfoPrinter = $phpDocInfoPrinter; @@ -90,6 +97,7 @@ public function __construct( $this->attributeAwareNodeFactory = $attributeAwareNodeFactory; $this->stringsTypePhpDocNodeDecorator = $stringsTypePhpDocNodeDecorator; $this->nodeTraverser = $nodeTraverser; + $this->importsInClassCollection = $importsInClassCollection; } public function hasTag(Node $node, string $name): bool @@ -391,27 +399,23 @@ public function replacePhpDocTypeByAnother( } /** - * @param string[] $alreadyImportedUses * @return string[] */ - public function importNames(Node $node, array $alreadyImportedUses): array + public function importNames(Node $node): array { if ($node->getDocComment() === null) { return []; } - $this->importedNames = []; - $phpDocInfo = $this->createPhpDocInfoFromNode($node); $phpDocNode = $phpDocInfo->getPhpDocNode(); - $this->nodeTraverser->traverseWithCallable($phpDocNode, function (AttributeAwareNodeInterface $node) use ( - $alreadyImportedUses - ) { + $this->nodeTraverser->traverseWithCallable($phpDocNode, function (AttributeAwareNodeInterface $node) { if (! $node instanceof IdentifierTypeNode) { return $node; } + // is class without namespaced name → skip $name = ltrim($node->name, '\\'); if (! Strings::contains($name, '\\')) { return $node; @@ -420,7 +424,7 @@ public function importNames(Node $node, array $alreadyImportedUses): array $fullyQualifiedName = $this->getFullyQualifiedName($node); $shortName = $this->getShortName($name); - return $this->processFqnNameImport($node, $alreadyImportedUses, $shortName, $fullyQualifiedName); + return $this->processFqnNameImport($node, $shortName, $fullyQualifiedName); }); $this->updateNodeWithPhpDocInfo($node, $phpDocInfo); @@ -468,6 +472,11 @@ public function changeUnderscoreType(Node $node, string $namespacePrefix, ?array $this->updateNodeWithPhpDocInfo($node, $phpDocInfo); } + public function resetImportedNames(): void + { + $this->importedNames = []; + } + private function addTypeSpecificTag(Node $node, string $name, string $type): void { // there might be no phpdoc at all @@ -607,15 +616,14 @@ private function getFullyQualifiedName(AttributeAwareNodeInterface $attributeAwa /** * @param AttributeAwareIdentifierTypeNode $attributeAwareNode - * @param string[] $alreadyImportedUses */ private function processFqnNameImport( AttributeAwareNodeInterface $attributeAwareNode, - array $alreadyImportedUses, string $shortName, string $fullyQualifiedName ): AttributeAwareNodeInterface { - $alreadyImportedUses = array_merge($this->importedNames, $alreadyImportedUses); + $alreadyImportedUses = array_merge($this->importedNames, $this->importsInClassCollection->get()); + $alreadyImportedUses = array_unique($alreadyImportedUses); foreach ($alreadyImportedUses as $alreadyImportedUse) { $shortAlreadyImportedUsed = $this->getShortName($alreadyImportedUse);