🎨 Added PHPStan strict rules and address reported issues (#80)

dpi · Jun 18, 2022 · bc6b7cf · bc6b7cf
1 parent 5432564
commit bc6b7cf
Show file tree

Hide file tree

Showing 24 changed files with 74 additions and 55 deletions.
diff --git a/composer.json b/composer.json
@@ -22,7 +22,7 @@
         "phpspec/prophecy-phpunit": "^2.0",
         "psr/http-client": "^1.0",
         "psr/log": "^2.0",
-        "symfony/browser-kit": "^5.3",
+        "symfony/browser-kit": "^5.3.1",
         "symfony/console": "^5.3",
         "symfony/css-selector": "^5.3",
         "symfony/dom-crawler": "^5.4",
@@ -38,6 +38,7 @@
         "mikey179/vfsstream": "^1.6.10",
         "phpstan/extension-installer": "^1.1",
         "phpstan/phpstan": "1.7.14",
+        "phpstan/phpstan-strict-rules": "^1.2",
         "phpstan/phpstan-symfony": "^1",
         "phpunit/phpunit": "^9.5.20"
     },

diff --git a/phpstan.neon b/phpstan.neon
@@ -6,3 +6,5 @@ parameters:
 		- ./tests
 	symfony:
 		console_application_loader: phpstan/console-application.php
+	ignoreErrors:
+		- '#Dynamic call to static method PHPUnit\\Framework\\.*#'
diff --git a/src/Commands/IssueTimelineCommand.php b/src/Commands/IssueTimelineCommand.php
@@ -78,16 +78,16 @@ protected function execute(InputInterface $input, OutputInterface $output): int
             $comments[$event->getComment()->id()][] = $event;
         }
 
-        foreach ($comments as $events) {
-            $firstComment = reset($events)->getComment();
+        foreach ($comments as $commentEvents) {
+            $firstComment = reset($commentEvents)->getComment();
             $io->title(sprintf('<href=%s>Comment #%s (%d) at %s</>',
                 $firstComment->url(),
                 $firstComment->id(),
                 $firstComment->getSequence(),
                 $firstComment->getCreated()->format('r'),
             ));
-            foreach ($events as $event) {
-                $io->text((string) $event);
+            foreach ($commentEvents as $commentEvent) {
+                $io->text((string) $commentEvent);
             }
         }
 

diff --git a/src/Commands/Options/IssueMergeRequestOptions.php b/src/Commands/Options/IssueMergeRequestOptions.php
@@ -31,7 +31,7 @@ public static function fromInput(InputInterface $input): static
         $instance->cookies = $input->getOption(static::OPTION_COOKIE);
         $directory = (string) $input->getArgument(static::ARGUMENT_DIRECTORY);
         $cwd = \getcwd();
-        $instance->directory = ('.' === $directory && $cwd) ? $cwd : $directory;
+        $instance->directory = ('.' === $directory && is_string($cwd)) ? $cwd : $directory;
         $instance->isHttp = $input->getOption(static::OPTION_HTTP);
 
         $nid = $input->getArgument(static::ARGUMENT_ISSUE_ID);

diff --git a/src/Commands/Options/IssueTimelineCommandOptions.php b/src/Commands/Options/IssueTimelineCommandOptions.php
@@ -22,7 +22,7 @@ public static function fromInput(InputInterface $input): static
 
         /** @var string $nid */
         $nid = $input->getArgument(static::ARGUMENT_ISSUE_ID);
-        $instance->nid = preg_match('/^\d{1,10}$/m', $nid) ? (int) $nid : throw new \UnexpectedValueException('Issue ID is not valid');
+        $instance->nid = (1 === preg_match('/^\d{1,10}$/m', $nid)) ? (int) $nid : throw new \UnexpectedValueException('Issue ID is not valid');
         $instance->noComments = (bool) $input->getOption(static::OPTION_NO_COMMENTS);
         $instance->noEvents = (bool) $input->getOption(static::OPTION_NO_EVENTS);
 

diff --git a/src/Commands/Options/PatchToBranchOptions.php b/src/Commands/Options/PatchToBranchOptions.php
@@ -65,7 +65,7 @@ public static function fromInput(InputInterface $input): static
 
         // Check is a proper Composer constraint. e.g 8.8.x is not accepted:
         $constraint = (string) $input->getArgument(static::ARGUMENT_VERSION_CONSTRAINTS);
-        if (!empty($constraint)) {
+        if (strlen($constraint) > 0) {
             try {
                 (new VersionParser())->parseConstraints($constraint);
             } catch (\UnexpectedValueException $e) {

diff --git a/src/Commands/Options/ProjectMergeRequestOptions.php b/src/Commands/Options/ProjectMergeRequestOptions.php
@@ -33,7 +33,7 @@ public static function fromInput(InputInterface $input): static
         $instance->branchName = $input->getOption(static::OPTION_BRANCH);
         $directory = (string) $input->getArgument(static::ARGUMENT_DIRECTORY);
         $cwd = \getcwd();
-        $instance->directory = ('.' === $directory && $cwd) ? $cwd : $directory;
+        $instance->directory = ('.' === $directory && is_string($cwd)) ? $cwd : $directory;
         $instance->project = $input->getArgument(static::ARGUMENT_PROJECT);
         $instance->isHttp = $input->getOption(static::OPTION_HTTP);
         $instance->includeAll = (bool) $input->getOption(static::OPTION_ALL);

diff --git a/src/Commands/ProjectCloneCommand.php b/src/Commands/ProjectCloneCommand.php
@@ -51,14 +51,14 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 
         $url = sprintf($options->isHttp ? 'https://git.drupalcode.org/project/%s.git' : '[email protected]:project/%s.git', $options->project);
         $params = [];
-        if (!empty($options->branch)) {
+        if (null !== $options->branch && strlen($options->branch) > 0) {
             $params['-b'] = $options->branch;
         }
 
         // When directory isnt provided then use a directory with the same name as the project, if the directory
         // doesn't exist.
         $directory = $options->directory;
-        if (!$directory) {
+        if (null === $directory) {
             $directory = $options->project;
         }
 

diff --git a/src/DrupalOrg/IssueGraph/DrupalOrgIssueGraph.php b/src/DrupalOrg/IssueGraph/DrupalOrgIssueGraph.php
@@ -74,7 +74,7 @@ public function graph(): \Generator
             yield new CommentEvent($commentStub);
 
             // Detect merge requests.
-            if (count($branchMrs) > 0 && $repoUrlGit && $repoUrlHttp) {
+            if (count($branchMrs) > 0 && null !== $repoUrlGit && null !== $repoUrlHttp) {
                 $fieldItems = $commentCrawler->filter('.field-items > *');
                 foreach ($fieldItems as $fieldItem) {
                     if (str_contains($fieldItem->textContent, 'opened merge request !')) {
@@ -172,7 +172,7 @@ private function findComments(Crawler $crawler): array
         foreach ($crawler->filter('#content .comments > .comment') as $commentElement) {
             assert($commentElement instanceof \DOMElement);
             $id = $commentElement->getAttribute('id');
-            if (empty($id)) {
+            if (0 === strlen($id)) {
                 throw new \LogicException('All comments are expected to have an ID.');
             }
 

diff --git a/src/DrupalOrg/Objects/DrupalOrgComment.php b/src/DrupalOrg/Objects/DrupalOrgComment.php
@@ -22,7 +22,9 @@ public function __construct(protected int $id)
 
     public function getCreated(): \DateTimeImmutable
     {
-        !$this->isStub ?: throw new \DomainException('Data missing for stubs.');
+        if ($this->isStub) {
+            throw new \DomainException('Data missing for stubs.');
+        }
         $timestamp = $this->data->created ?? throw new \DomainException('Missing created date');
 
         return new \DateTimeImmutable('@' . $timestamp);
@@ -67,7 +69,9 @@ public function getAuthorId(): int
 
     public function getIssue(): DrupalOrgIssue
     {
-        !$this->isStub ?: throw new \DomainException('Data missing for stubs.');
+        if ($this->isStub) {
+            throw new \DomainException('Data missing for stubs.');
+        }
 
         return $this->repository->share(DrupalOrgIssue::fromStub($this->data->node));
     }
@@ -88,7 +92,7 @@ public function getComment(): string
     {
         $commentBody = $this->data->comment_body ?? throw new \DomainException('Data missing for stubs.');
 
-        return !empty($commentBody->value) ? $commentBody->value : '';
+        return isset($commentBody->value) && strlen($commentBody->value) > 0 ? $commentBody->value : '';
     }
 
     public function importResponse(ResponseInterface $response): static

diff --git a/src/DrupalOrg/Objects/DrupalOrgFile.php b/src/DrupalOrg/Objects/DrupalOrgFile.php
@@ -17,22 +17,28 @@ public function __construct(protected int $id)
 
     public function getMime(): string
     {
-        !$this->isStub ?: throw new \DomainException('Data missing for stubs.');
+        if ($this->isStub) {
+            throw new \DomainException('Data missing for stubs.');
+        }
 
         return $this->data->mime ?? throw new \DomainException('Missing mime type');
     }
 
     public function getCreated(): \DateTimeImmutable
     {
-        !$this->isStub ?: throw new \DomainException('Data missing for stubs.');
+        if ($this->isStub) {
+            throw new \DomainException('Data missing for stubs.');
+        }
         $timestamp = $this->data->timestamp ?? throw new \DomainException('Missing created date');
 
         return new \DateTimeImmutable('@' . $timestamp);
     }
 
     public function getUrl(): string
     {
-        !$this->isStub ?: throw new \DomainException('Data missing for stubs.');
+        if ($this->isStub) {
+            throw new \DomainException('Data missing for stubs.');
+        }
 
         return $this->data->url ?? throw new \DomainException('Missing URL');
     }

diff --git a/src/DrupalOrg/Objects/DrupalOrgIssue.php b/src/DrupalOrg/Objects/DrupalOrgIssue.php
@@ -17,15 +17,19 @@ public function __construct(protected int $id)
 
     public function getCreated(): \DateTimeImmutable
     {
-        !$this->isStub ?: throw new \DomainException('Data missing for stubs.');
+        if ($this->isStub) {
+            throw new \DomainException('Data missing for stubs.');
+        }
         $timestamp = $this->data->created ?? throw new \DomainException('Missing created date');
 
         return new \DateTimeImmutable('@' . $timestamp);
     }
 
     public function getCurrentVersion(): string
     {
-        !$this->isStub ?: throw new \DomainException('Data missing for stubs.');
+        if ($this->isStub) {
+            throw new \DomainException('Data missing for stubs.');
+        }
 
         return $this->data->field_issue_version ?? throw new \DomainException('Missing issue version');
     }

diff --git a/src/Git/GitOperator.php b/src/Git/GitOperator.php
@@ -17,17 +17,17 @@ public function __construct(protected GitRepository $gitRepository)
 
     public function isClean(): bool
     {
-        return empty($this->gitRepository->execute('status', '--porcelain'));
+        return 0 === count($this->gitRepository->execute('status', '--porcelain'));
     }
 
     public function resetHard(?string $treeIsh = null): bool
     {
         $args = ['reset', '--hard', '--quiet'];
-        if ($treeIsh) {
+        if (null !== $treeIsh) {
             $args[] = $treeIsh;
         }
 
-        return empty($this->gitRepository->execute(...$args));
+        return 0 === count($this->gitRepository->execute(...$args));
     }
 
     public function clean(): void
@@ -114,7 +114,7 @@ public function deleteBranch(string $branchName): void
     public function renameBranch(string $newBranchName, string $oldBranchName = null): void
     {
         $args = ['branch', '-M'];
-        if ($oldBranchName) {
+        if (null !== $oldBranchName) {
             $args[] = $oldBranchName;
         }
         $args[] = $newBranchName;

diff --git a/src/Git/GitResolver.php b/src/Git/GitResolver.php
@@ -24,7 +24,7 @@ public function getHash(): string
             sprintf('remotes/origin/%s', $this->patch->getGitReference()),
         ]);
 
-        if (!$return) {
+        if (0 === count($return)) {
             throw new \Exception('Failed to get hash.');
         }
 

diff --git a/src/HttplugBrowser.php b/src/HttplugBrowser.php
@@ -6,7 +6,9 @@
 
 use Http\Client\HttpAsyncClient;
 use Psr\Http\Message\RequestFactoryInterface;
+use Psr\Http\Message\RequestInterface;
 use Symfony\Component\BrowserKit\AbstractBrowser;
+use Symfony\Component\BrowserKit\Request;
 use Symfony\Component\BrowserKit\Response;
 
 final class HttplugBrowser extends AbstractBrowser
@@ -21,11 +23,10 @@ public function __construct(RequestFactoryInterface $httpFactory, HttpAsyncClien
         $this->httpClient = $httpClient;
     }
 
-    /**
-     * @param \Psr\Http\Message\RequestInterface|\Symfony\Component\BrowserKit\Request $request
-     */
-    protected function doRequest($request): Response
+    protected function doRequest(object $request): Response
     {
+        assert($request instanceof RequestInterface || $request instanceof Request);
+
         $response = $this->httpClient->sendAsyncRequest(
             $this->httpFactory->createRequest($request->getMethod(), $request->getUri())
         )->wait();

diff --git a/src/Listeners/PatchToBranch/Filter/ByConstraintOption.php b/src/Listeners/PatchToBranch/Filter/ByConstraintOption.php
@@ -18,7 +18,7 @@ public function __invoke(FilterEvent $event): void
         $event->logger->info('Filtering patches by constraint argument.');
 
         $versionConstraints = $event->options->versionConstraints;
-        if (!empty($versionConstraints)) {
+        if (strlen($versionConstraints) > 0) {
             $event->filter(fn (DrupalOrgPatch $patch): bool => Semver::satisfies(
                 str_replace('.x', '.9999', $patch->getVersion()),
                 $versionConstraints,

diff --git a/src/Listeners/PatchToBranch/Filter/PrimaryTrunk.php b/src/Listeners/PatchToBranch/Filter/PrimaryTrunk.php
@@ -27,7 +27,7 @@ public function __invoke(FilterEvent $event): void
         $versionHwm = null;
         $event->filter(function (DrupalOrgPatch $patch) use (&$versionHwm, &$secondaries): bool {
             $patchVersion = $patch->getVersion();
-            if ($versionHwm && Comparator::lessThan($patchVersion, $versionHwm)) {
+            if (null !== $versionHwm && strlen($versionHwm) > 0 && Comparator::lessThan($patchVersion, $versionHwm)) {
                 // Skip this one.
                 $secondaries[] = $patch;
 

diff --git a/src/Listeners/PatchToBranch/FilterByResponse/ByBody.php b/src/Listeners/PatchToBranch/FilterByResponse/ByBody.php
@@ -26,7 +26,7 @@ public function __invoke(FilterByResponseEvent $event): void
                 throw new \LogicException(sprintf('Missing patch contents for patch #%s %s', $patch->getParent()->getSequence(), $patch->getUrl()), 0, $e);
             }
 
-            if (empty($contents)) {
+            if (null === $contents || 0 === strlen($contents)) {
                 $logger->debug('Removed empty patch #{comment_id} {patch_url}.', [
                     'comment_id' => $patch->getParent()->getSequence(),
                     'patch_url' => $patch->getUrl(),

diff --git a/src/Listeners/PatchToBranch/GitApplyPatches/GitApplyPatches.php b/src/Listeners/PatchToBranch/GitApplyPatches/GitApplyPatches.php
@@ -28,19 +28,20 @@ public function __invoke(GitApplyPatchesEvent $event): void
         $patchLevel = $event->options->patchLevel;
         $linearMode = $event->linearMode;
 
-        if (0 === count($patches)) {
-            $output->writeln('No patches found.');
-            $event->setFailure();
-
-            return;
-        }
-
         // Resolve hashes.
+        /** @var array<array{\dogit\DrupalOrg\Objects\DrupalOrgPatch, string}> $hashes */
         $hashes = array_map(fn (DrupalOrgPatch $patch): array => [
             $patch,
             (new GitResolver($patch, $gitIo))->getHash(),
         ], $patches);
 
+        if (0 === count($hashes)) {
+            $output->writeln('No patches found.');
+            $event->setFailure();
+
+            return;
+        }
+
         $firstHash = reset($hashes)[1];
         $gitIo->resetHard($firstHash);
 
@@ -58,9 +59,8 @@ public function __invoke(GitApplyPatchesEvent $event): void
 
         $patchAllProgressBar->start();
         foreach ($hashes as [$patch, $hash]) {
-            assert($patch instanceof DrupalOrgPatch);
             $patchContents = $patch->getContents();
-            if (!$patchContents) {
+            if (null === $patchContents || 0 === strlen($patchContents)) {
                 // This should have been filtered by \dogit\Listeners\PatchToBranch\FilterByResponse\ByBody.
                 $io->warning(sprintf('Patch for comment %s is empty. Skipping.', $patch->getParent()->getSequence()));
 
@@ -79,7 +79,7 @@ public function __invoke(GitApplyPatchesEvent $event): void
             ];
 
             // Merge and reset state to without the changes from last patch.
-            if ($patchCommitHash) {
+            if (null !== $patchCommitHash && strlen($patchCommitHash) > 0) {
                 if (!$linearMode) {
                     $patchSingleProgressBar->setProgress(10);
                     $logger->info(sprintf('Merging into commit #%s before patch', substr($patchCommitHash, 0, 8)), $contextArgs);
@@ -93,7 +93,7 @@ public function __invoke(GitApplyPatchesEvent $event): void
                         $gitIo->commit([
                             '--amend',
                             '--allow-empty',
-                            ['--reuse-message', (string) $gitIo->getLastCommitId()],
+                            ['--reuse-message', $gitIo->getLastCommitId()],
                             ['--date', $patch->getCreated()->getTimestamp()],
                             ['--author', 'dogit <[email protected]>'],
                         ]);

diff --git a/src/Listeners/PatchToBranch/GitBranch/GitBranch.php b/src/Listeners/PatchToBranch/GitBranch/GitBranch.php
@@ -17,7 +17,7 @@ public function __invoke(GitBranchEvent $event): void
     {
         $branchName = $event->options->branchName;
 
-        if (empty($branchName)) {
+        if (null === $branchName || 0 === strlen($branchName)) {
             $branchName = 'dogit-' . $event->issue->id() . '-' . $event->initialGitReference;
         }
 
@@ -44,7 +44,7 @@ public function __invoke(GitBranchEvent $event): void
         $event->gitIo->checkoutNew($branchName, 'origin/' . $event->initialGitReference);
         $event->logger->info('Checked out branch: ' . $branchName);
 
-        if ($deleteBranchName) {
+        if (null !== $deleteBranchName && strlen($deleteBranchName) > 0) {
             $event->logger->info('Deleting old branch {branch_name}', [
                 'branch_name' => $deleteBranchName,
             ]);
@@ -54,7 +54,7 @@ public function __invoke(GitBranchEvent $event): void
 
     private function branchToDeleteSuffix(string $branchName): string
     {
-        return $this->branchToDeleteSuffixGenerator
+        return null !== $this->branchToDeleteSuffixGenerator
             ? ($this->branchToDeleteSuffixGenerator)($branchName)
             : $branchName . '-to-delete-' . (string) time();
     }

diff --git a/src/Listeners/PatchToBranch/Terminate/Statistics.php b/src/Listeners/PatchToBranch/Terminate/Statistics.php
@@ -7,7 +7,6 @@
 use dogit\DrupalOrg\Objects\DrupalOrgComment;
 use dogit\DrupalOrg\Objects\DrupalOrgFile;
 use dogit\DrupalOrg\Objects\DrupalOrgIssue;
-use dogit\DrupalOrg\Objects\DrupalOrgObject;
 use dogit\DrupalOrg\Objects\DrupalOrgPatch;
 use dogit\Events\PatchToBranch\TerminateEvent;
 
@@ -21,7 +20,6 @@ public function __invoke(TerminateEvent $event): void
 
         $objectCounter = [];
         foreach ($event->repository->all() as $object) {
-            assert($object instanceof DrupalOrgObject);
             $objectCounter[$object::class] = ($objectCounter[$object::class] ?? 0) + 1;
         }
         $event->io->definitionList(