Skip to content

Commit

Permalink
Replace empty() and some isset() calls (#100)
Browse files Browse the repository at this point in the history 
* 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
#99). This change should
have the same effect while not ignoring warnings.

* Replace `empty()` in other files with more explicit checks

Again, this is because `empty()` allows undefined values and hides
potential bugs from static analysis.

* Replace isset with array_key_exists in Cli

* Replace isset in addCacheEntry

* Replace isset with array_key_exists in CliOptions->fromArray

* Replace isset in CliOptions->toArray

* Replace isset with array_key_exists in FullReporter

* Replace isset in getPhpcsStandardOption

* Use boolval to make CliOptions->toArray more explicit

* Make json_encode guard more explicit in JsonReporter

* Make fileName guard in LintMessages more explicit

* Use more explicit guards in some UnixShell getters

* Make getExecutablePath guards more explicit

* Make getExecutablePath even more explicit

* 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.

* Replace `empty()` in UnixShell->getPhpcsVersion
  • Loading branch information
@sirbrillig
sirbrillig authored May 23, 2024
1 parent 3811a96 commit 7be9f99
Show file tree
Hide file tree
Showing 9 changed files with 121 additions and 90 deletions.
6 changes: 3 additions & 3 deletions PhpcsChanged/CacheManager.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
14 changes: 7 additions & 7 deletions PhpcsChanged/Cli.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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 = '';
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
Expand All @@ -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();
Expand Down
143 changes: 87 additions & 56 deletions PhpcsChanged/CliOptions.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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) {
Expand All @@ -309,33 +309,36 @@ 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) {
$options['diff'] = $this->diffFile;
$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;
}
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;
Expand All @@ -349,34 +352,62 @@ 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;
}
$env = getenv('PHPCS');
if (is_string($env) && strlen($env) > 0) {
return $env;
}
return 'phpcs';
case 'git':
return $this->gitPath ?: getenv('GIT') ?: 'git';
if (is_string($this->gitPath) && strlen($this->gitPath) > 0) {
return $this->gitPath;
}
$env = getenv('GIT');
if (is_string($env) && strlen($env) > 0) {
return $env;
}
return 'git';
case 'cat':
return $this->catPath ?: getenv('CAT') ?: 'cat';
if (is_string($this->catPath) && strlen($this->catPath) > 0) {
return $this->catPath;
}
$env = getenv('CAT');
if (is_string($env) && strlen($env) > 0) {
return $env;
}
return 'cat';
case 'svn':
return $this->svnPath ?: getenv('SVN') ?: 'svn';
if (is_string($this->svnPath) && strlen($this->svnPath) > 0) {
return $this->svnPath;
}
$env = getenv('SVN');
if (is_string($env) && strlen($env) > 0) {
return $env;
}
return 'svn';
default:
throw new \Exception("No executable found called '{$executableName}'.");
}
}

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.');
}
}
Expand Down
4 changes: 2 additions & 2 deletions PhpcsChanged/FullReporter.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
}

Expand Down Expand Up @@ -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));

Expand Down
4 changes: 2 additions & 2 deletions PhpcsChanged/JsonReporter.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
}

Expand Down Expand Up @@ -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;
Expand Down
Loading

0 comments on commit 7be9f99

Please sign in to comment.