Install & Use quality tools (#277)

* Install squizlabs/php_codesniffer to provide checkstyle rules to code base * Apply PSR12 code style to sources * Install phpstan/phpstan to provide static analysis & fixes all spotted errors * Run checkstyle & static analysis with Github Actions * Remove mocks from vendor services
prestaconcept · Apr 9, 2021 · 4537ed9 · 4537ed9
1 parent 2663b36
commit 4537ed9
Show file tree

Hide file tree

Showing 44 changed files with 656 additions and 441 deletions.
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
@@ -66,14 +66,14 @@ jobs:
           coverage: xdebug
           php-version: ${{ matrix.php-version }}
 
-      - name: "Require symfony/messenger dependencies when possible"
-        if: matrix.symfony-version != '3.4.*'
-        run: |
-          composer require symfony/messenger:${{ matrix.symfony-version }} --no-interaction --no-update
-
       - name: "Install dependencies with composer"
         run: |
-          composer require symfony/console:${{ matrix.symfony-version }} symfony/framework-bundle:${{ matrix.symfony-version }} symfony/http-kernel:${{ matrix.symfony-version }} symfony/routing:${{ matrix.symfony-version }} --no-interaction --no-update
+          composer require --no-interaction --no-update \
+            symfony/console:${{ matrix.symfony-version }} \
+            symfony/framework-bundle:${{ matrix.symfony-version }} \
+            symfony/http-kernel:${{ matrix.symfony-version }} \
+            symfony/routing:${{ matrix.symfony-version }} \
+            symfony/messenger:${{ matrix.symfony-version }}
           composer update --no-interaction --no-progress --no-suggest
 
       - name: "Run tests with phpunit/phpunit"
@@ -84,3 +84,69 @@ jobs:
 
       - name: "Upload coverage to Codecov"
         uses: codecov/codecov-action@v1
+
+  phpstan:
+    name: "PhpStan"
+    runs-on: ubuntu-latest
+
+    strategy:
+      matrix:
+        include:
+          - php-version: 7.4
+            symfony-version: 5.2.*
+
+    steps:
+      - name: "Checkout"
+        uses: actions/checkout@v2
+
+      - name: "Setup PHP"
+        uses: shivammathur/setup-php@v2
+        with:
+          coverage: none
+          php-version: ${{ matrix.php-version }}
+
+      - name: "Install dependencies with composer"
+        run: |
+          composer require --no-interaction --no-update \
+            symfony/console:${{ matrix.symfony-version }} \
+            symfony/framework-bundle:${{ matrix.symfony-version }} \
+            symfony/http-kernel:${{ matrix.symfony-version }} \
+            symfony/routing:${{ matrix.symfony-version }} \
+            symfony/messenger:${{ matrix.symfony-version }}
+          composer update --no-interaction --no-progress --no-suggest
+
+      - name: "Run static analysis with phpstan/phpstan"
+        run: vendor/bin/phpstan analyze
+
+  checkstyke:
+    name: "Checkstyle"
+    runs-on: ubuntu-latest
+
+    strategy:
+      matrix:
+        include:
+          - php-version: 7.4
+            symfony-version: 5.2.*
+
+    steps:
+      - name: "Checkout"
+        uses: actions/checkout@v2
+
+      - name: "Setup PHP"
+        uses: shivammathur/setup-php@v2
+        with:
+          coverage: none
+          php-version: ${{ matrix.php-version }}
+
+      - name: "Install dependencies with composer"
+        run: |
+          composer require --no-interaction --no-update \
+            symfony/console:${{ matrix.symfony-version }} \
+            symfony/framework-bundle:${{ matrix.symfony-version }} \
+            symfony/http-kernel:${{ matrix.symfony-version }} \
+            symfony/routing:${{ matrix.symfony-version }} \
+            symfony/messenger:${{ matrix.symfony-version }}
+          composer update --no-interaction --no-progress --no-suggest
+
+      - name: "Run checkstyle with squizlabs/php_codesniffer"
+        run: vendor/bin/phpcs
diff --git a/.gitignore b/.gitignore
@@ -1,5 +1,5 @@
 /.idea/
+/.phpcs-cache
+/.phpunit.result.cache
 /composer.lock
-/phpunit.xml
 /vendor/
-/var/
diff --git a/composer.json b/composer.json
@@ -21,7 +21,9 @@
     },
     "require-dev": {
         "doctrine/annotations": "^1.0",
+        "phpstan/phpstan": "^0.12.82",
         "phpunit/phpunit": "^7.5",
+        "squizlabs/php_codesniffer": "^3.5",
         "symfony/messenger": "^4.4|^5.0",
         "symfony/browser-kit": "^4.4|^5.0",
         "symfony/phpunit-bridge": "^4.4|^5.0",

diff --git a/phpcs.xml.dist b/phpcs.xml.dist
@@ -0,0 +1,21 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:noNamespaceSchemaLocation="vendor/squizlabs/php_codesniffer/phpcs.xsd">
+
+  <arg name="basepath" value="."/>
+  <arg name="cache" value=".phpcs-cache"/>
+  <arg name="colors"/>
+  <arg name="extensions" value="php"/>
+
+  <rule ref="PSR12"/>
+
+  <file>src/</file>
+
+  <rule ref="Generic.PHP.ForbiddenFunctions">
+    <properties>
+      <property name="forbiddenFunctions" type="array" value="dump=>null,var_dump=>null,die=>null"/>
+    </properties>
+  </rule>
+
+</ruleset>
diff --git a/phpstan.neon.dist b/phpstan.neon.dist
@@ -0,0 +1,23 @@
+parameters:
+  level: max
+  paths:
+    - src/
+
+  ignoreErrors:
+    # config definition is not well parsed
+    - '#Symfony\\Component\\Config\\Definition#'
+
+    # issues from vendor using array that has no iterable specification
+    -
+        message: "#^Method Presta\\\\SitemapBundle\\\\DependencyInjection\\\\PrestaSitemapExtension\\:\\:load\\(\\) has parameter \\$configs with no value type specified in iterable type array\\.$#"
+        count: 1
+        path: src/DependencyInjection/PrestaSitemapExtension.php
+    -
+        message: "#^Method Presta\\\\SitemapBundle\\\\EventListener\\\\RouteAnnotationEventListener\\:\\:getSubscribedEvents\\(\\) return type has no value type specified in iterable type array\\.$#"
+        count: 1
+        path: src/EventListener/RouteAnnotationEventListener.php
+    -
+        message: "#^Method Presta\\\\SitemapBundle\\\\EventListener\\\\StaticRoutesAlternateEventListener\\:\\:getSubscribedEvents\\(\\) return type has no value type specified in iterable type array\\.$#"
+        count: 1
+        path: src/EventListener/StaticRoutesAlternateEventListener.php
+
diff --git a/src/Command/DumpSitemapsCommand.php b/src/Command/DumpSitemapsCommand.php
@@ -44,7 +44,7 @@ class DumpSitemapsCommand extends Command
      */
     private $defaultTarget;
 
-    public function __construct(RouterInterface $router, DumperInterface $dumper, $defaultTarget)
+    public function __construct(RouterInterface $router, DumperInterface $dumper, string $defaultTarget)
     {
         $this->router = $router;
         $this->dumper = $dumper;
@@ -56,7 +56,7 @@ public function __construct(RouterInterface $router, DumperInterface $dumper, $d
     /**
      * @inheritdoc
      */
-    protected function configure()
+    protected function configure(): void
     {
         $this
             ->setDescription('Dumps sitemaps to given location')
@@ -70,7 +70,8 @@ protected function configure()
                 'base-url',
                 null,
                 InputOption::VALUE_REQUIRED,
-                'Base url to use for absolute urls. Good example - http://acme.com/, bad example - acme.com. Defaults to router.request_context.host parameter'
+                'Base url to use for absolute urls. Good example - http://acme.com/, bad example - acme.com.' .
+                ' Defaults to router.request_context.host parameter'
             )
             ->addOption(
                 'gzip',
@@ -89,11 +90,15 @@ protected function configure()
     /**
      * @inheritdoc
      */
-    protected function execute(InputInterface $input, OutputInterface $output) : int
+    protected function execute(InputInterface $input, OutputInterface $output): int
     {
-        $targetDir = rtrim($input->getArgument('target'), '/');
+        /** @var string $targetDir */
+        $targetDir = $input->getArgument('target');
+        $targetDir = rtrim($targetDir, '/');
 
-        if ($baseUrl = $input->getOption('base-url')) {
+        /** @var string|null $baseUrl */
+        $baseUrl = $input->getOption('base-url');
+        if ($baseUrl) {
             $baseUrl = rtrim($baseUrl, '/') . '/';
 
             //sanity check
@@ -112,11 +117,13 @@ protected function execute(InputInterface $input, OutputInterface $output) : int
             $baseUrl = $this->getBaseUrl();
         }
 
-        if ($input->getOption('section')) {
+        /** @var string|null $section */
+        $section = $input->getOption('section');
+        if ($section) {
             $output->writeln(
                 sprintf(
                     "Dumping sitemaps section <comment>%s</comment> into <comment>%s</comment> directory",
-                    $input->getOption('section'),
+                    $section,
                     $targetDir
                 )
             );
@@ -128,13 +135,17 @@ protected function execute(InputInterface $input, OutputInterface $output) : int
                 )
             );
         }
+
         $options = [
-            'gzip' => (Boolean)$input->getOption('gzip'),
+            'gzip' => (bool)$input->getOption('gzip'),
         ];
-        $filenames = $this->dumper->dump($targetDir, $baseUrl, $input->getOption('section'), $options);
+        $filenames = $this->dumper->dump($targetDir, $baseUrl, $section, $options);
 
-        if ($filenames === false) {
-            $output->writeln("<error>No URLs were added to sitemap by EventListeners</error> - this may happen when provided section is invalid");
+        if (!is_array($filenames)) {
+            $output->writeln(
+                "<error>No URLs were added to sitemap by EventListeners</error>" .
+                " - this may happen when provided section is invalid"
+            );
 
             return 1;
         }
@@ -147,10 +158,7 @@ protected function execute(InputInterface $input, OutputInterface $output) : int
         return 0;
     }
 
-    /**
-     * @return string
-     */
-    private function getBaseUrl()
+    private function getBaseUrl(): string
     {
         $context = $this->router->getContext();
 
@@ -164,9 +172,9 @@ private function getBaseUrl()
         $port = '';
 
         if ('http' === $scheme && 80 != $context->getHttpPort()) {
-            $port = ':'.$context->getHttpPort();
+            $port = ':' . $context->getHttpPort();
         } elseif ('https' === $scheme && 443 != $context->getHttpsPort()) {
-            $port = ':'.$context->getHttpsPort();
+            $port = ':' . $context->getHttpsPort();
         }
 
         return rtrim($scheme . '://' . $host . $port, '/') . '/';

diff --git a/src/Controller/SitemapController.php b/src/Controller/SitemapController.php
@@ -34,10 +34,7 @@ class SitemapController
      */
     private $ttl;
 
-    /**
-     * @param int $ttl
-     */
-    public function __construct(GeneratorInterface $generator, $ttl)
+    public function __construct(GeneratorInterface $generator, int $ttl)
     {
         $this->generator = $generator;
         $this->ttl = $ttl;
@@ -48,7 +45,7 @@ public function __construct(GeneratorInterface $generator, $ttl)
      *
      * @return Response
      */
-    public function indexAction()
+    public function indexAction(): Response
     {
         $sitemapindex = $this->generator->fetch('root');
 
@@ -71,7 +68,7 @@ public function indexAction()
      *
      * @return Response
      */
-    public function sectionAction($name)
+    public function sectionAction(string $name): Response
     {
         $section = $this->generator->fetch($name);
 

diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php
@@ -16,19 +16,18 @@
 use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition;
 use Symfony\Component\Config\Definition\Builder\TreeBuilder;
 use Symfony\Component\Config\Definition\ConfigurationInterface;
-use Symfony\Component\HttpKernel\Kernel;
 
 /**
  * This is the class that validates and merges configuration from your app/config files
  */
 class Configuration implements ConfigurationInterface
 {
-    const DEFAULT_FILENAME = 'sitemap';
+    public const DEFAULT_FILENAME = 'sitemap';
 
     /**
      * @inheritDoc
      */
-    public function getConfigTreeBuilder()
+    public function getConfigTreeBuilder(): TreeBuilder
     {
         $treeBuilder = new TreeBuilder('presta_sitemap');
         $rootNode = $treeBuilder->getRootNode();
@@ -42,7 +41,10 @@ public function getConfigTreeBuilder()
                 ->end()
                 ->scalarNode('sitemap_file_prefix')
                     ->defaultValue(self::DEFAULT_FILENAME)
-                    ->info('Sets sitemap filename prefix defaults to "sitemap" -> sitemap.xml (for index); sitemap.<section>.xml(.gz) (for sitemaps)')
+                    ->info(
+                        'Sets sitemap filename prefix defaults to "sitemap" -> sitemap.xml (for index);' .
+                        ' sitemap.<section>.xml(.gz) (for sitemaps)'
+                    )
                 ->end()
                 ->integerNode('items_by_set')
                     // Add one to the limit items value because it's an
@@ -53,13 +55,11 @@ public function getConfigTreeBuilder()
                 ->scalarNode('route_annotation_listener')->defaultTrue()->end()
                 ->scalarNode('dump_directory')
                     ->info(
-                        'The directory to which the sitemap will be dumped. '.
-                        'It can be either absolute, or relative (to the place where the command will be triggered). '.
-                        'Default to Symfony\'s public dir.'
-                    )
-                    ->defaultValue(
-                        '%kernel.project_dir%/'.(version_compare(Kernel::VERSION, '4.0') >= 0 ? 'public' : 'web')
+                        'The directory to which the sitemap will be dumped.' .
+                        ' It can be either absolute, or relative (to the place where the command will be triggered).' .
+                        ' Default to Symfony\'s public dir.'
                     )
+                    ->defaultValue('%kernel.project_dir%/public')
                 ->end()
                 ->arrayNode('defaults')
                     ->addDefaultsIfNotSet()
@@ -81,14 +81,14 @@ public function getConfigTreeBuilder()
         return $treeBuilder;
     }
 
-    private function addAlternateSection(ArrayNodeDefinition $rootNode)
+    private function addAlternateSection(ArrayNodeDefinition $rootNode): void
     {
         $rootNode
             ->children()
                 ->arrayNode('alternate')
                     ->info(
                         'Automatically generate alternate (hreflang) urls with static routes.' .
-                       ' Requires route_annotation_listener config to be enabled.'
+                        ' Requires route_annotation_listener config to be enabled.'
                     )
                     ->canBeEnabled()
                     ->children()
@@ -100,7 +100,11 @@ private function addAlternateSection(ArrayNodeDefinition $rootNode)
                             ->defaultValue(['en'])
                             ->beforeNormalization()
                             ->ifString()
-                                ->then(function ($v) { return preg_split('/\s*,\s*/', $v); })
+                                ->then(
+                                    function ($v) {
+                                        return preg_split('/\s*,\s*/', $v);
+                                    }
+                                )
                             ->end()
                             ->prototype('scalar')->end()
                             ->info('List of supported locales of your routes.')

diff --git a/src/DependencyInjection/PrestaSitemapExtension.php b/src/DependencyInjection/PrestaSitemapExtension.php
@@ -25,7 +25,7 @@ class PrestaSitemapExtension extends Extension
     /**
      * @inheritDoc
      */
-    public function load(array $configs, ContainerBuilder $container)
+    public function load(array $configs, ContainerBuilder $container): void
     {
         $configuration = new Configuration();
         $config = $this->processConfiguration($configuration, $configs);