From 9c80a754305ab0023e6af4385876f07558b9ba28 Mon Sep 17 00:00:00 2001 From: TomasVotruba Date: Tue, 5 May 2020 23:13:54 +0200 Subject: [PATCH 1/4] make compiler restore original dependencies locally --- compiler/composer.json | 1 + compiler/config/config.yaml | 2 ++ .../src/Composer/ComposerJsonManipulator.php | 1 - .../src/Console/Command/CompileCommand.php | 24 ++++++++++++++++++- .../ClassMethodParamVendorLockResolver.php | 10 ++------ .../ClassMethodReturnVendorLockResolver.php | 8 +++---- 6 files changed, 31 insertions(+), 15 deletions(-) diff --git a/compiler/composer.json b/compiler/composer.json index 7fec8b6ea944..fa49ea2d7644 100644 --- a/compiler/composer.json +++ b/compiler/composer.json @@ -6,6 +6,7 @@ "php": "^7.2", "nette/neon": "^3.0", "nette/utils": "^3.0", + "ondram/ci-detector": "^3.3", "sebastian/diff": "^3.0|^4.0", "symfony/console": "^4.4|^5.0", "symfony/process": "^4.4|^5.0", diff --git a/compiler/config/config.yaml b/compiler/config/config.yaml index 95b2362f41d8..84a6295ce789 100644 --- a/compiler/config/config.yaml +++ b/compiler/config/config.yaml @@ -14,3 +14,5 @@ services: - '../src/HttpKernel/*' Symfony\Component\Filesystem\Filesystem: null + + OndraM\CiDetector\CiDetector: null diff --git a/compiler/src/Composer/ComposerJsonManipulator.php b/compiler/src/Composer/ComposerJsonManipulator.php index 4da687de7cc6..6f76114b72d0 100644 --- a/compiler/src/Composer/ComposerJsonManipulator.php +++ b/compiler/src/Composer/ComposerJsonManipulator.php @@ -61,7 +61,6 @@ public function fixComposerJson(string $composerJsonFile): void public function restoreComposerJson(string $composerJsonFile): void { $this->filesystem->dumpFile($composerJsonFile, $this->originalComposerJsonFileContent); - // re-run @todo composer update on root } private function removeDevKeys(array $json): array diff --git a/compiler/src/Console/Command/CompileCommand.php b/compiler/src/Console/Command/CompileCommand.php index 20c9ae37f03a..bca623da6886 100644 --- a/compiler/src/Console/Command/CompileCommand.php +++ b/compiler/src/Console/Command/CompileCommand.php @@ -4,6 +4,7 @@ namespace Rector\Compiler\Console\Command; +use OndraM\CiDetector\CiDetector; use Rector\Compiler\Composer\ComposerJsonManipulator; use Rector\Compiler\Debug\FileLister; use Rector\Compiler\Renaming\JetbrainsStubsRenamer; @@ -49,13 +50,19 @@ final class CompileCommand extends Command */ private $fileLister; + /** + * @var CiDetector + */ + private $ciDetector; + public function __construct( string $dataDir, string $buildDir, ComposerJsonManipulator $composerJsonManipulator, SymfonyStyle $symfonyStyle, JetbrainsStubsRenamer $jetbrainsStubsRenamer, - FileLister $fileLister + FileLister $fileLister, + CiDetector $ciDetector ) { $this->dataDir = $dataDir; $this->buildDir = $buildDir; @@ -64,6 +71,7 @@ public function __construct( $this->jetbrainsStubsRenamer = $jetbrainsStubsRenamer; $this->fileLister = $fileLister; $this->symfonyStyle = $symfonyStyle; + $this->ciDetector = $ciDetector; parent::__construct(); } @@ -102,6 +110,20 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->symfonyStyle->note('You still need to run "composer update" to install those dependencies'); $this->composerJsonManipulator->restoreComposerJson($composerJsonFile); + $this->restoreDependenciesLocallyIfNotCi($output); + return ShellCode::SUCCESS; } + + private function restoreDependenciesLocallyIfNotCi(OutputInterface $output): void + { + if ($this->ciDetector->isCiDetected()) { + return; + } + + $process = new Process(['composer', 'install'], $this->buildDir, null, null, null); + $process->mustRun(static function (string $type, string $buffer) use ($output): void { + $output->write($buffer); + }); + } } diff --git a/packages/vendor-locker/src/NodeVendorLocker/ClassMethodParamVendorLockResolver.php b/packages/vendor-locker/src/NodeVendorLocker/ClassMethodParamVendorLockResolver.php index ec326754d121..2d419eed3196 100644 --- a/packages/vendor-locker/src/NodeVendorLocker/ClassMethodParamVendorLockResolver.php +++ b/packages/vendor-locker/src/NodeVendorLocker/ClassMethodParamVendorLockResolver.php @@ -26,10 +26,8 @@ public function isVendorLocked(ClassMethod $classMethod, int $paramPosition): bo /** @var string $methodName */ $methodName = $this->nodeNameResolver->getName($classMethod); - // @todo extract to some "inherited parent method" service /** @var string|null $parentClassName */ $parentClassName = $classMethod->getAttribute(AttributeKey::PARENT_CLASS_NAME); - if ($parentClassName !== null) { $vendorLock = $this->isParentClassVendorLocking($paramPosition, $parentClassName, $methodName); if ($vendorLock !== null) { @@ -50,7 +48,6 @@ private function isParentClassVendorLocking(int $paramPosition, string $parentCl $parentClassNode = $this->parsedNodeCollector->findClass($parentClassName); if ($parentClassNode !== null) { $parentMethodNode = $parentClassNode->getMethod($methodName); - // @todo validate type is conflicting // parent class method in local scope → it's ok if ($parentMethodNode !== null) { // parent method has no type → we cannot change it here @@ -58,14 +55,11 @@ private function isParentClassVendorLocking(int $paramPosition, string $parentCl } } - // if not, look for it's parent parent - @todo recursion - + // if not, look for it's parent parent if (method_exists($parentClassName, $methodName)) { - // @todo validate type is conflicting // parent class method in external scope → it's not ok + // if not, look for it's parent parent return true; - - // if not, look for it's parent parent - @todo recursion } return null; diff --git a/packages/vendor-locker/src/NodeVendorLocker/ClassMethodReturnVendorLockResolver.php b/packages/vendor-locker/src/NodeVendorLocker/ClassMethodReturnVendorLockResolver.php index b76d5f103c3c..9947e65378e6 100644 --- a/packages/vendor-locker/src/NodeVendorLocker/ClassMethodReturnVendorLockResolver.php +++ b/packages/vendor-locker/src/NodeVendorLocker/ClassMethodReturnVendorLockResolver.php @@ -46,21 +46,19 @@ private function isVendorLockedByParentClass(string $parentClassName, string $me $parentClassNode = $this->parsedNodeCollector->findClass($parentClassName); if ($parentClassNode !== null) { $parentMethodNode = $parentClassNode->getMethod($methodName); - // @todo validate type is conflicting + // validate type is conflicting // parent class method in local scope → it's ok if ($parentMethodNode !== null) { return $parentMethodNode->returnType !== null; } - // if not, look for it's parent parent - @todo recursion + // if not, look for it's parent parent } if (method_exists($parentClassName, $methodName)) { - // @todo validate type is conflicting + // validate type is conflicting // parent class method in external scope → it's not ok return true; - - // if not, look for it's parent parent - @todo recursion } return false; From d8907d06e8c97b416359d89b9e48bbcfa2b095e9 Mon Sep 17 00:00:00 2001 From: TomasVotruba Date: Wed, 6 May 2020 00:03:07 +0200 Subject: [PATCH 2/4] drop helpers --- config/services.yaml | 1 - helpers/extract_class_changes_from_diff.php | 98 ---------- ...xtract_default_value_changes_from_diff.php | 169 ------------------ 3 files changed, 268 deletions(-) delete mode 100644 helpers/extract_class_changes_from_diff.php delete mode 100644 helpers/extract_default_value_changes_from_diff.php diff --git a/config/services.yaml b/config/services.yaml index 01f7466e5c44..d8bff01edd9c 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -8,7 +8,6 @@ services: exclude: - '../src/Rector/**/*Rector.php' - '../src/Testing/PHPUnit/*' - - '../src/Testing/Dumper/*' - '../src/RectorDefinition/*' - '../src/Exception/*' - '../src/DependencyInjection/CompilerPass/*' diff --git a/helpers/extract_class_changes_from_diff.php b/helpers/extract_class_changes_from_diff.php deleted file mode 100644 index 36700b21cb32..000000000000 --- a/helpers/extract_class_changes_from_diff.php +++ /dev/null @@ -1,98 +0,0 @@ -[^-].*?)$\n\+(?.*?)$#ms'; - - /** - * @var string - */ - private const CLASS_NAME_PREFIX = 'Doctrine'; - - /** - * @see https://regex101.com/r/tyABnc/2/ - * @var string - */ - private const CLASS_NAME_PATTERN = '#(?' . self::CLASS_NAME_PREFIX . '\\\\[\w|\\\\]+)#'; - - /** - * @var string[] - */ - private $classesBeforeAfter = []; - - public function run(string $diffFilePath): void - { - $diff = FileSystem::read($diffFilePath); - $beforeAfterMatches = Strings::matchAll($diff, self::BEFORE_AFTER_PATTERN); - - $classesBeforeAfter = []; - foreach ($beforeAfterMatches as $beforeAfterMatch) { - $classBeforeAndAfter = $this->resolveClassBeforeAndAfter($beforeAfterMatch); - if ($classBeforeAndAfter === null) { - continue; - } - - [$classBefore, $classAfter] = $classBeforeAndAfter; - - if (Strings::contains($classBefore, 'Tests')) { - continue; - } - - // classes are the same, no change in the class name - if ($classBefore === $classAfter) { - continue; - } - - $this->classesBeforeAfter[$classBefore] = $classAfter; - } - - $this->report(); - } - - private function report(): void - { - $this->classesBeforeAfter = array_unique($this->classesBeforeAfter); - ksort($this->classesBeforeAfter); - - $yaml = Yaml::dump($this->classesBeforeAfter); - echo PHP_EOL; - echo $yaml; - echo PHP_EOL; - } - - /** - * @return string[]|null - */ - private function resolveClassBeforeAndAfter(array $beforeAfterMatch): ?array - { - // file change - if (Strings::contains($beforeAfterMatch['before'], '//')) { - return null; - } - - $classNameBefore = Strings::match($beforeAfterMatch['before'], self::CLASS_NAME_PATTERN); - if ($classNameBefore === null) { - return null; - } - - $classNameAfter = Strings::match($beforeAfterMatch['after'], self::CLASS_NAME_PATTERN); - if ($classNameAfter === null) { - return null; - } - - return [$classNameBefore['class_name'], $classNameAfter['class_name']]; - } -} - -$classChangesDiffExtractor = new ClassChangesDiffExtractor(); -// $classChangesDiffExtractor->run(); diff --git a/helpers/extract_default_value_changes_from_diff.php b/helpers/extract_default_value_changes_from_diff.php deleted file mode 100644 index a3ab8f2de540..000000000000 --- a/helpers/extract_default_value_changes_from_diff.php +++ /dev/null @@ -1,169 +0,0 @@ -(.*?))\.php$#ms'; - - /** - * @see https://regex101.com/r/pe3DNc/1/ - * @var string - */ - private const REMOVED_DEFAULT_VALUE_PATTERN = '#^-(.*?)function\s(?\w+)(.*?)=\s?(?.*?)\)$#ms'; - - /** - * @var string|null - */ - private $currentClass; - - /** - * @var mixed[] - */ - private $collectedChanges = []; - - /** - * @var string[] - */ - private $newToOldClasses = []; - - /** - * @var string[] - */ - private const CLASSES_TO_SKIP = [ - 'PhpOffice\PhpSpreadsheet\Shared\JAMA\utils\Error', - 'PhpOffice\PhpSpreadsheet\Worksheet\Dimension', - ]; - - public function __construct() - { - $yaml = Yaml::parseFile(__DIR__ . '/../config/set/php-office/phpexcel-to-phpspreadsheet.yaml'); - $oldToNewClasses = $yaml['services']['Rector\Renaming\Rector\Class_\RenameClassRector']['$oldToNewClasses']; - $this->newToOldClasses = array_flip($oldToNewClasses); - - // extra map - @todo possibly check class name changes and add to config - $this->newToOldClasses['PhpOffice\PhpSpreadsheet\CalcEngine\Logger'] = 'PHPExcel_CalcEngine_Logger'; - $this->newToOldClasses['PhpOffice\PhpSpreadsheet\Calculation'] = 'PHPExcel_Calculation'; - $this->newToOldClasses['PhpOffice\PhpSpreadsheet\Cell'] = 'PHPExcel_Cell'; - $this->newToOldClasses['PhpOffice\PhpSpreadsheet\Chart'] = 'PHPExcel_Chart'; - $this->newToOldClasses['PhpOffice\PhpSpreadsheet\RichText'] = 'PHPExcel_RichText'; - $this->newToOldClasses['PhpOffice\PhpSpreadsheet\Style'] = 'PHPExcel_Style'; - $this->newToOldClasses['PhpOffice\PhpSpreadsheet\Worksheet'] = 'PHPExcel_Worksheet'; - $this->newToOldClasses['PhpOffice\PhpSpreadsheet\Writer\Pdf\Core'] = 'PHPExcel_Writer_PDF_Core'; - $this->newToOldClasses['PhpOffice\PhpSpreadsheet\Writer\Pdf\DomPDF'] = 'PHPExcel_Writer_PDF_DomPDF'; - $this->newToOldClasses['PhpOffice\PhpSpreadsheet\Writer\Pdf\MPDF'] = 'PHPExcel_Writer_PDF_mPDF'; - $this->newToOldClasses['PhpOffice\PhpSpreadsheet\Writer\Pdf\TcPDF'] = 'PHPExcel_Writer_PDF_tcPDF'; - } - - public function run(string $diffFilepath) - { - $lineByLineContent = $this->readFileToLines($diffFilepath); - - foreach ($lineByLineContent as $lineContent) { - $this->detectCurrentClass($lineContent); - - if (in_array($this->currentClass, self::CLASSES_TO_SKIP, true)) { - continue; - } - - $matches = Strings::match($lineContent, self::REMOVED_DEFAULT_VALUE_PATTERN); - if (! $matches) { - continue; - } - - // match args - $match = Strings::match($lineContent, '#\((?.*?)\)#'); - if(!isset($match['parameters'])) { - throw new ShouldNotHappenException(); - } - - $methodName = $matches['method_name']; - $value = $matches['value']; - - $parameters = Strings::split($match['parameters'], '#,\s+#'); - foreach ($parameters as $position => $parameterContent) { - $match = Strings::match($parameterContent, '#=\s+(?.*?)$#'); - if ($match === null) { - dump($lineContent); - dump($parameterContent); - continue; - } - - $value = $match['default_value']; - if ($value === 'null') { - $value = null; - } - - if ($value === 'false') { - $value = false; - } - - if ($value === 'true') { - $value = true; - } - - $this->collectedChanges[$this->currentClass][$methodName][$position] = $value; - } - } - - $this->reportCollectedChanges(); - } - - private function detectCurrentClass(string $fileContent): void - { - $matches = Strings::match($fileContent, self::FILE_PATTERN); - if (! $matches) { - return; - } - - // turn file into class - $class = Strings::replace($matches['file_name'], '#/#', '\\'); - $newClass = 'PhpOffice\\' . $class; - - $isClassSkipped = in_array($newClass, self::CLASSES_TO_SKIP, true); - if(! $isClassSkipped && ! isset($this->newToOldClasses[$newClass])) { - throw new ShouldNotHappenException(sprintf('Could not find old class for "%s"', $newClass)); - } - - $oldClass = $this->newToOldClasses[$newClass] ?? $newClass; - $this->currentClass = $oldClass; - } - - /** - * @return string[] - */ - private function readFileToLines(string $diffFilepath): array - { - $fileContent = FileSystem::read($diffFilepath); - return explode(PHP_EOL, $fileContent); - } - - private function reportCollectedChanges(): void - { - ksort($this->collectedChanges); - - $output = Json::encode($this->collectedChanges, Json::PRETTY); - echo PHP_EOL; - echo $output; - echo PHP_EOL; - - FileSystem::write('output.json', $output); - } -} - -$removedDefaultValueDiffExtractor = new RemovedDefaultValueDiffExtractor(); -$removedDefaultValueDiffExtractor->run(__DIR__ . '/../workhouse/some.diff'); From 65a25fc57e40dd1736261d3454c651abf4a8021f Mon Sep 17 00:00:00 2001 From: TomasVotruba Date: Wed, 6 May 2020 12:52:50 +0200 Subject: [PATCH 3/4] fix comment preserving for SimplifyIfReturnBoolRector --- .../Comment/MergedNodeCommentPreserver.php | 26 ++++++++++++++ .../Rector/If_/SimplifyIfReturnBoolRector.php | 27 +++++++------- .../Fixture/inner_comment_up.php.inc | 35 +++++++++++++++++++ .../fixture3291_keep_private_property.php.inc | 4 +-- 4 files changed, 74 insertions(+), 18 deletions(-) create mode 100644 packages/better-php-doc-parser/src/Comment/MergedNodeCommentPreserver.php create mode 100644 rules/code-quality/tests/Rector/If_/SimplifyIfReturnBoolRector/Fixture/inner_comment_up.php.inc diff --git a/packages/better-php-doc-parser/src/Comment/MergedNodeCommentPreserver.php b/packages/better-php-doc-parser/src/Comment/MergedNodeCommentPreserver.php new file mode 100644 index 000000000000..3086784b1ebc --- /dev/null +++ b/packages/better-php-doc-parser/src/Comment/MergedNodeCommentPreserver.php @@ -0,0 +1,26 @@ +getComments()); + } + + if ($comments === []) { + return; + } + + $comments = array_merge($newNode->getComments(), $comments); + $newNode->setAttribute('comments', $comments); + } +} diff --git a/rules/code-quality/src/Rector/If_/SimplifyIfReturnBoolRector.php b/rules/code-quality/src/Rector/If_/SimplifyIfReturnBoolRector.php index c8bcfa9a5b0a..1508f79e9e1e 100644 --- a/rules/code-quality/src/Rector/If_/SimplifyIfReturnBoolRector.php +++ b/rules/code-quality/src/Rector/If_/SimplifyIfReturnBoolRector.php @@ -17,6 +17,7 @@ use PHPStan\Type\BooleanType; use PHPStan\Type\Type; use PHPStan\Type\UnionType; +use Rector\BetterPhpDocParser\Comment\MergedNodeCommentPreserver; use Rector\Core\Rector\AbstractRector; use Rector\Core\RectorDefinition\CodeSample; use Rector\Core\RectorDefinition\RectorDefinition; @@ -34,9 +35,17 @@ final class SimplifyIfReturnBoolRector extends AbstractRector */ private $staticTypeAnalyzer; - public function __construct(StaticTypeAnalyzer $staticTypeAnalyzer) - { + /** + * @var MergedNodeCommentPreserver + */ + private $mergedNodeCommentPreserver; + + public function __construct( + StaticTypeAnalyzer $staticTypeAnalyzer, + MergedNodeCommentPreserver $mergedNodeCommentPreserver + ) { $this->staticTypeAnalyzer = $staticTypeAnalyzer; + $this->mergedNodeCommentPreserver = $mergedNodeCommentPreserver; } public function getDefinition(): RectorDefinition @@ -94,7 +103,7 @@ public function refactor(Node $node): ?Node return null; } - $this->keepComments($node, $nextNode, $newReturnNode); + $this->mergedNodeCommentPreserver->keepComments($newReturnNode, $node, $ifInnerNode, $nextNode, $newReturnNode); $this->removeNode($nextNode); return $newReturnNode; @@ -163,18 +172,6 @@ private function processReturnFalse(If_ $ifNode, Return_ $nextReturnNode): ?Retu return new Return_($this->boolCastOrNullCompareIfNeeded(new BooleanNot($ifNode->cond))); } - private function keepComments(Node $oldNode, Node $nextNode, Node $newNode): void - { - /** @var Node $node */ - foreach ([$oldNode, $nextNode] as $node) { - $newNode->setAttribute('comments', array_merge($newNode->getComments(), $node->getComments())); - } - - if ($nextNode->getDocComment() !== null) { - $newNode->setDocComment($nextNode->getDocComment()); - } - } - private function isIfWithSingleReturnExpr(If_ $if): bool { if (count($if->stmts) !== 1) { diff --git a/rules/code-quality/tests/Rector/If_/SimplifyIfReturnBoolRector/Fixture/inner_comment_up.php.inc b/rules/code-quality/tests/Rector/If_/SimplifyIfReturnBoolRector/Fixture/inner_comment_up.php.inc new file mode 100644 index 000000000000..e0b53c5ab995 --- /dev/null +++ b/rules/code-quality/tests/Rector/If_/SimplifyIfReturnBoolRector/Fixture/inner_comment_up.php.inc @@ -0,0 +1,35 @@ + +----- + diff --git a/tests/Issues/Issue3291/Fixture/fixture3291_keep_private_property.php.inc b/tests/Issues/Issue3291/Fixture/fixture3291_keep_private_property.php.inc index c60d422492e0..6cc9951efa2e 100644 --- a/tests/Issues/Issue3291/Fixture/fixture3291_keep_private_property.php.inc +++ b/tests/Issues/Issue3291/Fixture/fixture3291_keep_private_property.php.inc @@ -2,7 +2,6 @@ namespace Rector\Core\Tests\Issues\Issue3291\Fixture; - class Fixture3291KeepPrivateProperty { private $unusedProperty = []; @@ -15,12 +14,11 @@ class Fixture3291KeepPrivateProperty } ?> - ----- +----- Date: Wed, 6 May 2020 12:58:48 +0200 Subject: [PATCH 4/4] keep comments removed with related-stmt --- .../ClassMethodReturnVendorLockResolver.php | 10 +++----- .../keep_comments.php.inc | 23 ------------------- .../RemoveDeadStmtRectorTest.php | 1 - 3 files changed, 3 insertions(+), 31 deletions(-) delete mode 100644 rules/dead-code/tests/Rector/Stmt/RemoveDeadStmtRector/FixtureRemovedComments/keep_comments.php.inc diff --git a/packages/vendor-locker/src/NodeVendorLocker/ClassMethodReturnVendorLockResolver.php b/packages/vendor-locker/src/NodeVendorLocker/ClassMethodReturnVendorLockResolver.php index 9947e65378e6..0bdd493972d6 100644 --- a/packages/vendor-locker/src/NodeVendorLocker/ClassMethodReturnVendorLockResolver.php +++ b/packages/vendor-locker/src/NodeVendorLocker/ClassMethodReturnVendorLockResolver.php @@ -55,12 +55,8 @@ private function isVendorLockedByParentClass(string $parentClassName, string $me // if not, look for it's parent parent } - if (method_exists($parentClassName, $methodName)) { - // validate type is conflicting - // parent class method in external scope → it's not ok - return true; - } - - return false; + // validate type is conflicting + // parent class method in external scope → it's not ok + return method_exists($parentClassName, $methodName); } } diff --git a/rules/dead-code/tests/Rector/Stmt/RemoveDeadStmtRector/FixtureRemovedComments/keep_comments.php.inc b/rules/dead-code/tests/Rector/Stmt/RemoveDeadStmtRector/FixtureRemovedComments/keep_comments.php.inc deleted file mode 100644 index f21108b801ae..000000000000 --- a/rules/dead-code/tests/Rector/Stmt/RemoveDeadStmtRector/FixtureRemovedComments/keep_comments.php.inc +++ /dev/null @@ -1,23 +0,0 @@ - ------ - diff --git a/rules/dead-code/tests/Rector/Stmt/RemoveDeadStmtRector/RemoveDeadStmtRectorTest.php b/rules/dead-code/tests/Rector/Stmt/RemoveDeadStmtRector/RemoveDeadStmtRectorTest.php index c1aedf9cb345..b17e45416451 100644 --- a/rules/dead-code/tests/Rector/Stmt/RemoveDeadStmtRector/RemoveDeadStmtRectorTest.php +++ b/rules/dead-code/tests/Rector/Stmt/RemoveDeadStmtRector/RemoveDeadStmtRectorTest.php @@ -28,7 +28,6 @@ public function provideData(): Iterator */ public function testKeepComments(string $file): void { - $this->markTestSkipped('Temporary skip removed docs'); $this->doTestFile($file); }