From 16bd368a58ec10bfda81771849385c19ea7ec563 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 6 Aug 2022 09:30:13 +0200 Subject: [PATCH 1/2] Centralized how routes are configured to support multi-segment slugs --- composer.json | 2 +- config/autoload/routes.config.php | 14 ++--- config/config.php | 1 + .../src/Config/MultiSegmentSlugProcessor.php | 30 ++++++++++ .../src/Repository/ShortUrlRepository.php | 8 +-- .../Core/src/Repository/VisitRepository.php | 8 +-- module/Core/src/Spec/InDateRange.php | 8 +-- .../Config/MultiSegmentSlugProcessorTest.php | 60 +++++++++++++++++++ module/Rest/src/ConfigProvider.php | 9 +-- module/Rest/test/ConfigProviderTest.php | 19 +----- 10 files changed, 113 insertions(+), 46 deletions(-) create mode 100644 module/Core/src/Config/MultiSegmentSlugProcessor.php create mode 100644 module/Core/test/Config/MultiSegmentSlugProcessorTest.php diff --git a/composer.json b/composer.json index c6e293bd..a2e6c96c 100644 --- a/composer.json +++ b/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", diff --git a/config/autoload/routes.config.php b/config/autoload/routes.config.php index e7e24916..298b9349 100644 --- a/config/autoload/routes.config.php +++ b/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, diff --git a/config/config.php b/config/config.php index a1e0428a..201c1a17 100644 --- a/config/config.php +++ b/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(); diff --git a/module/Core/src/Config/MultiSegmentSlugProcessor.php b/module/Core/src/Config/MultiSegmentSlugProcessor.php new file mode 100644 index 00000000..b9cf2457 --- /dev/null +++ b/module/Core/src/Config/MultiSegmentSlugProcessor.php @@ -0,0 +1,30 @@ + $path] = $route; + $route['path'] = str_replace(self::SINGLE_SHORT_CODE_PATTERN, self::MULTI_SHORT_CODE_PATTERN, $path); + return $route; + }); + + return $config; + } +} diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index 5946f255..af4f662d 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/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(); diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 3e33d60a..f24153fc 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -245,11 +245,11 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo { $conn = $this->getEntityManager()->getConnection(); - if ($dateRange?->startDate() !== null) { - $qb->andWhere($qb->expr()->gte('v.date', $conn->quote($dateRange->startDate()->toDateTimeString()))); + if ($dateRange?->startDate !== null) { + $qb->andWhere($qb->expr()->gte('v.date', $conn->quote($dateRange->startDate->toDateTimeString()))); } - if ($dateRange?->endDate() !== null) { - $qb->andWhere($qb->expr()->lte('v.date', $conn->quote($dateRange->endDate()->toDateTimeString()))); + if ($dateRange?->endDate !== null) { + $qb->andWhere($qb->expr()->lte('v.date', $conn->quote($dateRange->endDate->toDateTimeString()))); } } diff --git a/module/Core/src/Spec/InDateRange.php b/module/Core/src/Spec/InDateRange.php index 05cb6f0a..994e6d63 100644 --- a/module/Core/src/Spec/InDateRange.php +++ b/module/Core/src/Spec/InDateRange.php @@ -20,12 +20,12 @@ class InDateRange extends BaseSpecification { $criteria = []; - if ($this->dateRange?->startDate() !== null) { - $criteria[] = Spec::gte($this->field, $this->dateRange->startDate()->toDateTimeString()); + if ($this->dateRange?->startDate !== null) { + $criteria[] = Spec::gte($this->field, $this->dateRange->startDate->toDateTimeString()); } - if ($this->dateRange?->endDate() !== null) { - $criteria[] = Spec::lte($this->field, $this->dateRange->endDate()->toDateTimeString()); + if ($this->dateRange?->endDate !== null) { + $criteria[] = Spec::lte($this->field, $this->dateRange->endDate->toDateTimeString()); } return Spec::andX(...$criteria); diff --git a/module/Core/test/Config/MultiSegmentSlugProcessorTest.php b/module/Core/test/Config/MultiSegmentSlugProcessorTest.php new file mode 100644 index 00000000..630a5d90 --- /dev/null +++ b/module/Core/test/Config/MultiSegmentSlugProcessorTest.php @@ -0,0 +1,60 @@ +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'], + ], + ]; + } +} diff --git a/module/Rest/src/ConfigProvider.php b/module/Rest/src/ConfigProvider.php index 3304ce4d..6d389038 100644 --- a/module/Rest/src/ConfigProvider.php +++ b/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; }); diff --git a/module/Rest/test/ConfigProviderTest.php b/module/Rest/test/ConfigProviderTest.php index 07fa4e43..a3f7d0c9 100644 --- a/module/Rest/test/ConfigProviderTest.php +++ b/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'], - ], - ]; } } From 334aee64ad01782625c7b8f5bb7bbf740630e859 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 6 Aug 2022 09:37:15 +0200 Subject: [PATCH 2/2] Updated changelog --- CHANGELOG.md | 17 +++++++++++++++++ config/autoload/url-shortener.local.php.dist | 1 + 2 files changed, 18 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c11faf60..d32bb1b9 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/config/autoload/url-shortener.local.php.dist b/config/autoload/url-shortener.local.php.dist index 20140a9b..0069ffa9 100644 --- a/config/autoload/url-shortener.local.php.dist +++ b/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, ], ];