diff --git a/.gitignore b/.gitignore index 1cadc9a..996e6f6 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,7 @@ refactor.phar .pear build/ composer.phar + +/php-refactoring-browser.sublime-project + +/php-refactoring-browser.sublime-workspace diff --git a/README.md b/README.md index 4d4e848..ff09d2b 100644 --- a/README.md +++ b/README.md @@ -90,6 +90,15 @@ the command to fix class and namespaces. php refactor.phar fix-class-names +### Optimize use statements + +Optimizes the use of Fully qualified names in a file so that FQN is imported with +"use" at the top of the file and the FQN is replaced with its classname. + +All other use statements will be untouched, only new ones will be added. + + php refactor.phar optimize-use + ## Roadmap Not prioritized. diff --git a/features/bootstrap/FeatureContext.php b/features/bootstrap/FeatureContext.php index 9dab76f..5e4fb1e 100644 --- a/features/bootstrap/FeatureContext.php +++ b/features/bootstrap/FeatureContext.php @@ -99,7 +99,7 @@ public function iUseRefactoringWith($refactoringName, TableNode $table) public function thePhpFileShouldBeRefactored($file, PyStringNode $expectedPatch) { $output = array_map('trim', explode("\n", rtrim($this->output))); - $expectedPatch = array_map('trim', explode("\n", rtrim((string)$expectedPatch))); + $expectedPatch = $this->formatExpectedPatch((string)$expectedPatch); assertEquals( $expectedPatch, $output, @@ -107,4 +107,38 @@ public function thePhpFileShouldBeRefactored($file, PyStringNode $expectedPatch) "Diff:\n" . print_r(array_diff($expectedPatch, $output), true) ); } + + /** + * converts / paths in expectedPatch text to \ paths + * + * leaves the a/ b/ slashes untouched + * returns an array of lines + * @return array + */ + protected function formatExpectedPatch($patch) + { + if ('\\' === DIRECTORY_SEPARATOR) { + $formatLine = function ($line) { + $line = preg_replace_callback('~^((?:---|\+\+\+)\s*(?:a|b)/)(.*)~', function ($match) { + list($all, $diff, $path) = $match; + + if (0 === preg_match('~^[a-z]+://~i', $path)) { + $path = str_replace('/', '\\', $path); + } + + return $diff.$path; + + }, $line); + + return trim($line); + }; + + } else { + $formatLine = function ($line) { + return trim($line); + }; + } + + return array_map($formatLine, explode("\n", rtrim($patch))); + } } diff --git a/features/optimize_use.feature b/features/optimize_use.feature new file mode 100644 index 0000000..4fb767c --- /dev/null +++ b/features/optimize_use.feature @@ -0,0 +1,50 @@ +Feature: Optimize use + To optimize the use statements in my code + As a developer + I need to convert every FQN in the code to a use statement in the file + + Scenario: Convert FQN and leave relative names + Given a PHP File named "src/Foo.php" with: + """ + operation(new \Bar\Qux\Adapter($flag)); + + return $service; + } + } + """ + When I use refactoring "optimize-use" with: + | arg | value | + | file | src/Foo.php | + Then the PHP File "src/Foo.php" should be refactored: + """ + --- a/vfs://project/src/Foo.php + +++ b/vfs://project/src/Foo.php + @@ -3,5 +3,6 @@ + namespace Bar; + + use Bar\Baz\Service; + +use Bar\Qux\Adapter; + + class Foo + @@ -12,5 +12,5 @@ + + $service = new Service(); + - $service->operation(new \Bar\Qux\Adapter($flag)); + + $service->operation(new Adapter($flag)); + + return $service; + """ + diff --git a/src/main/QafooLabs/Refactoring/Adapters/PHPParser/ParserPhpNameScanner.php b/src/main/QafooLabs/Refactoring/Adapters/PHPParser/ParserPhpNameScanner.php index 20e3606..1c87727 100644 --- a/src/main/QafooLabs/Refactoring/Adapters/PHPParser/ParserPhpNameScanner.php +++ b/src/main/QafooLabs/Refactoring/Adapters/PHPParser/ParserPhpNameScanner.php @@ -5,6 +5,8 @@ use QafooLabs\Refactoring\Adapters\PHPParser\Visitor\PhpNameCollector; use QafooLabs\Refactoring\Domain\Services\PhpNameScanner; use QafooLabs\Refactoring\Domain\Model\File; +use QafooLabs\Refactoring\Domain\Model\LineRange; +use QafooLabs\Refactoring\Domain\Model\UseStatement; use QafooLabs\Refactoring\Domain\Model\PhpName; use PHPParser_Parser; @@ -24,8 +26,25 @@ public function findNames(File $file) $traverser->addVisitor($collector); $traverser->traverse($stmts); - return array_map(function ($use) use ($file) { - return new PhpName($use['fqcn'], $use['alias'], $file, $use['line']); + return array_map(function ($name) use ($file) { + return new PhpName( + $name['fqcn'], + $name['alias'], + $file, + $name['line'], + isset($name['parent']) + ? $this->createParent($name['parent'], $file) + : null + ); + }, $collector->collectedNameDeclarations()); } + + private function createParent(Array $parent, File $file) + { + switch ($parent['type']) { + case 'use': + return new UseStatement($file, LineRange::fromLines($parent['lines'][0], $parent['lines'][1])); + } + } } diff --git a/src/main/QafooLabs/Refactoring/Adapters/PHPParser/Visitor/PhpNameCollector.php b/src/main/QafooLabs/Refactoring/Adapters/PHPParser/Visitor/PhpNameCollector.php index 00eceb7..bedc80b 100644 --- a/src/main/QafooLabs/Refactoring/Adapters/PHPParser/Visitor/PhpNameCollector.php +++ b/src/main/QafooLabs/Refactoring/Adapters/PHPParser/Visitor/PhpNameCollector.php @@ -33,23 +33,35 @@ class PhpNameCollector extends \PHPParser_NodeVisitorAbstract public function enterNode(PHPParser_Node $node) { - if ($node instanceof PHPParser_Node_Stmt_UseUse) { - $name = implode('\\', $node->name->parts); - $this->useStatements[$node->alias] = $name; - $this->nameDeclarations[] = array( - 'alias' => $name, - 'fqcn' => $name, - 'line' => $node->getLine() + if ($node instanceof PHPParser_Node_Stmt_Use) { + $parent = array( + 'type' => 'use', + 'lines' => array($node->getAttribute('startLine'), $node->getAttribute('endLine')) ); + + foreach ($node->uses as $use) { + if ($use instanceof PHPParser_Node_Stmt_UseUse) { + $name = implode('\\', $use->name->parts); + + $this->useStatements[$use->alias] = $name; + $this->nameDeclarations[] = array( + 'alias' => $name, + 'fqcn' => $name, + 'line' => $use->getLine(), + 'parent' => $parent + ); + } + } } + if ($node instanceof PHPParser_Node_Expr_New && $node->class instanceof PHPParser_Node_Name) { $usedAlias = implode('\\', $node->class->parts); $this->nameDeclarations[] = array( 'alias' => $usedAlias, - 'fqcn' => $this->fullyQualifiedNameFor($usedAlias), + 'fqcn' => $this->fullyQualifiedNameFor($usedAlias, $node->class->isFullyQualified()), 'line' => $node->getLine(), ); } @@ -59,7 +71,7 @@ public function enterNode(PHPParser_Node $node) $this->nameDeclarations[] = array( 'alias' => $usedAlias, - 'fqcn' => $this->fullyQualifiedNameFor($usedAlias), + 'fqcn' => $this->fullyQualifiedNameFor($usedAlias, $node->class->isFullyQualified()), 'line' => $node->getLine(), ); } @@ -70,7 +82,7 @@ public function enterNode(PHPParser_Node $node) $this->nameDeclarations[] = array( 'alias' => $usedAlias, - 'fqcn' => $this->fullyQualifiedNameFor($usedAlias), + 'fqcn' => $this->fullyQualifiedNameFor($usedAlias, $node->extends->isFullyQualified()), 'line' => $node->extends->getLine(), ); } @@ -80,7 +92,7 @@ public function enterNode(PHPParser_Node $node) $this->nameDeclarations[] = array( 'alias' => $usedAlias, - 'fqcn' => $this->fullyQualifiedNameFor($usedAlias), + 'fqcn' => $this->fullyQualifiedNameFor($usedAlias, $implement->isFullyQualified()), 'line' => $implement->getLine(), ); } @@ -92,11 +104,11 @@ public function enterNode(PHPParser_Node $node) } } - private function fullyQualifiedNameFor($alias) + private function fullyQualifiedNameFor($alias, $isFullyQualified) { $isAbsolute = $alias[0] === "\\"; - if ($isAbsolute) { + if ($isAbsolute || $isFullyQualified) { $class = $alias; } else if (isset($this->useStatements[$alias])) { $class = $this->useStatements[$alias]; diff --git a/src/main/QafooLabs/Refactoring/Adapters/Symfony/CliApplication.php b/src/main/QafooLabs/Refactoring/Adapters/Symfony/CliApplication.php index 5736832..99bdcc6 100644 --- a/src/main/QafooLabs/Refactoring/Adapters/Symfony/CliApplication.php +++ b/src/main/QafooLabs/Refactoring/Adapters/Symfony/CliApplication.php @@ -42,6 +42,7 @@ protected function getDefaultCommands() $commands[] = new Commands\RenameLocalVariableCommand(); $commands[] = new Commands\ConvertLocalToInstanceVariableCommand(); $commands[] = new Commands\FixClassNamesCommand(); + $commands[] = new Commands\OptimizeUseCommand(); return $commands; } diff --git a/src/main/QafooLabs/Refactoring/Adapters/Symfony/Commands/OptimizeUseCommand.php b/src/main/QafooLabs/Refactoring/Adapters/Symfony/Commands/OptimizeUseCommand.php new file mode 100644 index 0000000..f842898 --- /dev/null +++ b/src/main/QafooLabs/Refactoring/Adapters/Symfony/Commands/OptimizeUseCommand.php @@ -0,0 +1,84 @@ +setName('optimize-use') + ->setDescription('Optimize use statements of a file. Replace FQNs with imported aliases.') + ->addArgument('file', InputArgument::REQUIRED, 'File that contains the use statements to optimize') + ->setHelp(<<Operations: + +1. import found FQNs +2. replace FQNs with the imported classname + +Pre-Conditions: + +1. File has a single namespace defined + +Known issues: + +1. a FQN might be renamed with an conflicting name when the className of the renamend full qualified name is already in use +2. if there is no use statement in the whole file, new ones will be appended after the namespace + +Usage: + + php refactor.phar optimize-use file.php + +Will optimize the use statements in file.php. +HELP + ); + ; + } + + protected function execute(InputInterface $input, OutputInterface $output) + { + $file = File::createFromPath($input->getArgument('file'), getcwd()); + + $codeAnalysis = new StaticCodeAnalysis(); + $editor = new PatchEditor(new OutputPatchCommand($output)); + $phpNameScanner = new ParserPhpNameScanner(); + + $optimizeUse = new OptimizeUse($codeAnalysis, $editor, $phpNameScanner); + $optimizeUse->refactor($file); + } +} diff --git a/src/main/QafooLabs/Refactoring/Application/OptimizeUse.php b/src/main/QafooLabs/Refactoring/Application/OptimizeUse.php new file mode 100644 index 0000000..de486ad --- /dev/null +++ b/src/main/QafooLabs/Refactoring/Application/OptimizeUse.php @@ -0,0 +1,68 @@ +codeAnalysis = $codeAnalysis; + $this->editor = $editor; + $this->phpNameScanner = $phpNameScanner; + } + + public function refactor(File $file) + { + $classes = $this->codeAnalysis->findClasses($file); + $names = $this->phpNameScanner->findNames($file); + $class = $classes[0]; + + $lastUseStatement = null; + $usedNames = array(); + $fqcns = array(); + foreach ($names as $name) { + if ($name->isUse()) { + $lastUseStatement = $name->parent(); + $usedNames[] = $name->fullyQualifiedName(); + } elseif ($name->isFullyQualified()) { + $fqcns[] = $name; + } + } + $lastUseStatementLine = null !== $lastUseStatement ? $lastUseStatement->getEndLine() : $class->getNamespaceDeclarationLine()+2; + + if (count($fqcns) > 0) { + + $buffer = $this->editor->openBuffer($file); + foreach ($fqcns as $name) { + $buffer->replaceString($name->declaredLine(), '\\'.$name->fullyQualifiedName(), $name->shortname()); + + if (!in_array($name->fullyQualifiedName(), $usedNames)) { + $buffer->append($lastUseStatementLine, array(sprintf('use %s;', $name->fullyQualifiedName()))); + $lastUseStatementLine++; + } + } + + $this->editor->save(); + } + } +} diff --git a/src/main/QafooLabs/Refactoring/Domain/Model/File.php b/src/main/QafooLabs/Refactoring/Domain/Model/File.php index d63e258..88b9afa 100644 --- a/src/main/QafooLabs/Refactoring/Domain/Model/File.php +++ b/src/main/QafooLabs/Refactoring/Domain/Model/File.php @@ -35,7 +35,8 @@ public static function createFromPath($path, $workingDirectory) } $code = file_get_contents($path); - $relativePath = ltrim(str_replace($workingDirectory, "", $path), "/"); + $workingDirectory = rtrim($workingDirectory, '/\\'); + $relativePath = ltrim(str_replace($workingDirectory, "", $path), "/\\"); return new self($relativePath, $code); } diff --git a/src/main/QafooLabs/Refactoring/Domain/Model/PhpName.php b/src/main/QafooLabs/Refactoring/Domain/Model/PhpName.php index fd60c0e..0dff8fb 100644 --- a/src/main/QafooLabs/Refactoring/Domain/Model/PhpName.php +++ b/src/main/QafooLabs/Refactoring/Domain/Model/PhpName.php @@ -15,17 +15,20 @@ class PhpName { + private $fullyQualifiedName; private $relativeName; private $file; private $declaredLine; + private $parent; - public function __construct($fullyQualifiedName, $relativeName, File $file = null, $declaredLine = null) + public function __construct($fullyQualifiedName, $relativeName, File $file = null, $declaredLine = null, $parent = null) { $this->fullyQualifiedName = $fullyQualifiedName; $this->relativeName = $relativeName; $this->file = $file; $this->declaredLine = $declaredLine; + $this->parent = $parent; } /** @@ -105,9 +108,46 @@ public function relativeName() return $this->relativeName; } + public function parent() + { + return $this->parent; + } + + /** + * @return bool + */ public function equals(PhpName $other) { return $this->fullyQualifiedName === $other->fullyQualifiedName && $this->relativeName === $other->relativeName; } + + /** + * Is the relative name fully qualified ? + * + * @return bool + */ + public function isFullyQualified() + { + return $this->fullyQualifiedName === $this->relativeName; + } + + /** + * Is the php name found in a use statement? + * + * @return bool + */ + public function isUse() + { + return $this->parent instanceof UseStatement; + } + + /** + * @return string + */ + public function shortName() + { + $parts = explode("\\", $this->fullyQualifiedName); + return end($parts); + } } diff --git a/src/main/QafooLabs/Refactoring/Domain/Model/UseStatement.php b/src/main/QafooLabs/Refactoring/Domain/Model/UseStatement.php new file mode 100644 index 0000000..e9fee18 --- /dev/null +++ b/src/main/QafooLabs/Refactoring/Domain/Model/UseStatement.php @@ -0,0 +1,39 @@ +file = $file; + $this->declaredLines = $declaredLines; + } + + public function getEndLine() + { + return $this->declaredLines->getEnd(); + } +} diff --git a/src/test/QafooLabs/Refactoring/Adapters/PHPParser/ParserPhpNameScannerTest.php b/src/test/QafooLabs/Refactoring/Adapters/PHPParser/ParserPhpNameScannerTest.php index f29056a..a915180 100644 --- a/src/test/QafooLabs/Refactoring/Adapters/PHPParser/ParserPhpNameScannerTest.php +++ b/src/test/QafooLabs/Refactoring/Adapters/PHPParser/ParserPhpNameScannerTest.php @@ -4,6 +4,8 @@ use QafooLabs\Refactoring\Domain\Model\File; use QafooLabs\Refactoring\Domain\Model\PhpName; +use QafooLabs\Refactoring\Domain\Model\UseStatement; +use QafooLabs\Refactoring\Domain\Model\LineRange; class ParserPhpNameScannerTest extends \PHPUnit_Framework_TestCase { @@ -13,16 +15,103 @@ public function testFindNames() $scanner = new ParserPhpNameScanner(); $names = $scanner->findNames($file); + $useStmt = function ($line) use ($file) { + return new UseStatement($file, LineRange::fromSingleLine($line)); + }; + $this->assertEquals( array( - new PhpName('QafooLabs\Refactoring\Domain\Model\File', 'QafooLabs\Refactoring\Domain\Model\File', $file, 5), - new PhpName('QafooLabs\Refactoring\Domain\Model\PhpName', 'QafooLabs\Refactoring\Domain\Model\PhpName', $file, 6), - new PhpName('QafooLabs\Refactoring\Adapters\PHPParser\PHPUnit_Framework_TestCase', 'PHPUnit_Framework_TestCase', $file, 8), - new PhpName('QafooLabs\Refactoring\Domain\Model\File', 'File', $file, 12), - new PhpName('QafooLabs\Refactoring\Adapters\PHPParser\ParserPhpNameScanner', 'ParserPhpNameScanner', $file, 13), + new PhpName('QafooLabs\Refactoring\Domain\Model\File', 'QafooLabs\Refactoring\Domain\Model\File', $file, 5, $useStmt(5)), + new PhpName('QafooLabs\Refactoring\Domain\Model\PhpName', 'QafooLabs\Refactoring\Domain\Model\PhpName', $file, 6, $useStmt(6)), + new PhpName('QafooLabs\Refactoring\Domain\Model\UseStatement', 'QafooLabs\Refactoring\Domain\Model\UseStatement', $file, 7, $useStmt(7)), + new PhpName('QafooLabs\Refactoring\Domain\Model\LineRange', 'QafooLabs\Refactoring\Domain\Model\LineRange', $file, 8, $useStmt(8)), + new PhpName('PHPUnit_Framework_TestCase', 'PHPUnit_Framework_TestCase', $file, 10), + new PhpName('QafooLabs\Refactoring\Domain\Model\File', 'File', $file, 14), + new PhpName('QafooLabs\Refactoring\Adapters\PHPParser\ParserPhpNameScanner', 'ParserPhpNameScanner', $file, 15), ), - array_slice($names, 0, 5) + array_slice($names, 0, 7) ); } + + public function testRegressionFindNamesDetectsFQCNCorrectly() + { + $file = new File("Fqcn.php", <<<'PHP' +findNames($file); + + $this->assertEquals( + array( + new PhpName('Bar\Qux\Adapter', 'Bar\Qux\Adapter', $file, 9) + ), + $names + ); + } + + + public function testFindNamesFindsParentForPhpNameInSingleLineUseStatement() + { + $file = new File("Fqcn.php", <<<'PHP' +findNames($file); + + $this->assertEquals( + array( + new PhpName( + 'Bar\Qux\Adapter', + 'Bar\Qux\Adapter', + $file, + 3, + new UseStatement( + $file, + LineRange::fromSingleLine(3) + ) + ) + ), + $names + ); + } + + public function testFindNamesFindsParentForPhpNameInMultiLineUseStatement() + { + $file = new File("Fqcn.php", <<<'PHP' +findNames($file); + + $use = new UseStatement($file, LineRange::fromLines(3, 4)); + + $this->assertEquals( + array( + new PhpName('Bar\Qux\Adapter', 'Bar\Qux\Adapter', $file, 3, $use), + new PhpName('Bar\Qux\Foo', 'Bar\Qux\Foo', $file, 4, $use), + ), + $names + ); + } +} diff --git a/src/test/QafooLabs/Refactoring/Domain/Model/FileTest.php b/src/test/QafooLabs/Refactoring/Domain/Model/FileTest.php new file mode 100644 index 0000000..34c2f45 --- /dev/null +++ b/src/test/QafooLabs/Refactoring/Domain/Model/FileTest.php @@ -0,0 +1,38 @@ +root = vfsStream::setup('project', 0644, + array( + 'src'=> + array( + 'Foo'=> + array( + 'Bar.php'=>'' + ) + ) + ) + ); + } + + public function testGetRelativePathRespectsMixedWindowsPathsAndWorkingDirectoryTrailingSlashs() + { + $workingDir = $this->root->getChild('src')->url().'/'; + + $file = File::createFromPath( + $this->root->getChild('src')->url().'\Foo\Bar.php', + $workingDir + ); + + $this->assertEquals("Foo\Bar.php", $file->getRelativePath()); + } +} diff --git a/src/test/QafooLabs/Refactoring/Domain/Model/PhpNameTest.php b/src/test/QafooLabs/Refactoring/Domain/Model/PhpNameTest.php index 715fb36..7485744 100644 --- a/src/test/QafooLabs/Refactoring/Domain/Model/PhpNameTest.php +++ b/src/test/QafooLabs/Refactoring/Domain/Model/PhpNameTest.php @@ -81,4 +81,43 @@ public function testRegression3() $this->assertEquals('Foo\\Bar\\Foo', $changed->fullyQualifiedName()); $this->assertEquals('Foo\\Bar\\Foo', $changed->relativeName()); } + + /** + * @dataProvider provideIsFullyQualified + */ + public function testIsFullyQualified($fqcn, $relativeName, $expected = TRUE) + { + $name = new PHPName($fqcn, $relativeName); + + $this->assertEquals($expected, $name->isFullyQualified()); + } + + public static function provideIsFullyQualified() + { + $tests = array(); + + $tests[] = array('Foo', 'Foo', TRUE); + $tests[] = array('Foo\\Bar\\Foo', 'Foo\\Bar\\Foo', TRUE); + + $tests[] = array('Foo\\Bar\\Foo', 'Foo', FALSE); + $tests[] = array('Foo\\Bar\\Foo', 'Bar\\Foo', FALSE); + + return $tests; + } + + public function testGetShortNameReturnsLastPartForFQCN() + { + $name = new PhpName('Foo\\Bar', "Foo\\Bar", null, null); + $short = new PhpName("Foo", "Foo", null, null); + + $this->assertEquals('Bar', $name->shortName()); + $this->assertEquals('Foo', $short->shortName()); + } + + public function testIsUseStatementWhenParentIsAUseStatement() + { + $name = new PhpName('Foo\\Bar', 'Foo\\Bar', null, null, new UseStatement()); + + $this->assertTrue($name->isUse()); + } } diff --git a/src/test/QafooLabs/Refactoring/Domain/Model/UseStatementTest.php b/src/test/QafooLabs/Refactoring/Domain/Model/UseStatementTest.php new file mode 100644 index 0000000..9af3b7c --- /dev/null +++ b/src/test/QafooLabs/Refactoring/Domain/Model/UseStatementTest.php @@ -0,0 +1,21 @@ +useStatement = new UseStatement($file, LineRange::fromLines(3,5)); + } + + public function testReturnsEndLineFromLineRange() + { + $this->assertEquals(5, $this->useStatement->getEndLine()); + } +}