Prepare generators arguments order change in 4.0 (prestaconcept#319)

* Prepare generators arguments order change in 4.0 * Add phpstan typesafe errors to baseline * Prepare SitemapPopulateEvent constructor arguments order change in 4.0 --------- Co-authored-by: Yann Eugoné <[email protected]>
m0nken · Oct 9, 2023 · 32cb887 · 32cb887
1 parent d741a34
commit 32cb887
Show file tree

Hide file tree

Showing 9 changed files with 200 additions and 16 deletions.
diff --git a/config/services.xml b/config/services.xml
@@ -23,9 +23,9 @@
         <service id="presta_sitemap.dumper_default" class="%presta_sitemap.dumper.class%">
             <argument id="event_dispatcher" type="service" />
             <argument id="filesystem" type="service" />
+            <argument id="router" type="service" />
             <argument>%presta_sitemap.sitemap_file_prefix%</argument>
             <argument>%presta_sitemap.items_by_set%</argument>
-            <argument id="router" type="service" />
             <call method="setDefaults">
                 <argument>%presta_sitemap.defaults%</argument>
             </call>

diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -0,0 +1,76 @@
+parameters:
+	ignoreErrors:
+		-
+			message: "#^Call to function is_string\\(\\) with Symfony\\\\Component\\\\Routing\\\\Generator\\\\UrlGeneratorInterface will always evaluate to false\\.$#"
+			count: 1
+			path: src/Event/SitemapPopulateEvent.php
+
+		-
+			message: "#^Else branch is unreachable because ternary operator condition is always true\\.$#"
+			count: 2
+			path: src/Event/SitemapPopulateEvent.php
+
+		-
+			message: "#^Instanceof between string and Symfony\\\\Component\\\\Routing\\\\Generator\\\\UrlGeneratorInterface will always evaluate to false\\.$#"
+			count: 1
+			path: src/Event/SitemapPopulateEvent.php
+
+		-
+			message: "#^Result of && is always false\\.$#"
+			count: 2
+			path: src/Event/SitemapPopulateEvent.php
+
+		-
+			message: "#^Call to function is_int\\(\\) with Symfony\\\\Component\\\\Routing\\\\Generator\\\\UrlGeneratorInterface will always evaluate to false\\.$#"
+			count: 1
+			path: src/Service/AbstractGenerator.php
+
+		-
+			message: "#^Else branch is unreachable because ternary operator condition is always true\\.$#"
+			count: 2
+			path: src/Service/AbstractGenerator.php
+
+		-
+			message: "#^Instanceof between int and Symfony\\\\Component\\\\Routing\\\\Generator\\\\UrlGeneratorInterface will always evaluate to false\\.$#"
+			count: 1
+			path: src/Service/AbstractGenerator.php
+
+		-
+			message: "#^Result of && is always false\\.$#"
+			count: 2
+			path: src/Service/AbstractGenerator.php
+
+		-
+			message: "#^Call to function is_int\\(\\) with string will always evaluate to false\\.$#"
+			count: 1
+			path: src/Service/Dumper.php
+
+		-
+			message: "#^Call to function is_null\\(\\) with string will always evaluate to false\\.$#"
+			count: 1
+			path: src/Service/Dumper.php
+
+		-
+			message: "#^Call to function is_string\\(\\) with Symfony\\\\Component\\\\Routing\\\\Generator\\\\UrlGeneratorInterface\\|null will always evaluate to false\\.$#"
+			count: 1
+			path: src/Service/Dumper.php
+
+		-
+			message: "#^Else branch is unreachable because ternary operator condition is always true\\.$#"
+			count: 3
+			path: src/Service/Dumper.php
+
+		-
+			message: "#^Instanceof between int and Symfony\\\\Component\\\\Routing\\\\Generator\\\\UrlGeneratorInterface will always evaluate to false\\.$#"
+			count: 1
+			path: src/Service/Dumper.php
+
+		-
+			message: "#^Result of && is always false\\.$#"
+			count: 4
+			path: src/Service/Dumper.php
+
+		-
+			message: "#^Result of \\|\\| is always false\\.$#"
+			count: 1
+			path: src/Service/Dumper.php
diff --git a/phpstan.neon.dist b/phpstan.neon.dist
@@ -1,3 +1,6 @@
+includes:
+  - phpstan-baseline.neon
+
 parameters:
   level: max
   paths:

diff --git a/src/Event/SitemapPopulateEvent.php b/src/Event/SitemapPopulateEvent.php
@@ -53,9 +53,41 @@ class SitemapPopulateEvent extends Event
      */
     public function __construct(
         UrlContainerInterface $urlContainer,
-        string $section = null,
-        UrlGeneratorInterface $urlGenerator = null
+        $urlGenerator = null,
+        $section = null
     ) {
+        if (
+            (\is_null($urlGenerator) || \is_string($urlGenerator))
+            && (\is_null($section) || $section instanceof UrlGeneratorInterface)
+        ) {
+            $tmpUrlGenerator = $section;
+            $section = $urlGenerator;
+            $urlGenerator = $tmpUrlGenerator;
+            @\trigger_error(
+                \sprintf(
+                    '%s will change in 4.0, the argument #2 will be %s $urlGenerator.',
+                    __METHOD__,
+                    UrlGeneratorInterface::class
+                ),
+                \E_USER_DEPRECATED
+            );
+        }
+        if (!\is_null($urlGenerator) && !$urlGenerator instanceof UrlGeneratorInterface) {
+            throw new \TypeError(\sprintf(
+                '%s(): Argument #2 ($urlGenerator) must be of type %s, %s given.',
+                __METHOD__,
+                UrlGeneratorInterface::class,
+                \is_object($urlGenerator) ? \get_class($urlGenerator) : \gettype($urlGenerator)
+            ));
+        }
+        if (!\is_null($section) && !\is_string($section)) {
+            throw new \TypeError(\sprintf(
+                '%s(): Argument #3 ($itemsBySet) must be of type ?string, %s given.',
+                __METHOD__,
+                \is_object($section) ? \get_class($section) : \gettype($section)
+            ));
+        }
+
         $this->urlContainer = $urlContainer;
         $this->section = $section;
         $this->urlGenerator = $urlGenerator;

diff --git a/src/Service/AbstractGenerator.php b/src/Service/AbstractGenerator.php
@@ -64,15 +64,47 @@ abstract class AbstractGenerator implements UrlContainerInterface
 
     /**
      * @param EventDispatcherInterface   $dispatcher
-     * @param int|null                   $itemsBySet
      * @param UrlGeneratorInterface|null $urlGenerator
+     * @param int|null                   $itemsBySet
      */
     public function __construct(
         EventDispatcherInterface $dispatcher,
-        int $itemsBySet = null,
-        UrlGeneratorInterface $urlGenerator = null
+        $urlGenerator = null,
+        $itemsBySet = null
     ) {
-        if (!$urlGenerator) {
+        if (
+            (\is_null($urlGenerator) || \is_int($urlGenerator))
+            && (\is_null($itemsBySet) || $itemsBySet instanceof UrlGeneratorInterface)
+        ) {
+            $tmpUrlGenerator = $itemsBySet;
+            $itemsBySet = $urlGenerator;
+            $urlGenerator = $tmpUrlGenerator;
+            @\trigger_error(
+                \sprintf(
+                    '%s will change in 4.0, the argument #2 will be %s $urlGenerator.',
+                    __METHOD__,
+                    UrlGeneratorInterface::class
+                ),
+                \E_USER_DEPRECATED
+            );
+        }
+        if (!\is_null($urlGenerator) && !$urlGenerator instanceof UrlGeneratorInterface) {
+            throw new \TypeError(\sprintf(
+                '%s(): Argument #2 ($urlGenerator) must be of type %s, %s given.',
+                __METHOD__,
+                UrlGeneratorInterface::class,
+                \is_object($urlGenerator) ? \get_class($urlGenerator) : \gettype($urlGenerator)
+            ));
+        }
+        if (!\is_null($itemsBySet) && !\is_int($itemsBySet)) {
+            throw new \TypeError(\sprintf(
+                '%s(): Argument #3 ($itemsBySet) must be of type ?int, %s given.',
+                __METHOD__,
+                \is_object($itemsBySet) ? \get_class($itemsBySet) : \gettype($itemsBySet)
+            ));
+        }
+
+        if ($urlGenerator === null) {
             @trigger_error(
                 'Not injecting the $urlGenerator is deprecated and will be required in 4.0.',
                 \E_USER_DEPRECATED
@@ -167,7 +199,7 @@ abstract protected function newUrlset(string $name, \DateTimeInterface $lastmod
      */
     protected function populate(string $section = null): void
     {
-        $event = new SitemapPopulateEvent($this, $section, $this->urlGenerator);
+        $event = new SitemapPopulateEvent($this, $this->urlGenerator, $section);
 
         $this->dispatcher->dispatch($event, SitemapPopulateEvent::ON_SITEMAP_POPULATE);
     }

diff --git a/src/Service/Dumper.php b/src/Service/Dumper.php
@@ -49,18 +49,59 @@ class Dumper extends AbstractGenerator implements DumperInterface
     /**
      * @param EventDispatcherInterface   $dispatcher Symfony's EventDispatcher
      * @param Filesystem                 $filesystem Symfony's Filesystem
+     * @param UrlGeneratorInterface|null $urlGenerator
      * @param string                     $sitemapFilePrefix
      * @param int|null                   $itemsBySet
-     * @param UrlGeneratorInterface|null $urlGenerator
      */
     public function __construct(
         EventDispatcherInterface $dispatcher,
         Filesystem $filesystem,
-        string $sitemapFilePrefix = Configuration::DEFAULT_FILENAME,
-        int $itemsBySet = null,
-        UrlGeneratorInterface $urlGenerator = null
+        $urlGenerator = null,
+        $sitemapFilePrefix = Configuration::DEFAULT_FILENAME,
+        $itemsBySet = null
     ) {
-        parent::__construct($dispatcher, $itemsBySet, $urlGenerator);
+        if (
+            \is_string($urlGenerator)
+            && (\is_null($sitemapFilePrefix) || \is_int($sitemapFilePrefix))
+            && (\is_null($itemsBySet) || $itemsBySet instanceof UrlGeneratorInterface)
+        ) {
+            $tmpUrlGenerator = $itemsBySet;
+            $itemsBySet = $sitemapFilePrefix;
+            $sitemapFilePrefix = $urlGenerator;
+            $urlGenerator = $tmpUrlGenerator;
+            @\trigger_error(
+                \sprintf(
+                    '%s will change in 4.0, the argument #3 will be %s $urlGenerator.',
+                    __METHOD__,
+                    UrlGeneratorInterface::class
+                ),
+                \E_USER_DEPRECATED
+            );
+        }
+        if (!\is_null($urlGenerator) && !$urlGenerator instanceof UrlGeneratorInterface) {
+            throw new \TypeError(\sprintf(
+                '%s(): Argument #3 ($urlGenerator) must be of type %s, %s given.',
+                __METHOD__,
+                UrlGeneratorInterface::class,
+                \is_object($urlGenerator) ? \get_class($urlGenerator) : \gettype($urlGenerator)
+            ));
+        }
+        if (!\is_string($sitemapFilePrefix)) {
+            throw new \TypeError(\sprintf(
+                '%s(): Argument #4 ($sitemapFilePrefix) must be of type string, %s given.',
+                __METHOD__,
+                \is_object($sitemapFilePrefix) ? \get_class($sitemapFilePrefix) : \gettype($sitemapFilePrefix)
+            ));
+        }
+        if (!\is_null($itemsBySet) && !\is_int($itemsBySet)) {
+            throw new \TypeError(\sprintf(
+                '%s(): Argument #5 ($itemsBySet) must be of type ?int, %s given.',
+                __METHOD__,
+                \is_object($itemsBySet) ? \get_class($itemsBySet) : \gettype($itemsBySet)
+            ));
+        }
+
+        parent::__construct($dispatcher, $urlGenerator, $itemsBySet);
 
         $this->filesystem = $filesystem;
         $this->sitemapFilePrefix = $sitemapFilePrefix;

diff --git a/src/Service/Generator.php b/src/Service/Generator.php
@@ -36,7 +36,7 @@ public function __construct(
         UrlGeneratorInterface $router,
         int $itemsBySet = null
     ) {
-        parent::__construct($dispatcher, $itemsBySet, $router);
+        parent::__construct($dispatcher, $router, $itemsBySet);
 
         $this->router = $router;
     }

diff --git a/tests/Unit/EventListener/RouteAnnotationEventListenerTest.php b/tests/Unit/EventListener/RouteAnnotationEventListenerTest.php
@@ -133,7 +133,7 @@ static function () use ($routes): RouteCollection {
 
         $urlContainer = new InMemoryUrlContainer();
         $dispatcher->addSubscriber(new RouteAnnotationEventListener($router, $dispatcher, 'default'));
-        $event = new SitemapPopulateEvent($urlContainer, $section, $router);
+        $event = new SitemapPopulateEvent($urlContainer, $router, $section);
         $dispatcher->dispatch($event, SitemapPopulateEvent::class);
 
         return $urlContainer;

diff --git a/tests/Unit/Service/DumperTest.php b/tests/Unit/Service/DumperTest.php
@@ -54,7 +54,7 @@ public function setUp(): void
         $this->eventDispatcher = new EventDispatcher();
         $this->filesystem = new Filesystem();
         $this->router = new Router(new ClosureLoader(), null);
-        $this->dumper = new Dumper($this->eventDispatcher, $this->filesystem, 'sitemap', 5, $this->router);
+        $this->dumper = new Dumper($this->eventDispatcher, $this->filesystem, $this->router, 'sitemap', 5);
 
         (new Filesystem())->remove(\glob(sys_get_temp_dir() . '/PrestaSitemaps-*'));
     }