From ea83068deb9a14c5a9c5188abeaf6e92266ef24a Mon Sep 17 00:00:00 2001 From: Barney Laurance Date: Sun, 7 Jul 2019 13:55:53 +0100 Subject: [PATCH] Allow resolving directories from config file location (continued) (#1910) * Extract function getPsalmHelpText() from psalm.php * Extract initialiseConfig from psalm.php * Add -c as valid short option for psalter and psalm-refactor * Use initialiseConfig in psalter, psalm-refactor and psalm-language-server as well as psalm * Rely on psalm --alter resolving directory from config file in test * Remove erronous condition for config file path This code was based on me wrongly thinking that the config file location was seprated from the argument name with a space instead of an equals sign * Use config dir as current dir in psalm and psalm-refactor, as with psalter and psalm-language-server * Remove redundant duplicated code * Refactor: move calls to \Psalm\Config::setComposerClassLoader inside initialiseConfig * PHPCS fix * Extract function get_path_to_config from command scripts * Refactor - extract functions from \Psalm\Config::loadFromXML * Refactor - reduce verbosity of config loading code * Allow running e2e tests on windows * Fix testCompactReport on Windows * convert line endings to make testCompactReport pass on Windows --- src/Psalm/Config.php | 182 ++++++++++----------------- src/command_functions.php | 159 ++++++++++++++++++++++- src/psalm-language-server.php | 24 +--- src/psalm-refactor.php | 22 ++-- src/psalm.php | 136 +------------------- src/psalter.php | 21 ++-- tests/EndToEnd/PsalmEndToEndTest.php | 12 +- tests/ReportOutputTest.php | 12 +- 8 files changed, 261 insertions(+), 307 deletions(-) diff --git a/src/Psalm/Config.php b/src/Psalm/Config.php index e017ac55a..b8e554e5f 100644 --- a/src/Psalm/Config.php +++ b/src/Psalm/Config.php @@ -563,17 +563,13 @@ class Config /** * Creates a new config object from an XML string * + * @throws ConfigException + * * @param string $base_dir * @param string $file_contents * @param string|null $current_dir Current working directory, if different to $base_dir * * @return self - * @psalm-suppress MixedArgument - * @psalm-suppress MixedPropertyFetch - * @psalm-suppress MixedMethodCall - * @psalm-suppress MixedAssignment - * @psalm-suppress MixedOperand - * @psalm-suppress MixedPropertyAssignment */ public static function loadFromXML($base_dir, $file_contents, $current_dir = null) { @@ -581,8 +577,17 @@ class Config $current_dir = $base_dir; } - $config = new static(); + self::validateXmlConfig($file_contents); + return self::fromXmlAndPaths($base_dir, $file_contents, $current_dir); + } + + + /** + * @throws ConfigException + */ + private static function validateXmlConfig(string $file_contents): void + { $schema_path = dirname(dirname(__DIR__)) . '/config.xsd'; if (!file_exists($schema_path)) { @@ -625,36 +630,60 @@ class Config } libxml_clear_errors(); } + } + + + /** + * @psalm-suppress MixedMethodCall + * @psalm-suppress MixedAssignment + * @psalm-suppress MixedOperand + * @psalm-suppress MixedPropertyAssignment + * @psalm-suppress MixedArgument + * @psalm-suppress MixedPropertyFetch + * + * @throws ConfigException + */ + private static function fromXmlAndPaths(string $base_dir, string $file_contents, string $current_dir): self + { + $config = new static(); $config_xml = new SimpleXMLElement($file_contents); - if (isset($config_xml['useDocblockTypes'])) { - $attribute_text = (string) $config_xml['useDocblockTypes']; - $config->use_docblock_types = $attribute_text === 'true' || $attribute_text === '1'; + $booleanAttributes = [ + 'useDocblockTypes' => 'use_docblock_types', + 'useDocblockPropertyTypes' => 'use_docblock_property_types', + 'throwExceptionOnError' => 'throw_exception', + 'hideExternalErrors' => 'hide_external_errors', + 'resolveFromConfigFile' => 'resolve_from_config_file', + 'allowFileIncludes' => 'allow_includes', + 'totallyTyped' => 'totally_typed', + 'strictBinaryOperands' => 'strict_binary_operands', + 'requireVoidReturnType' => 'add_void_docblocks', + 'useAssertForType' => 'use_assert_for_type', + 'rememberPropertyAssignmentsAfterCall' => 'remember_property_assignments_after_call', + 'allowPhpStormGenerics' => 'allow_phpstorm_generics', + 'allowStringToStandInForClass' => 'allow_string_standin_for_class', + 'usePhpDocMethodsWithoutMagicCall' => 'use_phpdoc_method_without_magic_or_parent', + 'memoizeMethodCallResults' => 'memoize_method_calls', + 'hoistConstants' => 'hoist_constants', + 'addParamDefaultToDocblockType' => 'add_param_default_to_docblock_type', + 'checkForThrowsDocblock' => 'check_for_throws_docblock', + 'checkForThrowsInGlobalScope' => 'check_for_throws_in_global_scope', + 'forbidEcho' => 'forbid_echo', + 'ignoreInternalFunctionFalseReturn' => 'ignore_internal_falsable_issues', + 'ignoreInternalFunctionNullReturn' => 'ignore_internal_nullable_issues', + ]; + + foreach ($booleanAttributes as $xmlName => $internalName) { + if (isset($config_xml[$xmlName])) { + $attribute_text = (string) $config_xml[$xmlName]; + $config->setBooleanAttribute( + $internalName, + $attribute_text === 'true' || $attribute_text === '1' + ); + } } - if (isset($config_xml['useDocblockPropertyTypes'])) { - $attribute_text = (string) $config_xml['useDocblockPropertyTypes']; - $config->use_docblock_property_types = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['throwExceptionOnError'])) { - $attribute_text = (string) $config_xml['throwExceptionOnError']; - $config->throw_exception = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['hideExternalErrors'])) { - $attribute_text = (string) $config_xml['hideExternalErrors']; - $config->hide_external_errors = $attribute_text === 'true' || $attribute_text === '1'; - } - - - if (isset($config_xml['resolveFromConfigFile'])) { - $attribute_text = (string) $config_xml['resolveFromConfigFile']; - $config->resolve_from_config_file = $attribute_text === 'true' || $attribute_text === '1'; - } - - if ($config->resolve_from_config_file) { $config->base_dir = $base_dir; } else { @@ -686,36 +715,6 @@ class Config trigger_error('Could not create cache directory: ' . $config->cache_directory, E_USER_ERROR); } - if (isset($config_xml['allowFileIncludes'])) { - $attribute_text = (string) $config_xml['allowFileIncludes']; - $config->allow_includes = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['totallyTyped'])) { - $attribute_text = (string) $config_xml['totallyTyped']; - $config->totally_typed = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['strictBinaryOperands'])) { - $attribute_text = (string) $config_xml['strictBinaryOperands']; - $config->strict_binary_operands = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['requireVoidReturnType'])) { - $attribute_text = (string) $config_xml['requireVoidReturnType']; - $config->add_void_docblocks = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['useAssertForType'])) { - $attribute_text = (string) $config_xml['useAssertForType']; - $config->use_assert_for_type = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['rememberPropertyAssignmentsAfterCall'])) { - $attribute_text = (string) $config_xml['rememberPropertyAssignmentsAfterCall']; - $config->remember_property_assignments_after_call = $attribute_text === 'true' || $attribute_text === '1'; - } - if (isset($config_xml['serializer'])) { $attribute_text = (string) $config_xml['serializer']; $config->use_igbinary = $attribute_text === 'igbinary'; @@ -723,50 +722,6 @@ class Config $config->use_igbinary = version_compare($igbinary_version, '2.0.5') >= 0; } - if (isset($config_xml['allowPhpStormGenerics'])) { - $attribute_text = (string) $config_xml['allowPhpStormGenerics']; - $config->allow_phpstorm_generics = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['allowStringToStandInForClass'])) { - $attribute_text = (string) $config_xml['allowStringToStandInForClass']; - $config->allow_string_standin_for_class = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['usePhpDocMethodsWithoutMagicCall'])) { - $attribute_text = (string) $config_xml['usePhpDocMethodsWithoutMagicCall']; - $config->use_phpdoc_method_without_magic_or_parent = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['memoizeMethodCallResults'])) { - $attribute_text = (string) $config_xml['memoizeMethodCallResults']; - $config->memoize_method_calls = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['hoistConstants'])) { - $attribute_text = (string) $config_xml['hoistConstants']; - $config->hoist_constants = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['addParamDefaultToDocblockType'])) { - $attribute_text = (string) $config_xml['addParamDefaultToDocblockType']; - $config->add_param_default_to_docblock_type = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['checkForThrowsDocblock'])) { - $attribute_text = (string) $config_xml['checkForThrowsDocblock']; - $config->check_for_throws_docblock = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['checkForThrowsInGlobalScope'])) { - $attribute_text = (string) $config_xml['checkForThrowsInGlobalScope']; - $config->check_for_throws_in_global_scope = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['forbidEcho'])) { - $attribute_text = (string) $config_xml['forbidEcho']; - $config->forbid_echo = $attribute_text === 'true' || $attribute_text === '1'; - } if (isset($config_xml['findUnusedCode'])) { $attribute_text = (string) $config_xml['findUnusedCode']; @@ -779,16 +734,6 @@ class Config $config->find_unused_variables = $attribute_text === 'true' || $attribute_text === '1'; } - if (isset($config_xml['ignoreInternalFunctionFalseReturn'])) { - $attribute_text = (string) $config_xml['ignoreInternalFunctionFalseReturn']; - $config->ignore_internal_falsable_issues = $attribute_text === 'true' || $attribute_text === '1'; - } - - if (isset($config_xml['ignoreInternalFunctionNullReturn'])) { - $attribute_text = (string) $config_xml['ignoreInternalFunctionNullReturn']; - $config->ignore_internal_nullable_issues = $attribute_text === 'true' || $attribute_text === '1'; - } - if (isset($config_xml['errorBaseline'])) { $attribute_text = (string) $config_xml['errorBaseline']; $config->error_baseline = $attribute_text; @@ -1811,4 +1756,9 @@ class Config { $this->stub_files[] = $stub_file; } + + private function setBooleanAttribute(string $name, bool $value): void + { + $this->$name = $value; + } } diff --git a/src/command_functions.php b/src/command_functions.php index 7c0a167b1..2f17cccb8 100644 --- a/src/command_functions.php +++ b/src/command_functions.php @@ -1,5 +1,9 @@ 0 && in_array($input_paths[$i-1], ['-c', '--config'])) { - // This is the path to the config file, not a path to check. - continue; - } - if (realpath($input_path) === realpath(dirname(__DIR__) . DIRECTORY_SEPARATOR . 'psalm') || realpath($input_path) === realpath(dirname(__DIR__) . DIRECTORY_SEPARATOR . 'psalter') || realpath($input_path) === realpath(Phar::running(false)) @@ -270,3 +269,151 @@ function getPathsToCheck($f_paths) return $paths_to_check; } + +function getPsalmHelpText(): string +{ + return <<getMessage() . PHP_EOL); + exit(1); + } + + $config->setComposerClassLoader($first_autoloader); + + return $config; +} + +function get_path_to_config(array $options): ?string +{ + $path_to_config = isset($options['c']) && is_string($options['c']) ? realpath($options['c']) : null; + + if ($path_to_config === false) { + /** @psalm-suppress InvalidCast */ + fwrite(STDERR, 'Could not resolve path to config ' . (string)$options['c'] . PHP_EOL); + exit(1); + } + return $path_to_config; +} diff --git a/src/psalm-language-server.php b/src/psalm-language-server.php index f218d6707..27cfcc3ee 100644 --- a/src/psalm-language-server.php +++ b/src/psalm-language-server.php @@ -199,13 +199,7 @@ $ini_handler->check(); setlocale(LC_CTYPE, 'C'); -$path_to_config = isset($options['c']) && is_string($options['c']) ? realpath($options['c']) : null; - -if ($path_to_config === false) { - /** @psalm-suppress InvalidCast */ - fwrite(STDERR, 'Could not resolve path to config ' . (string)$options['c'] . PHP_EOL); - exit(1); -} +$path_to_config = get_path_to_config($options); if (isset($options['tcp'])) { if (!is_string($options['tcp'])) { @@ -216,20 +210,14 @@ if (isset($options['tcp'])) { $find_dead_code = isset($options['find-dead-code']); -// initialise custom config, if passed -try { - if ($path_to_config) { - $config = Config::loadFromXMLFile($path_to_config, $current_dir); - } else { - $config = Config::getConfigForPath($current_dir, $current_dir, \Psalm\Report::TYPE_CONSOLE); - } -} catch (Psalm\Exception\ConfigException $e) { - fwrite(STDERR, $e->getMessage()); - exit(1); +$config = initialiseConfig($path_to_config, $current_dir, \Psalm\Report::TYPE_CONSOLE, $first_autoloader); + +if ($config->resolve_from_config_file) { + $current_dir = $config->base_dir; + chdir($current_dir); } $config->setServerMode(); -$config->setComposerClassLoader($first_autoloader); if (isset($options['clear-cache'])) { $cache_directory = $config->getCacheDirectory(); diff --git a/src/psalm-refactor.php b/src/psalm-refactor.php index b2fb3bd69..545c96f11 100644 --- a/src/psalm-refactor.php +++ b/src/psalm-refactor.php @@ -18,7 +18,7 @@ gc_disable(); $args = array_slice($argv, 1); -$valid_short_options = ['f:', 'm', 'h', 'r:']; +$valid_short_options = ['f:', 'm', 'h', 'r:', 'c:']; $valid_long_options = [ 'help', 'debug', 'config:', 'root:', 'threads:', 'move:', 'into:', 'rename:', 'to:', @@ -141,12 +141,7 @@ $first_autoloader = requireAutoloaders($current_dir, isset($options['r']), $vend // If XDebug is enabled, restart without it (new \Composer\XdebugHandler\XdebugHandler('PSALTER'))->check(); -$path_to_config = isset($options['c']) && is_string($options['c']) ? realpath($options['c']) : null; - -if ($path_to_config === false) { - /** @psalm-suppress InvalidCast */ - die('Could not resolve path to config ' . (string)$options['c'] . PHP_EOL); -} +$path_to_config = get_path_to_config($options); $args = getArguments(); @@ -230,15 +225,12 @@ if (!$to_refactor) { die('No --move or --rename arguments supplied' . PHP_EOL); } -// initialise custom config, if passed -// Initializing the config can be slow, so any UI logic should precede it, if possible -if ($path_to_config) { - $config = Config::loadFromXMLFile($path_to_config, $current_dir); -} else { - $config = Config::getConfigForPath($current_dir, $current_dir, \Psalm\Report::TYPE_CONSOLE); -} +$config = initialiseConfig($path_to_config, $current_dir, \Psalm\Report::TYPE_CONSOLE, $first_autoloader); -$config->setComposerClassLoader($first_autoloader); +if ($config->resolve_from_config_file) { + $current_dir = $config->base_dir; + chdir($current_dir); +} $threads = isset($options['threads']) ? (int)$options['threads'] diff --git a/src/psalm.php b/src/psalm.php index 30d98b490..a1476b91a 100644 --- a/src/psalm.php +++ b/src/psalm.php @@ -154,118 +154,9 @@ if (isset($options['c']) && is_array($options['c'])) { exit(1); } + if (array_key_exists('h', $options)) { - echo <<getMessage() . PHP_EOL); - exit(1); -} +$config = initialiseConfig($path_to_config, $current_dir, $output_format, $first_autoloader); if ($config->resolve_from_config_file) { $current_dir = $config->base_dir; + chdir($current_dir); } @@ -505,8 +381,6 @@ if (isset($options['shepherd'])) { $plugins[] = $shepherd_plugin; } -$config->setComposerClassLoader($first_autoloader); - if (isset($options['clear-cache'])) { $cache_directory = $config->getCacheDirectory(); diff --git a/src/psalter.php b/src/psalter.php index 69230246a..3158e0a70 100644 --- a/src/psalter.php +++ b/src/psalter.php @@ -18,7 +18,7 @@ gc_disable(); $args = array_slice($argv, 1); -$valid_short_options = ['f:', 'm', 'h', 'r:']; +$valid_short_options = ['f:', 'm', 'h', 'r:', 'c:']; $valid_long_options = [ 'help', 'debug', 'debug-by-line', 'config:', 'file:', 'root:', 'plugin:', 'issues:', 'list-supported-issues', 'php-version:', 'dry-run', 'safe-types', @@ -178,22 +178,15 @@ $first_autoloader = requireAutoloaders($current_dir, isset($options['r']), $vend $paths_to_check = getPathsToCheck(isset($options['f']) ? $options['f'] : null); -$path_to_config = isset($options['c']) && is_string($options['c']) ? realpath($options['c']) : null; +$path_to_config = get_path_to_config($options); -if ($path_to_config === false) { - /** @psalm-suppress InvalidCast */ - die('Could not resolve path to config ' . (string)$options['c'] . PHP_EOL); +$config = initialiseConfig($path_to_config, $current_dir, \Psalm\Report::TYPE_CONSOLE, $first_autoloader); + +if ($config->resolve_from_config_file) { + $current_dir = $config->base_dir; + chdir($current_dir); } -// initialise custom config, if passed -if ($path_to_config) { - $config = Config::loadFromXMLFile($path_to_config, $current_dir); -} else { - $config = Config::getConfigForPath($current_dir, $current_dir, \Psalm\Report::TYPE_CONSOLE); -} - -$config->setComposerClassLoader($first_autoloader); - $threads = isset($options['threads']) ? (int)$options['threads'] : 1; $providers = new Psalm\Internal\Provider\Providers( diff --git a/tests/EndToEnd/PsalmEndToEndTest.php b/tests/EndToEnd/PsalmEndToEndTest.php index f6423cded..4f1fcbe61 100644 --- a/tests/EndToEnd/PsalmEndToEndTest.php +++ b/tests/EndToEnd/PsalmEndToEndTest.php @@ -91,7 +91,7 @@ class PsalmEndToEndTest extends TestCase $this->assertStringContainsString( 'No errors found!', - $this->runPsalm(['--alter', '--issues=all'], false, false)['STDOUT'] // @todo get --alter working with config dir + $this->runPsalm(['--alter', '--issues=all'], false, true)['STDOUT'] ); $this->assertSame(0, $this->runPsalm([])['CODE']); @@ -100,7 +100,7 @@ class PsalmEndToEndTest extends TestCase public function testPsalter(): void { $this->runPsalmInit(); - (new Process([$this->psalter, '--alter', '--issues=InvalidReturnType'], self::$tmpDir))->mustRun(); + (new Process(['php', $this->psalter, '--alter', '--issues=InvalidReturnType'], self::$tmpDir))->mustRun(); $this->assertSame(0, $this->runPsalm([])['CODE']); } @@ -124,7 +124,7 @@ class PsalmEndToEndTest extends TestCase file_put_contents(self::$tmpDir . '/src/psalm.xml', $psalmXmlContent); - $process = new Process([$this->psalm, '--config', 'src/psalm.xml'], self::$tmpDir); + $process = new Process(['php', $this->psalm, '--config=src/psalm.xml'], self::$tmpDir); $process->run(); $this->assertSame(1, $process->getExitCode()); $this->assertStringContainsString('InvalidReturnType', $process->getOutput()); @@ -140,10 +140,12 @@ class PsalmEndToEndTest extends TestCase // As config files all contain `resolveFromConfigFile="true"` Psalm shouldn't need to be run from the same // directory that the code being analysed exists in. + // Windows doesn't read shabangs, so to allow this to work on windows we run `php psalm` rather than just `psalm`. + if ($relyOnConfigDir) { - $process = new Process(array_merge([$this->psalm, '-c', self::$tmpDir . '/psalm.xml'], $args), null); + $process = new Process(array_merge(['php', $this->psalm, '-c=' . self::$tmpDir . '/psalm.xml'], $args), null); } else { - $process = new Process(array_merge([$this->psalm], $args), self::$tmpDir); + $process = new Process(array_merge(['php', $this->psalm], $args), self::$tmpDir); } if (!$shouldFail) { diff --git a/tests/ReportOutputTest.php b/tests/ReportOutputTest.php index 18d23d433..98eb299c9 100644 --- a/tests/ReportOutputTest.php +++ b/tests/ReportOutputTest.php @@ -5,6 +5,7 @@ use function file_get_contents; use function json_decode; use function ob_end_clean; use function ob_start; +use function preg_replace; use Psalm\Context; use Psalm\Internal\Analyzer\FileAnalyzer; use Psalm\Internal\Analyzer\ProjectAnalyzer; @@ -408,10 +409,9 @@ INFO: PossiblyUndefinedGlobalVariable - somefile.php:15:6 - Possibly undefined g '| ERROR | 7 | UndefinedConstant | Const CHANGE_ME is not defined |' . "\n" . '| INFO | 15 | PossiblyUndefinedGlobalVariable | Possibly undefined global variable $a, first seen on line 10 |' . "\n" . '+----------+------+---------------------------------+---------------------------------------------------------------+' . "\n", - IssueBuffer::getOutput($compact_report_options) + $this->toUnixLineEndings(IssueBuffer::getOutput($compact_report_options)) ); } - /** * @return void */ @@ -493,4 +493,12 @@ INFO: PossiblyUndefinedGlobalVariable - somefile.php:15:6 - Possibly undefined g ', file_get_contents(__DIR__ . '/test-report.json')); unlink(__DIR__ . '/test-report.json'); } + + /** + * Needed when running on Windows + */ + private function toUnixLineEndings(string $output): string + { + return preg_replace('~\r\n?~', "\n", $output); + } }