From 41a4f71a9d596427ede8214223486556491c2878 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 12:30:19 -0400 Subject: [PATCH 01/16] Replace empty() calls in CliOptions Since `empty()` works on null undefined without a warning, it also hides bugs from static analysis (causing the bug in https://github.com/sirbrillig/phpcs-changed/pull/99). This change should have the same effect while not ignoring warnings. --- PhpcsChanged/CliOptions.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/PhpcsChanged/CliOptions.php b/PhpcsChanged/CliOptions.php index 1517ca2..127f011 100644 --- a/PhpcsChanged/CliOptions.php +++ b/PhpcsChanged/CliOptions.php @@ -362,21 +362,21 @@ public function getExecutablePath(string $executableName): string { } public function validate(): void { - if (empty($this->mode)) { + if (! boolval($this->mode)) { throw new InvalidOptionException('You must use either automatic or manual mode.'); } if ($this->mode === Modes::MANUAL) { - if (empty($this->diffFile) || empty($this->phpcsUnmodified) || empty($this->phpcsModified)) { + if ( ! boolval($this->diffFile) || ! boolval($this->phpcsUnmodified) || ! boolval($this->phpcsModified)) { throw new InvalidOptionException('Manual mode requires a diff, the unmodified file phpcs output, and the modified file phpcs output.'); } } - if ($this->mode === Modes::GIT_BASE && empty($this->gitBase)) { + if ($this->mode === Modes::GIT_BASE && ! boolval($this->gitBase)) { throw new InvalidOptionException('git-base mode requires a git object.'); } - if ($this->isGitMode() && empty($this->files)) { + if ($this->isGitMode() && ! boolval($this->files)) { throw new InvalidOptionException('You must supply at least one file or directory to run in git mode.'); } - if ($this->mode === Modes::SVN && empty($this->files)) { + if ($this->mode === Modes::SVN && ! boolval($this->files)) { throw new InvalidOptionException('You must supply at least one file or directory to run in svn mode.'); } } From 8fc89750b3d53fba9416ffc4789c1cc346bfcc7f Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 12:53:38 -0400 Subject: [PATCH 02/16] Replace `empty()` in other files with more explicit checks Again, this is because `empty()` allows undefined values and hides potential bugs from static analysis. --- PhpcsChanged/Cli.php | 8 ++++---- PhpcsChanged/FullReporter.php | 2 +- PhpcsChanged/JsonReporter.php | 2 +- PhpcsChanged/PhpcsMessagesHelpers.php | 2 +- PhpcsChanged/UnixShell.php | 6 +++--- PhpcsChanged/XmlReporter.php | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/PhpcsChanged/Cli.php b/PhpcsChanged/Cli.php index dfe5b63..884d776 100644 --- a/PhpcsChanged/Cli.php +++ b/PhpcsChanged/Cli.php @@ -260,7 +260,7 @@ function runSvnWorkflowForFile(string $svnFile, CliOptions $options, ShellOperat } $modifiedFilePhpcsMessages = PhpcsMessages::fromPhpcsJson($modifiedFilePhpcsOutput, $fileName); - $hasNewPhpcsMessages = !empty($modifiedFilePhpcsMessages->getMessages()); + $hasNewPhpcsMessages = count($modifiedFilePhpcsMessages->getMessages()) > 0; if (! $hasNewPhpcsMessages) { throw new NoChangesException("Modified file '{$svnFile}' has no PHPCS messages; skipping"); @@ -357,7 +357,7 @@ function runGitWorkflowForFile(string $gitFile, CliOptions $options, ShellOperat } $modifiedFilePhpcsMessages = PhpcsMessages::fromPhpcsJson($modifiedFilePhpcsOutput, $fileName); - $hasNewPhpcsMessages = !empty($modifiedFilePhpcsMessages->getMessages()); + $hasNewPhpcsMessages = count($modifiedFilePhpcsMessages->getMessages()) > 0; $unifiedDiff = ''; $unmodifiedFilePhpcsOutput = ''; @@ -414,7 +414,7 @@ function fileHasValidExtension(\SplFileInfo $file, string $phpcsExtensions = '') // The following logic is copied from PHPCS itself. See https://github.com/squizlabs/PHP_CodeSniffer/blob/2ecd8dc15364cdd6e5089e82ffef2b205c98c412/src/Filters/Filter.php#L161 // phpcs:disable - if (empty($phpcsExtensions)) { + if (! boolval($phpcsExtensions)) { $AllowedExtensions = [ 'php', 'inc', @@ -443,7 +443,7 @@ function fileHasValidExtension(\SplFileInfo $file, string $phpcsExtensions = '') array_shift($fileParts); } $matches = array_intersect($extensions, $AllowedExtensions); - if (empty($matches) === true) { + if (count($matches) === 0) { return false; } diff --git a/PhpcsChanged/FullReporter.php b/PhpcsChanged/FullReporter.php index 60e8fc0..6eafca2 100644 --- a/PhpcsChanged/FullReporter.php +++ b/PhpcsChanged/FullReporter.php @@ -13,7 +13,7 @@ public function getFormattedMessages(PhpcsMessages $messages, array $options): s $files = array_unique(array_map(function(LintMessage $message): string { return $message->getFile() ?? 'STDIN'; }, $messages->getMessages())); - if (empty($files)) { + if (count($files) === 0) { $files = ['STDIN']; } diff --git a/PhpcsChanged/JsonReporter.php b/PhpcsChanged/JsonReporter.php index 891f76c..b813dd4 100644 --- a/PhpcsChanged/JsonReporter.php +++ b/PhpcsChanged/JsonReporter.php @@ -13,7 +13,7 @@ public function getFormattedMessages(PhpcsMessages $messages, array $options): s $files = array_unique(array_map(function(LintMessage $message): string { return $message->getFile() ?? 'STDIN'; }, $messages->getMessages())); - if (empty($files)) { + if (count($files) === 0) { $files = ['STDIN']; } diff --git a/PhpcsChanged/PhpcsMessagesHelpers.php b/PhpcsChanged/PhpcsMessagesHelpers.php index 27a8755..befc9b4 100644 --- a/PhpcsChanged/PhpcsMessagesHelpers.php +++ b/PhpcsChanged/PhpcsMessagesHelpers.php @@ -9,7 +9,7 @@ class PhpcsMessagesHelpers { public static function fromPhpcsJson(string $messages, string $forcedFileName = null): PhpcsMessages { - if (empty($messages)) { + if (! boolval($messages)) { return self::fromArrays([], $forcedFileName ?? 'STDIN'); } /** diff --git a/PhpcsChanged/UnixShell.php b/PhpcsChanged/UnixShell.php index a402fe8..66ea644 100644 --- a/PhpcsChanged/UnixShell.php +++ b/PhpcsChanged/UnixShell.php @@ -72,7 +72,7 @@ protected function validateExecutableExists(string $name, string $command): void } private function getPhpcsExecutable(): string { - if (! empty($this->options->phpcsPath) || ! empty(getenv('PHPCS'))) { + if (boolval($this->options->phpcsPath) || boolval(getenv('PHPCS'))) { return $this->options->getExecutablePath('phpcs'); } if (! $this->options->noVendorPhpcs && $this->doesPhpcsExistInVendor()) { @@ -274,7 +274,7 @@ public function getGitUnifiedDiff(string $fileName): string { $debug = getDebug($this->options->debug); $git = $this->options->getExecutablePath('git'); $objectOption = $this->options->mode === Modes::GIT_BASE ? ' ' . escapeshellarg($this->options->gitBase) . '...' : ''; - $stagedOption = empty($objectOption) && $this->options->mode !== Modes::GIT_UNSTAGED ? ' --staged' : ''; + $stagedOption = ! boolval($objectOption) && $this->options->mode !== Modes::GIT_UNSTAGED ? ' --staged' : ''; $unifiedDiffCommand = "{$git} diff{$stagedOption}{$objectOption} --no-prefix " . escapeshellarg($fileName); $debug('running diff command:', $unifiedDiffCommand); $unifiedDiff = $this->executeCommand($unifiedDiffCommand); @@ -421,7 +421,7 @@ public function getPhpcsVersion(): string { } $matched = preg_match('/version\\s([0-9.]+)/uim', $versionPhpcsOutput, $matches); - if (empty($matched) || empty($matches[1])) { + if ($matched === false || empty($matches[1])) { throw new ShellException("Cannot parse phpcs version output"); } diff --git a/PhpcsChanged/XmlReporter.php b/PhpcsChanged/XmlReporter.php index 6bf15c2..2c9a92a 100644 --- a/PhpcsChanged/XmlReporter.php +++ b/PhpcsChanged/XmlReporter.php @@ -31,7 +31,7 @@ public function getFormattedMessages(PhpcsMessages $messages, array $options): s $files = array_unique(array_map(function(LintMessage $message): string { return $message->getFile() ?? 'STDIN'; }, $messages->getMessages())); - if (empty($files)) { + if (count($files) === 0) { $files = ['STDIN']; } From 964c92d47989644bc4de4c477b53d9d937696602 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 12:56:15 -0400 Subject: [PATCH 03/16] Replace isset with array_key_exists in Cli --- PhpcsChanged/Cli.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/PhpcsChanged/Cli.php b/PhpcsChanged/Cli.php index 884d776..0c2a646 100644 --- a/PhpcsChanged/Cli.php +++ b/PhpcsChanged/Cli.php @@ -529,10 +529,10 @@ function shouldIgnorePath(string $path, string $patternOption = null): bool { } function isCachingEnabled(array $options): bool { - if (isset($options['no-cache'])) { + if (array_key_exists('no-cache', $options)) { return false; } - if (isset($options['cache'])) { + if (array_key_exists('cache', $options)) { return true; } return false; @@ -553,7 +553,7 @@ function loadCache(CacheManager $cache, ShellOperator $shell, array $options): v } } - if (isset($options['clear-cache'])) { + if (array_key_exists('clear-cache', $options)) { $cache->clearCache(); try { $cache->save(); From 73333e3bdcfdf45278145f85675ea576343ec383 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 13:00:11 -0400 Subject: [PATCH 04/16] Replace isset in addCacheEntry --- PhpcsChanged/CacheManager.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/PhpcsChanged/CacheManager.php b/PhpcsChanged/CacheManager.php index 09b050f..139862f 100644 --- a/PhpcsChanged/CacheManager.php +++ b/PhpcsChanged/CacheManager.php @@ -162,13 +162,13 @@ public function setCacheForFile(string $filePath, string $type, string $hash, st public function addCacheEntry(CacheEntry $entry): void { $this->hasBeenModified = true; $this->pruneOldEntriesForFile($entry); - if (! isset($this->fileDataByPath[$entry->path])) { + if (! array_key_exists($entry->path, $this->fileDataByPath)) { $this->fileDataByPath[$entry->path] = []; } - if (! isset($this->fileDataByPath[$entry->path][$entry->type])) { + if (! array_key_exists($entry->type, $this->fileDataByPath[$entry->path])) { $this->fileDataByPath[$entry->path][$entry->type] = []; } - if (! isset($this->fileDataByPath[$entry->path][$entry->type][$entry->hash])) { + if (! array_key_exists($entry->hash, $this->fileDataByPath[$entry->path][$entry->type])) { $this->fileDataByPath[$entry->path][$entry->type][$entry->hash] = []; } $this->fileDataByPath[$entry->path][$entry->type][$entry->hash][$entry->phpcsStandard] = $entry; From fe1cd4d536469b58199261282de281744857f65b Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 13:04:49 -0400 Subject: [PATCH 05/16] Replace isset with array_key_exists in CliOptions->fromArray --- PhpcsChanged/CliOptions.php | 60 ++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/PhpcsChanged/CliOptions.php b/PhpcsChanged/CliOptions.php index 127f011..d2bef31 100644 --- a/PhpcsChanged/CliOptions.php +++ b/PhpcsChanged/CliOptions.php @@ -169,94 +169,94 @@ class CliOptions { public static function fromArray(array $options): self { $cliOptions = new self(); // Note that this array is likely created by `getopt()` which sets any - // boolean option to `false`, meaning that we must use `isset()` to - // determine if these options are set. - if (isset($options['files'])) { + // boolean option to `false` (it's so confusing), meaning that we cannot + // check the truthiness of the option. + if (array_key_exists('files', $options)) { $cliOptions->files = $options['files']; } - if (isset($options['no-vendor-phpcs'])) { + if (array_key_exists('no-vendor-phpcs', $options)) { $cliOptions->noVendorPhpcs = true; } - if (isset($options['phpcs-path'])) { + if (array_key_exists('phpcs-path', $options)) { $cliOptions->phpcsPath = $options['phpcs-path']; } - if (isset($options['git-path'])) { + if (array_key_exists('git-path', $options)) { $cliOptions->gitPath = $options['git-path']; } - if (isset($options['cat-path'])) { + if (array_key_exists('cat-path', $options)) { $cliOptions->catPath = $options['cat-path']; } - if (isset($options['svn-path'])) { + if (array_key_exists('svn-path', $options)) { $cliOptions->svnPath = $options['svn-path']; } - if (isset($options['svn'])) { + if (array_key_exists('svn', $options)) { $cliOptions->mode = Modes::SVN; } - if (isset($options['git'])) { + if (array_key_exists('git', $options)) { $cliOptions->mode = Modes::GIT_STAGED; } - if (isset($options['git-unstaged'])) { + if (array_key_exists('git-unstaged', $options)) { $cliOptions->mode = Modes::GIT_UNSTAGED; } - if (isset($options['git-staged'])) { + if (array_key_exists('git-staged', $options)) { $cliOptions->mode = Modes::GIT_STAGED; } - if (isset($options['git-base'])) { + if (array_key_exists('git-base', $options)) { $cliOptions->mode = Modes::GIT_BASE; $cliOptions->gitBase = $options['git-base']; } - if (isset($options['report'])) { + if (array_key_exists('report', $options)) { $cliOptions->reporter = $options['report']; } - if (isset($options['debug'])) { + if (array_key_exists('debug', $options)) { $cliOptions->debug = true; } - if (isset($options['clear-cache'])) { + if (array_key_exists('clear-cache', $options)) { $cliOptions->clearCache = true; } - if (isset($options['cache'])) { + if (array_key_exists('cache', $options)) { $cliOptions->useCache = true; } - if (isset($options['no-cache'])) { + if (array_key_exists('no-cache', $options)) { $cliOptions->useCache = false; } - if (isset($options['diff'])) { + if (array_key_exists('diff', $options)) { $cliOptions->mode = Modes::MANUAL; $cliOptions->diffFile = $options['diff']; } - if (isset($options['phpcs-unmodified'])) { + if (array_key_exists('phpcs-unmodified', $options)) { $cliOptions->mode = Modes::MANUAL; $cliOptions->phpcsUnmodified = $options['phpcs-unmodified']; } - if (isset($options['phpcs-modified'])) { + if (array_key_exists('phpcs-modified', $options)) { $cliOptions->mode = Modes::MANUAL; $cliOptions->phpcsModified = $options['phpcs-modified']; } - if (isset($options['s'])) { + if (array_key_exists('s', $options)) { $cliOptions->showMessageCodes = true; } - if (isset($options['standard'])) { + if (array_key_exists('standard', $options)) { $cliOptions->phpcsStandard = $options['standard']; } - if (isset($options['extensions'])) { + if (array_key_exists('extensions', $options)) { $cliOptions->phpcsExtensions = $options['extensions']; } - if (isset($options['always-exit-zero'])) { + if (array_key_exists('always-exit-zero', $options)) { $cliOptions->alwaysExitZero = true; } - if (isset($options['no-cache-git-root'])) { + if (array_key_exists('no-cache-git-root', $options)) { $cliOptions->noCacheGitRoot = true; } - if (isset($options['no-verify-git-file'])) { + if (array_key_exists('no-verify-git-file', $options)) { $cliOptions->noVerifyGitFile = true; } - if (isset($options['warning-severity'])) { + if (array_key_exists('warning-severity', $options)) { $cliOptions->warningSeverity = $options['warning-severity']; } - if (isset($options['error-severity'])) { + if (array_key_exists('error-severity', $options)) { $cliOptions->errorSeverity = $options['error-severity']; } - if (isset($options['i'])) { + if (array_key_exists('i', $options)) { $cliOptions->mode = Modes::INFO_ONLY; } $cliOptions->validate(); From 0cb55172d17b5d410255e988e7db5fa9c21f876e Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 13:09:46 -0400 Subject: [PATCH 06/16] Replace isset in CliOptions->toArray --- PhpcsChanged/CliOptions.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/PhpcsChanged/CliOptions.php b/PhpcsChanged/CliOptions.php index d2bef31..60a2737 100644 --- a/PhpcsChanged/CliOptions.php +++ b/PhpcsChanged/CliOptions.php @@ -332,10 +332,13 @@ public function toArray(): array { if ($this->noVerifyGitFile) { $options['no-verify-git-file'] = true; } - if (isset($this->warningSeverity)) { + // Note that both warningSeverity and errorSeverity can be the string '0' + // which is falsy in PHP but is a valid value here so we must be careful + // when testing for it. + if (is_string($this->warningSeverity) && strlen($this->warningSeverity) > 0) { $options['warning-severity'] = $this->warningSeverity; } - if (isset($this->errorSeverity)) { + if (is_string($this->errorSeverity) && strlen($this->errorSeverity) > 0) { $options['error-severity'] = $this->errorSeverity; } return $options; From 27ace0918a20c6dfa87eb37a5a3ba14397f3e0d0 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 13:12:11 -0400 Subject: [PATCH 07/16] Replace isset with array_key_exists in FullReporter --- PhpcsChanged/FullReporter.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/PhpcsChanged/FullReporter.php b/PhpcsChanged/FullReporter.php index 6eafca2..dd33991 100644 --- a/PhpcsChanged/FullReporter.php +++ b/PhpcsChanged/FullReporter.php @@ -52,7 +52,7 @@ private function getFormattedMessagesForFile(array $messages, string $file, arra $formattedLines = implode("\n", array_map(function(LintMessage $message) use ($longestNumber, $options): string { $source = $message->getSource() ?: 'Unknown'; - $sourceString = isset($options['s']) ? " ({$source})" : ''; + $sourceString = array_key_exists('s', $options) ? " ({$source})" : ''; return sprintf(" %{$longestNumber}d | %s | %s%s", $message->getLineNumber(), $message->getType(), $message->getMessage(), $sourceString); }, $messages)); From 77933ce858e55535f7368a8e40c9a5984b920b1f Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 13:12:28 -0400 Subject: [PATCH 08/16] Replace isset in getPhpcsStandardOption --- PhpcsChanged/UnixShell.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/PhpcsChanged/UnixShell.php b/PhpcsChanged/UnixShell.php index 66ea644..6d36396 100644 --- a/PhpcsChanged/UnixShell.php +++ b/PhpcsChanged/UnixShell.php @@ -239,10 +239,10 @@ public function getGitHashOfUnmodifiedFile(string $fileName): string { private function getPhpcsStandardOption(): string { $phpcsStandard = $this->options->phpcsStandard; $phpcsStandardOption = $phpcsStandard ? ' --standard=' . escapeshellarg($phpcsStandard) : ''; - $warningSeverity = $this->options->warningSeverity; - $phpcsStandardOption .= isset($warningSeverity) ? ' --warning-severity=' . escapeshellarg($warningSeverity) : ''; - $errorSeverity = $this->options->errorSeverity; - $phpcsStandardOption .= isset($errorSeverity) ? ' --error-severity=' . escapeshellarg($errorSeverity) : ''; + $warningSeverity = $this->options->warningSeverity ?? ''; + $phpcsStandardOption .= strlen($warningSeverity) > 0 ? ' --warning-severity=' . escapeshellarg($warningSeverity) : ''; + $errorSeverity = $this->options->errorSeverity ?? ''; + $phpcsStandardOption .= strlen($errorSeverity) > 0 ? ' --error-severity=' . escapeshellarg($errorSeverity) : ''; return $phpcsStandardOption; } From 2303bf8ce1a5049b2e100f6eeab197d3921e3194 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 13:20:49 -0400 Subject: [PATCH 09/16] Use boolval to make CliOptions->toArray more explicit --- PhpcsChanged/CliOptions.php | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/PhpcsChanged/CliOptions.php b/PhpcsChanged/CliOptions.php index 60a2737..21a8b33 100644 --- a/PhpcsChanged/CliOptions.php +++ b/PhpcsChanged/CliOptions.php @@ -267,31 +267,31 @@ public function toArray(): array { $options = []; $options['report'] = $this->reporter; $options['files'] = $this->files; - if ($this->phpcsStandard) { + if (boolval($this->phpcsStandard)) { $options['standard'] = $this->phpcsStandard; } - if ($this->phpcsExtensions) { + if (boolval($this->phpcsExtensions)) { $options['extensions'] = $this->phpcsExtensions; } - if ($this->noVendorPhpcs) { + if (boolval($this->noVendorPhpcs)) { $options['no-vendor-phpcs'] = true; } - if ($this->phpcsPath) { + if (boolval($this->phpcsPath)) { $options['phpcs-path'] = $this->phpcsPath; } - if ($this->gitPath) { + if (boolval($this->gitPath)) { $options['git-path'] = $this->gitPath; } - if ($this->catPath) { + if (boolval($this->catPath)) { $options['cat-path'] = $this->catPath; } - if ($this->svnPath) { + if (boolval($this->svnPath)) { $options['svn-path'] = $this->svnPath; } - if ($this->debug) { + if (boolval($this->debug)) { $options['debug'] = true; } - if ($this->showMessageCodes) { + if (boolval($this->showMessageCodes)) { $options['s'] = true; } if ($this->mode === Modes::SVN) { @@ -309,13 +309,13 @@ public function toArray(): array { $options['git'] = true; $options['git-base'] = $this->gitBase; } - if ($this->useCache) { + if (boolval($this->useCache)) { $options['cache'] = true; } - if (! $this->useCache) { + if (! boolval($this->useCache)) { $options['no-cache'] = true; } - if ($this->clearCache) { + if (boolval($this->clearCache)) { $options['clear-cache'] = true; } if ($this->mode === Modes::MANUAL) { @@ -323,13 +323,13 @@ public function toArray(): array { $options['phpcs-unmodified'] = $this->phpcsUnmodified; $options['phpcs-modified'] = $this->phpcsModified; } - if ($this->alwaysExitZero) { + if (boolval($this->alwaysExitZero)) { $options['always-exit-zero'] = true; } - if ($this->noCacheGitRoot) { + if (boolval($this->noCacheGitRoot)) { $options['no-cache-git-root'] = true; } - if ($this->noVerifyGitFile) { + if (boolval($this->noVerifyGitFile)) { $options['no-verify-git-file'] = true; } // Note that both warningSeverity and errorSeverity can be the string '0' From e763202352f493584c9f093f8af5b78354903f31 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 13:26:10 -0400 Subject: [PATCH 10/16] Make json_encode guard more explicit in JsonReporter --- PhpcsChanged/JsonReporter.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/PhpcsChanged/JsonReporter.php b/PhpcsChanged/JsonReporter.php index b813dd4..56d8e06 100644 --- a/PhpcsChanged/JsonReporter.php +++ b/PhpcsChanged/JsonReporter.php @@ -42,7 +42,7 @@ public function getFormattedMessages(PhpcsMessages $messages, array $options): s 'files' => array_merge([], ...$outputByFile), ]; $output = json_encode($dataForJson, JSON_UNESCAPED_SLASHES); - if (! $output) { + if (! boolval($output)) { throw new \Exception('Failed to JSON-encode result messages'); } return $output; From 0a22a3cd042867f6507bd1a17bea43d7789b9370 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 13:29:01 -0400 Subject: [PATCH 11/16] Make fileName guard in LintMessages more explicit --- PhpcsChanged/LintMessages.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/PhpcsChanged/LintMessages.php b/PhpcsChanged/LintMessages.php index 1bf6184..6d6af64 100644 --- a/PhpcsChanged/LintMessages.php +++ b/PhpcsChanged/LintMessages.php @@ -35,7 +35,7 @@ public static function merge(array $messages) { */ public static function fromLintMessages(array $messages, string $fileName = null) { return new static(array_map(function(LintMessage $message) use ($fileName) { - if ($fileName) { + if (is_string($fileName) && strlen($fileName) > 0) { $message->setFile($fileName); } return $message; From fa3dc3d91e7d349535238110dbe40377b3924627 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 13:32:34 -0400 Subject: [PATCH 12/16] Use more explicit guards in some UnixShell getters --- PhpcsChanged/UnixShell.php | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/PhpcsChanged/UnixShell.php b/PhpcsChanged/UnixShell.php index 6d36396..abd8273 100644 --- a/PhpcsChanged/UnixShell.php +++ b/PhpcsChanged/UnixShell.php @@ -237,8 +237,8 @@ public function getGitHashOfUnmodifiedFile(string $fileName): string { } private function getPhpcsStandardOption(): string { - $phpcsStandard = $this->options->phpcsStandard; - $phpcsStandardOption = $phpcsStandard ? ' --standard=' . escapeshellarg($phpcsStandard) : ''; + $phpcsStandard = $this->options->phpcsStandard ?? ''; + $phpcsStandardOption = strlen($phpcsStandard) > 0 ? ' --standard=' . escapeshellarg($phpcsStandard) : ''; $warningSeverity = $this->options->warningSeverity ?? ''; $phpcsStandardOption .= strlen($warningSeverity) > 0 ? ' --warning-severity=' . escapeshellarg($warningSeverity) : ''; $errorSeverity = $this->options->errorSeverity ?? ''; @@ -247,8 +247,8 @@ private function getPhpcsStandardOption(): string { } private function getPhpcsExtensionsOption(): string { - $phpcsExtensions = $this->options->phpcsExtensions; - $phpcsExtensionsOption = $phpcsExtensions ? ' --extensions=' . escapeshellarg($phpcsExtensions) : ''; + $phpcsExtensions = $this->options->phpcsExtensions ?? ''; + $phpcsExtensionsOption = strlen($phpcsExtensions) > 0 ? ' --extensions=' . escapeshellarg($phpcsExtensions) : ''; return $phpcsExtensionsOption; } @@ -346,12 +346,8 @@ public function doesUnmodifiedFileExistInSvn(string $fileName): bool { public function getSvnRevisionId(string $fileName): string { $svnFileInfo = $this->getSvnFileInfo($fileName); preg_match('/\bLast Changed Rev:\s([^\n]+)/', $svnFileInfo, $matches); - $version = $matches[1] ?? null; - if (! $version) { - // New files will not have a revision - return ''; - } - return $version; + // New files will not have a revision + return $matches[1] ?? ''; } private function getSvnFileInfo(string $fileName): string { From 984969aae1f32fc518cea7fd20df2b5407f1fd60 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 13:48:54 -0400 Subject: [PATCH 13/16] Make getExecutablePath guards more explicit --- PhpcsChanged/CliOptions.php | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/PhpcsChanged/CliOptions.php b/PhpcsChanged/CliOptions.php index 21a8b33..5d8ad51 100644 --- a/PhpcsChanged/CliOptions.php +++ b/PhpcsChanged/CliOptions.php @@ -352,13 +352,25 @@ public function isGitMode(): bool { public function getExecutablePath(string $executableName): string { switch ($executableName) { case 'phpcs': - return $this->phpcsPath ?: getenv('PHPCS') ?: 'phpcs'; + if (is_string($this->phpcsPath) && strlen($this->phpcsPath) > 0) { + return $this->phpcsPath; + } + return getenv('PHPCS') ?: 'phpcs'; case 'git': - return $this->gitPath ?: getenv('GIT') ?: 'git'; + if (is_string($this->gitPath) && strlen($this->gitPath) > 0) { + return $this->gitPath; + } + return getenv('GIT') ?: 'git'; case 'cat': - return $this->catPath ?: getenv('CAT') ?: 'cat'; + if (is_string($this->catPath) && strlen($this->catPath) > 0) { + return $this->catPath; + } + return getenv('CAT') ?: 'cat'; case 'svn': - return $this->svnPath ?: getenv('SVN') ?: 'svn'; + if (is_string($this->svnPath) && strlen($this->svnPath) > 0) { + return $this->svnPath; + } + return getenv('SVN') ?: 'svn'; default: throw new \Exception("No executable found called '{$executableName}'."); } From f8b498255afc896feadf268649ead6466907ef5e Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 13:54:32 -0400 Subject: [PATCH 14/16] Make getExecutablePath even more explicit --- PhpcsChanged/CliOptions.php | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/PhpcsChanged/CliOptions.php b/PhpcsChanged/CliOptions.php index 5d8ad51..fe74a33 100644 --- a/PhpcsChanged/CliOptions.php +++ b/PhpcsChanged/CliOptions.php @@ -355,22 +355,38 @@ public function getExecutablePath(string $executableName): string { if (is_string($this->phpcsPath) && strlen($this->phpcsPath) > 0) { return $this->phpcsPath; } - return getenv('PHPCS') ?: 'phpcs'; + $env = getenv('PHPCS'); + if (is_string($env) && strlen($env) > 0) { + return $env; + } + return 'phpcs'; case 'git': if (is_string($this->gitPath) && strlen($this->gitPath) > 0) { return $this->gitPath; } - return getenv('GIT') ?: 'git'; + $env = getenv('GIT'); + if (is_string($env) && strlen($env) > 0) { + return $env; + } + return 'git'; case 'cat': if (is_string($this->catPath) && strlen($this->catPath) > 0) { return $this->catPath; } - return getenv('CAT') ?: 'cat'; + $env = getenv('CAT'); + if (is_string($env) && strlen($env) > 0) { + return $env; + } + return 'cat'; case 'svn': if (is_string($this->svnPath) && strlen($this->svnPath) > 0) { return $this->svnPath; } - return getenv('SVN') ?: 'svn'; + $env = getenv('SVN'); + if (is_string($env) && strlen($env) > 0) { + return $env; + } + return 'svn'; default: throw new \Exception("No executable found called '{$executableName}'."); } From 15f8d6b6605f6916a0a7eb880395f08756f84e12 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 13:58:13 -0400 Subject: [PATCH 15/16] Add line breaks for preg_match to debug which condition is a problem psalm reports `RiskyTruthyFalsyComparison` for this line but I can't figure out why. --- PhpcsChanged/UnixShell.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/PhpcsChanged/UnixShell.php b/PhpcsChanged/UnixShell.php index abd8273..f84a01d 100644 --- a/PhpcsChanged/UnixShell.php +++ b/PhpcsChanged/UnixShell.php @@ -417,7 +417,10 @@ public function getPhpcsVersion(): string { } $matched = preg_match('/version\\s([0-9.]+)/uim', $versionPhpcsOutput, $matches); - if ($matched === false || empty($matches[1])) { + if ( + $matched === false + || empty($matches[1]) + ) { throw new ShellException("Cannot parse phpcs version output"); } From e5a53c0205c5ad6e93c28da5b0262b7d95b0e30d Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Thu, 23 May 2024 14:00:21 -0400 Subject: [PATCH 16/16] Replace `empty()` in UnixShell->getPhpcsVersion --- PhpcsChanged/UnixShell.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/PhpcsChanged/UnixShell.php b/PhpcsChanged/UnixShell.php index f84a01d..55688b8 100644 --- a/PhpcsChanged/UnixShell.php +++ b/PhpcsChanged/UnixShell.php @@ -419,7 +419,8 @@ public function getPhpcsVersion(): string { $matched = preg_match('/version\\s([0-9.]+)/uim', $versionPhpcsOutput, $matches); if ( $matched === false - || empty($matches[1]) + || count($matches) < 2 + || strlen($matches[1]) < 1 ) { throw new ShellException("Cannot parse phpcs version output"); }