Integrate phpstan, psalm and fix factory method return types (#63)

ua-parser · Aug 5, 2020 · 417e65e · 417e65e
1 parent f2664da
commit 417e65e
Show file tree

Hide file tree

Showing 23 changed files with 171 additions and 106 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -22,5 +22,7 @@ install:
 
 script:
   - ./vendor/bin/phpunit
+  - ./vendor/bin/psalm
+  - ./vendor/bin/phpstan analyse -l 5 src
 
 sudo: false
diff --git a/composer.json b/composer.json
@@ -6,11 +6,13 @@
         "composer/ca-bundle": "^1.1"
     },
     "require-dev": {
-        "phpunit/phpunit": "^7 || ^8 || ^9",
-        "symfony/yaml": "^3.4 || ^4.3 || ^5.0",
-        "symfony/filesystem": "^3.4 || ^4.3 || ^5.0",
-        "symfony/finder": "^3.4 || ^4.3 || ^5.0",
-        "symfony/console": "^3.4 || ^4.3 || ^5.0"
+        "phpunit/phpunit": "^8 || ^9",
+        "symfony/yaml": "^3.4 || ^4.2 || ^4.3 || ^5.0",
+        "symfony/filesystem": "^3.4 || ^4.2 ||  ^4.3 || ^5.0",
+        "symfony/finder": "^3.4 || ^4.2 || ^4.3 || ^5.0",
+        "symfony/console": "^3.4 || ^4.2 || ^4.3 || ^5.0",
+        "phpstan/phpstan": "^0.12.33",
+        "vimeo/psalm": "^3.12"
     },
     "suggest": {
         "symfony/yaml": "Required for CLI usage - ^3.4 || ^4.3 || ^5.0",

diff --git a/psalm.xml b/psalm.xml
@@ -0,0 +1,15 @@
+<?xml version="1.0"?>
+<psalm
+    errorLevel="3"
+    resolveFromConfigFile="true"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xmlns="https://getpsalm.org/schema/config"
+    xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
+>
+    <projectFiles>
+        <directory name="src" />
+        <ignoreFiles>
+            <directory name="vendor" />
+        </ignoreFiles>
+    </projectFiles>
+</psalm>
diff --git a/src/AbstractParser.php b/src/AbstractParser.php
@@ -14,9 +14,6 @@
 
 abstract class AbstractParser
 {
-    /** @var string */
-    public static $defaultFile;
-
     /** @var array */
     protected $regexes = array();
 
@@ -25,49 +22,6 @@ public function __construct(array $regexes)
         $this->regexes = $regexes;
     }
 
-    /**
-     * Create parser instance
-     *
-     * Either pass a custom regexes.php file or leave the argument empty and use the default file.
-     * @throws FileNotFoundException
-     */
-    public static function create(?string $file = null): self
-    {
-        return $file ? static::createCustom($file) : static::createDefault();
-    }
-
-    /** @throws FileNotFoundException */
-    protected static function createDefault(): self
-    {
-        return static::createInstance(
-            static::getDefaultFile(),
-            [FileNotFoundException::class, 'defaultFileNotFound']
-        );
-    }
-
-    /** @throws FileNotFoundException */
-    protected static function createCustom(string $file): self
-    {
-        return static::createInstance(
-            $file,
-            [FileNotFoundException::class, 'customRegexFileNotFound']
-        );
-    }
-
-    private static function createInstance(string $file, callable $exceptionFactory): self
-    {
-        if (!file_exists($file)) {
-            throw $exceptionFactory($file);
-        }
-
-        static $map = [];
-        if (!isset($map[$file])) {
-            $map[$file] = include $file;
-        }
-
-        return new static($map[$file]);
-    }
-
     protected static function tryMatch(array $regexes, string $userAgent): array
     {
         foreach ($regexes as $regex) {
@@ -108,13 +62,8 @@ static function ($m) use ($matches) {
 
     private static function emptyStringToNull(?string $string): ?string
     {
-        $string = trim($string);
+        $string = trim($string ?? '');
 
         return $string === '' ? null : $string;
     }
-
-    protected static function getDefaultFile(): string
-    {
-        return static::$defaultFile ?: dirname(__DIR__).'/resources'.DIRECTORY_SEPARATOR.'regexes.php';
-    }
 }
diff --git a/src/Command/ConvertCommand.php b/src/Command/ConvertCommand.php
@@ -52,7 +52,11 @@ protected function configure(): void
 
     protected function execute(InputInterface $input, OutputInterface $output): int
     {
-        $this->getConverter()->convertFile($input->getArgument('file'), $input->getOption('no-backup'));
+        $file = $input->getArgument('file');
+        assert(is_string($file));
+        $noBackup = $input->getOption('no-backup');
+        assert(is_bool($noBackup));
+        $this->getConverter()->convertFile($file, $noBackup);
 
         return 0;
     }

diff --git a/src/Command/FetchCommand.php b/src/Command/FetchCommand.php
@@ -42,9 +42,10 @@ protected function configure(): void
 
     protected function execute(InputInterface $input, OutputInterface $output): int
     {
-        $fs = new Filesystem();
-        $fetcher = new Fetcher();
-        $fs->dumpFile($input->getArgument('file'), $fetcher->fetch());
+        $file = $input->getArgument('file');
+        assert(is_string($file));
+
+        (new Filesystem())->dumpFile($file, (new Fetcher())->fetch());
 
         return 0;
     }

diff --git a/src/Command/LogfileCommand.php b/src/Command/LogfileCommand.php
@@ -71,7 +71,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 
         $parser = Parser::create();
         $undefinedClients = array();
-        /** @var $file SplFileInfo */
         foreach ($this->getFiles($input) as $file) {
 
             $path = $this->getPath($file);
@@ -85,8 +84,9 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 
             $firstLine = reset($lines);
 
-            $reader = AbstractReader::factory($firstLine);
-            if (!$reader) {
+            try {
+                $reader = AbstractReader::factory($firstLine);
+            } catch (ReaderException $e) {
                 $output->writeln(sprintf('Could not find reader for file "%s"', $file->getPathname()));
                 $output->writeln('');
                 continue;
@@ -124,16 +124,18 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 
         $undefinedClients = $this->filter($undefinedClients);
 
-        $fs = new Filesystem();
-        $fs->dumpFile($input->getArgument('output'), implode(PHP_EOL, $undefinedClients));
+
+        $outputFile = $input->getArgument('output');
+        assert(is_string($outputFile));
+        (new Filesystem())->dumpFile($outputFile, implode(PHP_EOL, $undefinedClients));
 
         return 0;
     }
 
     private function outputProgress(OutputInterface $output, string $result, int $count, int $totalCount, bool $end = false): int
     {
         if (($count % 70) === 0 || $end) {
-            $formatString = '%s  %'.strlen($totalCount).'d / %-'.strlen($totalCount).'d (%3d%%)';
+            $formatString = '%s  %'.strlen((string)$totalCount).'d / %-'.strlen((string)$totalCount).'d (%3d%%)';
             $result = $end ? str_repeat(' ', 70 - ($count % 70)) : $result;
             $output->writeln(sprintf($formatString, $result, $count, $totalCount, $count / $totalCount * 100));
         } else {
@@ -164,20 +166,21 @@ private function getResult(Client $client): string
         return '.';
     }
 
-    private function getFiles(InputInterface $input): Finder
+    /** @psalm-return array<array-key, SplFileInfo>|iterable */
+    private function getFiles(InputInterface $input): iterable
     {
         $finder = Finder::create();
 
-        if ($input->getOption('log-file')) {
-            $file = $input->getOption('log-file');
-            $finder->append(Finder::create()->in(dirname($file))->name(basename($file)));
+        $logFile = $input->getOption('log-file');
+        if (is_string($logFile)) {
+            $finder->append(Finder::create()->in(dirname($logFile))->name(basename($logFile)));
         }
 
-        if ($input->getOption('log-dir')) {
-            $dirFinder = Finder::create()
-                ->in($input->getOption('log-dir'));
-            array_map(array($dirFinder, 'name'), $input->getOption('include'));
-            array_map(array($dirFinder, 'notName'), $input->getOption('exclude'));
+        $logDir = $input->getOption('log-dir');
+        if (is_string($logDir)) {
+            $dirFinder = Finder::create()->in($logDir);
+            array_map(array($dirFinder, 'name'), array_filter((array)$input->getOption('include'),'is_string'));
+            array_map(array($dirFinder, 'notName'), array_filter((array)$input->getOption('exclude'),'is_string'));
 
             $finder->append($dirFinder);
         }

diff --git a/src/Command/ParserCommand.php b/src/Command/ParserCommand.php
@@ -23,7 +23,6 @@ protected function configure()
             ->setDescription('Parses a user agent string and dumps the results.')
             ->addArgument(
                 'user-agent',
-                null,
                 InputArgument::REQUIRED,
                 'User agent string to analyze'
             )
@@ -32,7 +31,9 @@ protected function configure()
 
     protected function execute(InputInterface $input, OutputInterface $output): int
     {
-        $result = Parser::create()->parse($input->getArgument('user-agent'));
+        $userAgent = $input->getArgument('user-agent');
+        assert(is_string($userAgent));
+        $result = Parser::create()->parse($userAgent);
 
         $output->writeln(json_encode($result, JSON_PRETTY_PRINT));
 

diff --git a/src/DeviceParser.php b/src/DeviceParser.php
@@ -14,6 +14,8 @@
 
 class DeviceParser extends AbstractParser
 {
+    use ParserFactoryMethods;
+
     /** Attempts to see if the user agent matches a device regex from regexes.php */
     public function parseDevice(string $userAgent): Device
     {
@@ -22,7 +24,7 @@ public function parseDevice(string $userAgent): Device
         [$regex, $matches] = self::tryMatch($this->regexes['device_parsers'], $userAgent);
 
         if ($matches) {
-            $device->family = self::multiReplace($regex, 'device_replacement', $matches[1], $matches);
+            $device->family = self::multiReplace($regex, 'device_replacement', $matches[1], $matches) ?? $device->family;
             $device->brand = self::multiReplace($regex, 'brand_replacement', null, $matches);
             $deviceModelDefault = $matches[1] !== 'Other' ? $matches[1] : null;
             $device->model = self::multiReplace($regex, 'model_replacement', $deviceModelDefault, $matches);

diff --git a/src/Exception/FetcherException.php b/src/Exception/FetcherException.php
@@ -8,11 +8,11 @@
  */
 namespace UAParser\Exception;
 
-class FetcherException extends DomainException
+final class FetcherException extends DomainException
 {
     public static function httpError(string $resource, string $error): self
     {
-        return new static(
+        return new self(
             sprintf('Could not fetch HTTP resource "%s": %s', $resource, $error)
         );
     }

diff --git a/src/Exception/FileNotFoundException.php b/src/Exception/FileNotFoundException.php
@@ -10,16 +10,16 @@
 
 use Exception;
 
-class FileNotFoundException extends Exception
+final class FileNotFoundException extends Exception
 {
     public static function fileNotFound(string $file): self
     {
-        return new static(sprintf('File "%s" does not exist', $file));
+        return new self(sprintf('File "%s" does not exist', $file));
     }
 
     public static function customRegexFileNotFound(string $file): self
     {
-        return new static(
+        return new self(
             sprintf(
                 'ua-parser cannot find the custom regexes file you supplied ("%s"). Please make sure you have the correct path.',
                 $file
@@ -29,7 +29,7 @@ public static function customRegexFileNotFound(string $file): self
 
     public static function defaultFileNotFound(string $file): self
     {
-        return new static(
+        return new self(
             sprintf(
                 'Please download the "%s" file before using ua-parser by running "php bin/uaparser ua-parser:update"',
                 $file

diff --git a/src/Exception/InvalidArgumentException.php b/src/Exception/InvalidArgumentException.php
@@ -10,11 +10,11 @@
 
 use InvalidArgumentException as BaseInvalidArgumentException;
 
-class InvalidArgumentException extends BaseInvalidArgumentException
+final class InvalidArgumentException extends BaseInvalidArgumentException
 {
     public static function oneOfCommandArguments(string ...$args): self
     {
-        return new static(
+        return new self(
             sprintf('One of the command arguments "%s" is required', implode('", "', $args))
         );
     }

diff --git a/src/Exception/ReaderException.php b/src/Exception/ReaderException.php
@@ -8,10 +8,15 @@
  */
 namespace UAParser\Exception;
 
-class ReaderException extends DomainException
+final class ReaderException extends DomainException
 {
     public static function userAgentParserError(string $line): self
     {
-        return new static(sprintf('Cannot extract user agent string from line "%s"', $line));
+        return new self(sprintf('Cannot extract user agent string from line "%s"', $line));
+    }
+
+    public static function readerNotFound(string $line): self
+    {
+        return new self(sprintf('Cannot find reader that can handle "%s"', $line));
     }
 }
diff --git a/src/OperatingSystemParser.php b/src/OperatingSystemParser.php
@@ -14,6 +14,8 @@
 
 class OperatingSystemParser extends AbstractParser
 {
+    use ParserFactoryMethods;
+
     /** Attempts to see if the user agent matches an operating system regex from regexes.php */
     public function parseOperatingSystem(string $userAgent): OperatingSystem
     {
@@ -22,7 +24,7 @@ public function parseOperatingSystem(string $userAgent): OperatingSystem
         [$regex, $matches] = self::tryMatch($this->regexes['os_parsers'], $userAgent);
 
         if ($matches) {
-            $os->family = self::multiReplace($regex, 'os_replacement', $matches[1], $matches);
+            $os->family = self::multiReplace($regex, 'os_replacement', $matches[1], $matches) ?? $os->family;
             $os->major = self::multiReplace($regex, 'os_v1_replacement', $matches[2], $matches);
             $os->minor = self::multiReplace($regex, 'os_v2_replacement', $matches[3], $matches);
             $os->patch = self::multiReplace($regex, 'os_v3_replacement', $matches[4], $matches);

diff --git a/src/Parser.php b/src/Parser.php
@@ -13,6 +13,8 @@
 
 class Parser extends AbstractParser
 {
+    use ParserFactoryMethods;
+
     /** @var DeviceParser */
     private $deviceParser;