Merge pull request #1496 from acelaya-forks/feature/centralize-multi-segment

Feature/centralize multi segment

CHANGELOG.md

@@ -4,6 +4,23 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org).
 
+## [Unreleased]
+### Added
+* *Nothing*
+
+### Changed
+* [#1495](https://github.com/shlinkio/shlink/issues/1495) Centralized how routes are configured to support multi-segment slugs.
+
+### Deprecated
+* *Nothing*
+
+### Removed
+* *Nothing*
+
+### Fixed
+* *Nothing*
+
+
 ## [3.2.0] - 2022-08-05
 ### Added
 * [#854](https://github.com/shlinkio/shlink/issues/854) Added support for multi-segment custom slugs.

composer.json

@@ -43,7 +43,7 @@
         "php-middleware/request-id": "^4.1",
         "pugx/shortid-php": "^1.0",
         "ramsey/uuid": "^4.3",
-        "shlinkio/shlink-common": "^4.5",
+        "shlinkio/shlink-common": "dev-main#6163a03 as 5.0",
         "shlinkio/shlink-config": "^1.6",
         "shlinkio/shlink-event-dispatcher": "^2.4",
         "shlinkio/shlink-importer": "^3.0",

config/autoload/routes.config.php

@@ -7,23 +7,19 @@ namespace Shlinkio\Shlink;
 use Fig\Http\Message\RequestMethodInterface;
 use RKA\Middleware\IpAddress;
 use Shlinkio\Shlink\Core\Action as CoreAction;
-use Shlinkio\Shlink\Core\Config\EnvVars;
 use Shlinkio\Shlink\Rest\Action;
 use Shlinkio\Shlink\Rest\ConfigProvider;
 use Shlinkio\Shlink\Rest\Middleware;
 use Shlinkio\Shlink\Rest\Middleware\Mercure\NotConfiguredMercureErrorHandler;
 
-use function sprintf;
-
-// The order of the routes defined here matters. Changing it might cause path conflicts
 return (static function (): array {
     $contentNegotiationMiddleware = Middleware\ShortUrl\CreateShortUrlContentNegotiationMiddleware::class;
     $dropDomainMiddleware = Middleware\ShortUrl\DropDefaultDomainFromRequestMiddleware::class;
     $overrideDomainMiddleware = Middleware\ShortUrl\OverrideDomainMiddleware::class;
-    $multiSegment = (bool) EnvVars::MULTI_SEGMENT_SLUGS_ENABLED->loadFromEnv(false);
 
     return [
 
+        // The order of the routes defined here matters. Changing it might cause path conflicts
         'routes' => [
             // Rest
             ...ConfigProvider::applyRoutesPrefix([
@@ -64,7 +60,7 @@ return (static function (): array {
                 Action\Domain\DomainRedirectsAction::getRouteDef(),
 
                 Action\MercureInfoAction::getRouteDef([NotConfiguredMercureErrorHandler::class]),
-            ], $multiSegment),
+            ]),
 
             // Non-rest
             [
@@ -77,7 +73,7 @@ return (static function (): array {
             ],
             [
                 'name' => CoreAction\PixelAction::class,
-                'path' => sprintf('/{shortCode%s}/track', $multiSegment ? ':.+' : ''),
+                'path' => '/{shortCode}/track',
                 'middleware' => [
                     IpAddress::class,
                     CoreAction\PixelAction::class,
@@ -86,7 +82,7 @@ return (static function (): array {
             ],
             [
                 'name' => CoreAction\QrCodeAction::class,
-                'path' => sprintf('/{shortCode%s}/qr-code', $multiSegment ? ':.+' : ''),
+                'path' => '/{shortCode}/qr-code',
                 'middleware' => [
                     CoreAction\QrCodeAction::class,
                 ],
@@ -94,7 +90,7 @@ return (static function (): array {
             ],
             [
                 'name' => CoreAction\RedirectAction::class,
-                'path' => sprintf('/{shortCode%s}', $multiSegment ? ':.+' : ''),
+                'path' => '/{shortCode}',
                 'middleware' => [
                     IpAddress::class,
                     CoreAction\RedirectAction::class,

config/autoload/url-shortener.local.php.dist

@@ -12,6 +12,7 @@ return [
             'hostname' => sprintf('localhost:%s', $isSwoole ? '8080' : '8000'),
         ],
         'auto_resolve_titles' => true,
+//        'multi_segment_slugs_enabled' => true,
     ],
 
 ];

config/config.php

@@ -47,4 +47,5 @@ return (new ConfigAggregator\ConfigAggregator([
     new ConfigAggregator\PhpFileProvider('config/autoload/routes.config.php'),
 ], 'data/cache/app_config.php', [
     Core\Config\BasePathPrefixer::class,
+    Core\Config\MultiSegmentSlugProcessor::class,
 ]))->getMergedConfig();

module/Core/src/Config/MultiSegmentSlugProcessor.php

@@ -0,0 +1,30 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Config;
+
+use function Functional\map;
+use function str_replace;
+
+class MultiSegmentSlugProcessor
+{
+    private const SINGLE_SHORT_CODE_PATTERN = '{shortCode}';
+    private const MULTI_SHORT_CODE_PATTERN = '{shortCode:.+}';
+
+    public function __invoke(array $config): array
+    {
+        $multiSegmentEnabled = $config['url_shortener']['multi_segment_slugs_enabled'] ?? false;
+        if (! $multiSegmentEnabled) {
+            return $config;
+        }
+
+        $config['routes'] = map($config['routes'] ?? [], static function (array $route): array {
+            ['path' => $path] = $route;
+            $route['path'] = str_replace(self::SINGLE_SHORT_CODE_PATTERN, self::MULTI_SHORT_CODE_PATTERN, $path);
+            return $route;
+        });
+
+        return $config;
+    }
+}

module/Core/src/Repository/ShortUrlRepository.php

@@ -84,13 +84,13 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU
            ->where('1=1');
 
         $dateRange = $filtering->dateRange();
-        if ($dateRange?->startDate() !== null) {
+        if ($dateRange?->startDate !== null) {
             $qb->andWhere($qb->expr()->gte('s.dateCreated', ':startDate'));
-            $qb->setParameter('startDate', $dateRange->startDate(), ChronosDateTimeType::CHRONOS_DATETIME);
+            $qb->setParameter('startDate', $dateRange->startDate, ChronosDateTimeType::CHRONOS_DATETIME);
         }
-        if ($dateRange?->endDate() !== null) {
+        if ($dateRange?->endDate !== null) {
             $qb->andWhere($qb->expr()->lte('s.dateCreated', ':endDate'));
-            $qb->setParameter('endDate', $dateRange->endDate(), ChronosDateTimeType::CHRONOS_DATETIME);
+            $qb->setParameter('endDate', $dateRange->endDate, ChronosDateTimeType::CHRONOS_DATETIME);
         }
 
         $searchTerm = $filtering->searchTerm();

module/Core/src/Repository/VisitRepository.php

@@ -245,11 +245,11 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo
     {
         $conn = $this->getEntityManager()->getConnection();
 
-        if ($dateRange?->startDate() !== null) {
+        if ($dateRange?->startDate !== null) {
-            $qb->andWhere($qb->expr()->gte('v.date', $conn->quote($dateRange->startDate()->toDateTimeString())));
+            $qb->andWhere($qb->expr()->gte('v.date', $conn->quote($dateRange->startDate->toDateTimeString())));
         }
-        if ($dateRange?->endDate() !== null) {
+        if ($dateRange?->endDate !== null) {
-            $qb->andWhere($qb->expr()->lte('v.date', $conn->quote($dateRange->endDate()->toDateTimeString())));
+            $qb->andWhere($qb->expr()->lte('v.date', $conn->quote($dateRange->endDate->toDateTimeString())));
         }
     }
 

module/Core/src/Spec/InDateRange.php

@@ -20,12 +20,12 @@ class InDateRange extends BaseSpecification
     {
         $criteria = [];
 
-        if ($this->dateRange?->startDate() !== null) {
+        if ($this->dateRange?->startDate !== null) {
-            $criteria[] = Spec::gte($this->field, $this->dateRange->startDate()->toDateTimeString());
+            $criteria[] = Spec::gte($this->field, $this->dateRange->startDate->toDateTimeString());
         }
 
-        if ($this->dateRange?->endDate() !== null) {
+        if ($this->dateRange?->endDate !== null) {
-            $criteria[] = Spec::lte($this->field, $this->dateRange->endDate()->toDateTimeString());
+            $criteria[] = Spec::lte($this->field, $this->dateRange->endDate->toDateTimeString());
         }
 
         return Spec::andX(...$criteria);

module/Core/test/Config/MultiSegmentSlugProcessorTest.php

@@ -0,0 +1,60 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\Config;
+
+use PHPUnit\Framework\TestCase;
+use Shlinkio\Shlink\Core\Config\MultiSegmentSlugProcessor;
+
+class MultiSegmentSlugProcessorTest extends TestCase
+{
+    private MultiSegmentSlugProcessor $processor;
+
+    protected function setUp(): void
+    {
+        $this->processor = new MultiSegmentSlugProcessor();
+    }
+
+    /**
+     * @test
+     * @dataProvider provideConfigs
+     */
+    public function parsesRoutesAsExpected(array $config, array $expectedRoutes): void
+    {
+        self::assertEquals($expectedRoutes, ($this->processor)($config)['routes'] ?? []);
+    }
+
+    public function provideConfigs(): iterable
+    {
+        yield [[], []];
+        yield [['url_shortener' => []], []];
+        yield [['url_shortener' => ['multi_segment_slugs_enabled' => false]], []];
+        yield [
+            [
+                'url_shortener' => ['multi_segment_slugs_enabled' => false],
+                'routes' => $routes = [
+                    ['path' => '/foo'],
+                    ['path' => '/bar/{shortCode}'],
+                    ['path' => '/baz/{shortCode}/foo'],
+                ],
+            ],
+            $routes,
+        ];
+        yield [
+            [
+                'url_shortener' => ['multi_segment_slugs_enabled' => true],
+                'routes' => [
+                    ['path' => '/foo'],
+                    ['path' => '/bar/{shortCode}'],
+                    ['path' => '/baz/{shortCode}/foo'],
+                ],
+            ],
+            [
+                ['path' => '/foo'],
+                ['path' => '/bar/{shortCode:.+}'],
+                ['path' => '/baz/{shortCode:.+}/foo'],
+            ],
+        ];
+    }
+}

module/Rest/src/ConfigProvider.php

@@ -8,7 +8,6 @@ use function Functional\first;
 use function Functional\map;
 use function Shlinkio\Shlink\Config\loadConfigFromGlob;
 use function sprintf;
-use function str_replace;
 
 class ConfigProvider
 {
@@ -21,16 +20,12 @@ class ConfigProvider
         return loadConfigFromGlob(__DIR__ . '/../config/{,*.}config.php');
     }
 
-    public static function applyRoutesPrefix(array $routes, bool $multiSegmentEnabled): array
+    public static function applyRoutesPrefix(array $routes): array
     {
         $healthRoute = self::buildUnversionedHealthRouteFromExistingRoutes($routes);
-        $prefixedRoutes = map($routes, static function (array $route) use ($multiSegmentEnabled) {
+        $prefixedRoutes = map($routes, static function (array $route) {
             ['path' => $path] = $route;
-            if ($multiSegmentEnabled) {
-                $path = str_replace('{shortCode}', '{shortCode:.+}', $path);
-            }
             $route['path'] = sprintf('%s%s', self::ROUTES_PREFIX, $path);
-
             return $route;
         });
 

module/Rest/test/ConfigProviderTest.php

@@ -33,9 +33,9 @@ class ConfigProviderTest extends TestCase
      * @test
      * @dataProvider provideRoutesConfig
      */
-    public function routesAreProperlyPrefixed(array $routes, bool $multiSegmentEnabled, array $expected): void
+    public function routesAreProperlyPrefixed(array $routes, array $expected): void
     {
-        self::assertEquals($expected, ConfigProvider::applyRoutesPrefix($routes, $multiSegmentEnabled));
+        self::assertEquals($expected, ConfigProvider::applyRoutesPrefix($routes));
     }
 
     public function provideRoutesConfig(): iterable
@@ -47,7 +47,6 @@ class ConfigProviderTest extends TestCase
                 ['path' => '/baz/foo'],
                 ['path' => '/health'],
             ],
-            false,
             [
                 ['path' => '/rest/v{version:1|2}/foo'],
                 ['path' => '/rest/v{version:1|2}/bar'],
@@ -62,25 +61,11 @@ class ConfigProviderTest extends TestCase
                 ['path' => '/bar'],
                 ['path' => '/baz/foo'],
             ],
-            false,
             [
                 ['path' => '/rest/v{version:1|2}/foo'],
                 ['path' => '/rest/v{version:1|2}/bar'],
                 ['path' => '/rest/v{version:1|2}/baz/foo'],
             ],
         ];
-        yield 'multi-segment enabled' => [
-            [
-                ['path' => '/foo'],
-                ['path' => '/bar/{shortCode}'],
-                ['path' => '/baz/{shortCode}/foo'],
-            ],
-            true,
-            [
-                ['path' => '/rest/v{version:1|2}/foo'],
-                ['path' => '/rest/v{version:1|2}/bar/{shortCode:.+}'],
-                ['path' => '/rest/v{version:1|2}/baz/{shortCode:.+}/foo'],
-            ],
-        ];
     }
 }