From c42fb67efcc1ed9a911a9e5fe01b35a1fa8e04d6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 8 Nov 2025 22:47:24 +0100 Subject: [PATCH] Simplify NotFoundRedirectConfigInterface with property hooks and asymetric visibility --- .../Command/Domain/DomainRedirectsCommand.php | 6 +-- .../src/Command/Domain/ListDomainsCommand.php | 6 +-- .../Command/Domain/ListDomainsCommandTest.php | 4 +- .../Config/EmptyNotFoundRedirectConfig.php | 32 ++------------ .../NotFoundRedirectConfigInterface.php | 14 ++----- .../src/Config/NotFoundRedirectResolver.php | 7 ++-- module/Core/src/Config/NotFoundRedirects.php | 10 ++--- .../Options/NotFoundRedirectOptions.php | 42 +++---------------- module/Core/src/Domain/Entity/Domain.php | 36 ++-------------- .../EmptyNotFoundRedirectConfigTest.php | 9 ++-- .../Config/NotFoundRedirectResolverTest.php | 18 ++++---- module/Core/test/Domain/DomainServiceTest.php | 6 +-- .../Domain/Request/DomainRedirectsRequest.php | 6 +-- .../Domain/DomainRedirectsActionTest.php | 6 +-- .../Request/DomainRedirectsRequestTest.php | 4 +- 15 files changed, 54 insertions(+), 152 deletions(-) diff --git a/module/CLI/src/Command/Domain/DomainRedirectsCommand.php b/module/CLI/src/Command/Domain/DomainRedirectsCommand.php index 1e272c12..fc94ee49 100644 --- a/module/CLI/src/Command/Domain/DomainRedirectsCommand.php +++ b/module/CLI/src/Command/Domain/DomainRedirectsCommand.php @@ -88,15 +88,15 @@ class DomainRedirectsCommand extends Command $this->domainService->configureNotFoundRedirects($domainAuthority, NotFoundRedirects::withRedirects( $ask( 'URL to redirect to when a user hits this domain\'s base URL', - $domain?->baseUrlRedirect(), + $domain?->baseUrlRedirect, ), $ask( 'URL to redirect to when a user hits a not found URL other than an invalid short URL', - $domain?->regular404Redirect(), + $domain?->regular404Redirect, ), $ask( 'URL to redirect to when a user hits an invalid short URL', - $domain?->invalidShortUrlRedirect(), + $domain?->invalidShortUrlRedirect, ), )); diff --git a/module/CLI/src/Command/Domain/ListDomainsCommand.php b/module/CLI/src/Command/Domain/ListDomainsCommand.php index a66d6d7e..d33d1ed6 100644 --- a/module/CLI/src/Command/Domain/ListDomainsCommand.php +++ b/module/CLI/src/Command/Domain/ListDomainsCommand.php @@ -59,9 +59,9 @@ class ListDomainsCommand extends Command private function notFoundRedirectsToString(NotFoundRedirectConfigInterface $config): string { - $baseUrl = $config->baseUrlRedirect() ?? 'N/A'; - $regular404 = $config->regular404Redirect() ?? 'N/A'; - $invalidShortUrl = $config->invalidShortUrlRedirect() ?? 'N/A'; + $baseUrl = $config->baseUrlRedirect ?? 'N/A'; + $regular404 = $config->regular404Redirect ?? 'N/A'; + $invalidShortUrl = $config->invalidShortUrlRedirect ?? 'N/A'; return <<domainService->expects($this->once())->method('listDomains')->with()->willReturn([ DomainItem::forDefaultDomain('foo.com', new NotFoundRedirectOptions( - invalidShortUrl: 'https://foo.com/default/invalid', - baseUrl: 'https://foo.com/default/base', + invalidShortUrlRedirect: 'https://foo.com/default/invalid', + baseUrlRedirect: 'https://foo.com/default/base', )), DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com')), DomainItem::forNonDefaultDomain($bazDomain), diff --git a/module/Core/src/Config/EmptyNotFoundRedirectConfig.php b/module/Core/src/Config/EmptyNotFoundRedirectConfig.php index 5ec23bc1..48950829 100644 --- a/module/Core/src/Config/EmptyNotFoundRedirectConfig.php +++ b/module/Core/src/Config/EmptyNotFoundRedirectConfig.php @@ -6,33 +6,7 @@ namespace Shlinkio\Shlink\Core\Config; final class EmptyNotFoundRedirectConfig implements NotFoundRedirectConfigInterface { - public function invalidShortUrlRedirect(): string|null - { - return null; - } - - public function hasInvalidShortUrlRedirect(): bool - { - return false; - } - - public function regular404Redirect(): string|null - { - return null; - } - - public function hasRegular404Redirect(): bool - { - return false; - } - - public function baseUrlRedirect(): string|null - { - return null; - } - - public function hasBaseUrlRedirect(): bool - { - return false; - } + private(set) string|null $invalidShortUrlRedirect = null; + private(set) string|null $regular404Redirect = null; + private(set) string|null $baseUrlRedirect = null; } diff --git a/module/Core/src/Config/NotFoundRedirectConfigInterface.php b/module/Core/src/Config/NotFoundRedirectConfigInterface.php index 46c2c734..40b26f58 100644 --- a/module/Core/src/Config/NotFoundRedirectConfigInterface.php +++ b/module/Core/src/Config/NotFoundRedirectConfigInterface.php @@ -6,15 +6,7 @@ namespace Shlinkio\Shlink\Core\Config; interface NotFoundRedirectConfigInterface { - public function invalidShortUrlRedirect(): string|null; - - public function hasInvalidShortUrlRedirect(): bool; - - public function regular404Redirect(): string|null; - - public function hasRegular404Redirect(): bool; - - public function baseUrlRedirect(): string|null; - - public function hasBaseUrlRedirect(): bool; + public string|null $invalidShortUrlRedirect { get; } + public string|null $regular404Redirect { get; } + public string|null $baseUrlRedirect { get; } } diff --git a/module/Core/src/Config/NotFoundRedirectResolver.php b/module/Core/src/Config/NotFoundRedirectResolver.php index dbdf8151..d32ad232 100644 --- a/module/Core/src/Config/NotFoundRedirectResolver.php +++ b/module/Core/src/Config/NotFoundRedirectResolver.php @@ -32,10 +32,9 @@ class NotFoundRedirectResolver implements NotFoundRedirectResolverInterface UriInterface $currentUri, ): ResponseInterface|null { $urlToRedirectTo = match (true) { - $notFoundType->isBaseUrl() && $config->hasBaseUrlRedirect() => $config->baseUrlRedirect(), - $notFoundType->isRegularNotFound() && $config->hasRegular404Redirect() => $config->regular404Redirect(), - $notFoundType->isInvalidShortUrl() && $config->hasInvalidShortUrlRedirect() => - $config->invalidShortUrlRedirect(), + $notFoundType->isBaseUrl() => $config->baseUrlRedirect, + $notFoundType->isRegularNotFound() => $config->regular404Redirect, + $notFoundType->isInvalidShortUrl() => $config->invalidShortUrlRedirect, default => null, }; diff --git a/module/Core/src/Config/NotFoundRedirects.php b/module/Core/src/Config/NotFoundRedirects.php index 2753d44f..c202edd3 100644 --- a/module/Core/src/Config/NotFoundRedirects.php +++ b/module/Core/src/Config/NotFoundRedirects.php @@ -6,12 +6,12 @@ namespace Shlinkio\Shlink\Core\Config; use JsonSerializable; -final class NotFoundRedirects implements JsonSerializable +final readonly class NotFoundRedirects implements JsonSerializable { private function __construct( - public readonly string|null $baseUrlRedirect, - public readonly string|null $regular404Redirect, - public readonly string|null $invalidShortUrlRedirect, + public string|null $baseUrlRedirect, + public string|null $regular404Redirect, + public string|null $invalidShortUrlRedirect, ) { } @@ -30,7 +30,7 @@ final class NotFoundRedirects implements JsonSerializable public static function fromConfig(NotFoundRedirectConfigInterface $config): self { - return new self($config->baseUrlRedirect(), $config->regular404Redirect(), $config->invalidShortUrlRedirect()); + return new self($config->baseUrlRedirect, $config->regular404Redirect, $config->invalidShortUrlRedirect); } public function jsonSerialize(): array diff --git a/module/Core/src/Config/Options/NotFoundRedirectOptions.php b/module/Core/src/Config/Options/NotFoundRedirectOptions.php index 7c04d077..07dbbe44 100644 --- a/module/Core/src/Config/Options/NotFoundRedirectOptions.php +++ b/module/Core/src/Config/Options/NotFoundRedirectOptions.php @@ -10,48 +10,18 @@ use Shlinkio\Shlink\Core\Config\NotFoundRedirectConfigInterface; final readonly class NotFoundRedirectOptions implements NotFoundRedirectConfigInterface { public function __construct( - public string|null $invalidShortUrl = null, - public string|null $regular404 = null, - public string|null $baseUrl = null, + public string|null $invalidShortUrlRedirect = null, + public string|null $regular404Redirect = null, + public string|null $baseUrlRedirect = null, ) { } public static function fromEnv(): self { return new self( - invalidShortUrl: EnvVars::DEFAULT_INVALID_SHORT_URL_REDIRECT->loadFromEnv(), - regular404: EnvVars::DEFAULT_REGULAR_404_REDIRECT->loadFromEnv(), - baseUrl: EnvVars::DEFAULT_BASE_URL_REDIRECT->loadFromEnv(), + invalidShortUrlRedirect: EnvVars::DEFAULT_INVALID_SHORT_URL_REDIRECT->loadFromEnv(), + regular404Redirect: EnvVars::DEFAULT_REGULAR_404_REDIRECT->loadFromEnv(), + baseUrlRedirect: EnvVars::DEFAULT_BASE_URL_REDIRECT->loadFromEnv(), ); } - - public function invalidShortUrlRedirect(): string|null - { - return $this->invalidShortUrl; - } - - public function hasInvalidShortUrlRedirect(): bool - { - return $this->invalidShortUrl !== null; - } - - public function regular404Redirect(): string|null - { - return $this->regular404; - } - - public function hasRegular404Redirect(): bool - { - return $this->regular404 !== null; - } - - public function baseUrlRedirect(): string|null - { - return $this->baseUrl; - } - - public function hasBaseUrlRedirect(): bool - { - return $this->baseUrl !== null; - } } diff --git a/module/Core/src/Domain/Entity/Domain.php b/module/Core/src/Domain/Entity/Domain.php index b55a9dee..835620bf 100644 --- a/module/Core/src/Domain/Entity/Domain.php +++ b/module/Core/src/Domain/Entity/Domain.php @@ -15,9 +15,9 @@ class Domain extends AbstractEntity implements JsonSerializable, NotFoundRedirec private function __construct( public readonly string $authority, - private string|null $baseUrlRedirect = null, - private string|null $regular404Redirect = null, - private string|null $invalidShortUrlRedirect = null, + private(set) string|null $baseUrlRedirect = null, + private(set) string|null $regular404Redirect = null, + private(set) string|null $invalidShortUrlRedirect = null, ) { } @@ -31,36 +31,6 @@ class Domain extends AbstractEntity implements JsonSerializable, NotFoundRedirec return $this->authority; } - public function invalidShortUrlRedirect(): string|null - { - return $this->invalidShortUrlRedirect; - } - - public function hasInvalidShortUrlRedirect(): bool - { - return $this->invalidShortUrlRedirect !== null; - } - - public function regular404Redirect(): string|null - { - return $this->regular404Redirect; - } - - public function hasRegular404Redirect(): bool - { - return $this->regular404Redirect !== null; - } - - public function baseUrlRedirect(): string|null - { - return $this->baseUrlRedirect; - } - - public function hasBaseUrlRedirect(): bool - { - return $this->baseUrlRedirect !== null; - } - public function configureNotFoundRedirects(NotFoundRedirects $redirects): void { $this->baseUrlRedirect = $redirects->baseUrlRedirect; diff --git a/module/Core/test/Config/EmptyNotFoundRedirectConfigTest.php b/module/Core/test/Config/EmptyNotFoundRedirectConfigTest.php index 52190b4e..76a5d500 100644 --- a/module/Core/test/Config/EmptyNotFoundRedirectConfigTest.php +++ b/module/Core/test/Config/EmptyNotFoundRedirectConfigTest.php @@ -20,11 +20,8 @@ class EmptyNotFoundRedirectConfigTest extends TestCase #[Test] public function allMethodsReturnHardcodedValues(): void { - self::assertNull($this->redirectsConfig->invalidShortUrlRedirect()); - self::assertFalse($this->redirectsConfig->hasInvalidShortUrlRedirect()); - self::assertNull($this->redirectsConfig->regular404Redirect()); - self::assertFalse($this->redirectsConfig->hasRegular404Redirect()); - self::assertNull($this->redirectsConfig->baseUrlRedirect()); - self::assertFalse($this->redirectsConfig->hasBaseUrlRedirect()); + self::assertNull($this->redirectsConfig->invalidShortUrlRedirect); + self::assertNull($this->redirectsConfig->regular404Redirect); + self::assertNull($this->redirectsConfig->baseUrlRedirect); } } diff --git a/module/Core/test/Config/NotFoundRedirectResolverTest.php b/module/Core/test/Config/NotFoundRedirectResolverTest.php index 75df0948..d2e750db 100644 --- a/module/Core/test/Config/NotFoundRedirectResolverTest.php +++ b/module/Core/test/Config/NotFoundRedirectResolverTest.php @@ -57,57 +57,57 @@ class NotFoundRedirectResolverTest extends TestCase yield 'base URL with trailing slash' => [ $uri = new Uri('/'), self::notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), - new NotFoundRedirectOptions(baseUrl: 'https://example.com/baseUrl'), + new NotFoundRedirectOptions(baseUrlRedirect: 'https://example.com/baseUrl'), 'https://example.com/baseUrl', ]; yield 'base URL without trailing slash' => [ $uri = new Uri(''), self::notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), - new NotFoundRedirectOptions(baseUrl: 'https://example.com/baseUrl'), + new NotFoundRedirectOptions(baseUrlRedirect: 'https://example.com/baseUrl'), 'https://example.com/baseUrl', ]; yield 'base URL with domain placeholder' => [ $uri = new Uri('https://s.test'), self::notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), - new NotFoundRedirectOptions(baseUrl: 'https://redirect-here.com/{DOMAIN}'), + new NotFoundRedirectOptions(baseUrlRedirect: 'https://redirect-here.com/{DOMAIN}'), 'https://redirect-here.com/s.test', ]; yield 'base URL with domain placeholder in query' => [ $uri = new Uri('https://s.test'), self::notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), - new NotFoundRedirectOptions(baseUrl: 'https://redirect-here.com/?domain={DOMAIN}'), + new NotFoundRedirectOptions(baseUrlRedirect: 'https://redirect-here.com/?domain={DOMAIN}'), 'https://redirect-here.com/?domain=s.test', ]; yield 'regular 404' => [ $uri = new Uri('/foo/bar'), self::notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), - new NotFoundRedirectOptions(regular404: 'https://example.com/regular404'), + new NotFoundRedirectOptions(regular404Redirect: 'https://example.com/regular404'), 'https://example.com/regular404', ]; yield 'regular 404 with path placeholder in query' => [ $uri = new Uri('/foo/bar'), self::notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), - new NotFoundRedirectOptions(regular404: 'https://redirect-here.com/?path={ORIGINAL_PATH}'), + new NotFoundRedirectOptions(regular404Redirect: 'https://redirect-here.com/?path={ORIGINAL_PATH}'), 'https://redirect-here.com/?path=%2Ffoo%2Fbar', ]; yield 'regular 404 with multiple placeholders' => [ $uri = new Uri('https://s.test/foo/bar'), self::notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), new NotFoundRedirectOptions( - regular404: 'https://redirect-here.com/{ORIGINAL_PATH}/{DOMAIN}/?d={DOMAIN}&p={ORIGINAL_PATH}', + regular404Redirect: 'https://redirect-here.com/{ORIGINAL_PATH}/{DOMAIN}/?d={DOMAIN}&p={ORIGINAL_PATH}', ), 'https://redirect-here.com/foo/bar/s.test/?d=s.test&p=%2Ffoo%2Fbar', ]; yield 'invalid short URL' => [ new Uri('/foo'), self::notFoundType(self::requestForRoute(RedirectAction::class)), - new NotFoundRedirectOptions(invalidShortUrl: 'https://example.com/invalidShortUrl'), + new NotFoundRedirectOptions(invalidShortUrlRedirect: 'https://example.com/invalidShortUrl'), 'https://example.com/invalidShortUrl', ]; yield 'invalid short URL with path placeholder' => [ new Uri('/foo'), self::notFoundType(self::requestForRoute(RedirectAction::class)), - new NotFoundRedirectOptions(invalidShortUrl: 'https://redirect-here.com/{ORIGINAL_PATH}'), + new NotFoundRedirectOptions(invalidShortUrlRedirect: 'https://redirect-here.com/{ORIGINAL_PATH}'), 'https://redirect-here.com/foo', ]; } diff --git a/module/Core/test/Domain/DomainServiceTest.php b/module/Core/test/Domain/DomainServiceTest.php index fb601d51..f4836711 100644 --- a/module/Core/test/Domain/DomainServiceTest.php +++ b/module/Core/test/Domain/DomainServiceTest.php @@ -181,9 +181,9 @@ class DomainServiceTest extends TestCase if ($foundDomain !== null) { self::assertSame($result, $foundDomain); } - self::assertEquals('foo.com', $result->baseUrlRedirect()); - self::assertEquals('bar.com', $result->regular404Redirect()); - self::assertEquals('baz.com', $result->invalidShortUrlRedirect()); + self::assertEquals('foo.com', $result->baseUrlRedirect); + self::assertEquals('bar.com', $result->regular404Redirect); + self::assertEquals('baz.com', $result->invalidShortUrlRedirect); } public static function provideFoundDomains(): iterable diff --git a/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php b/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php index 0c55f967..b203df31 100644 --- a/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php +++ b/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php @@ -69,11 +69,11 @@ class DomainRedirectsRequest public function toNotFoundRedirects(NotFoundRedirectConfigInterface|null $defaults = null): NotFoundRedirects { return NotFoundRedirects::withRedirects( - $this->baseUrlRedirectWasProvided ? $this->baseUrlRedirect : $defaults?->baseUrlRedirect(), - $this->regular404RedirectWasProvided ? $this->regular404Redirect : $defaults?->regular404Redirect(), + $this->baseUrlRedirectWasProvided ? $this->baseUrlRedirect : $defaults?->baseUrlRedirect, + $this->regular404RedirectWasProvided ? $this->regular404Redirect : $defaults?->regular404Redirect, $this->invalidShortUrlRedirectWasProvided ? $this->invalidShortUrlRedirect - : $defaults?->invalidShortUrlRedirect(), + : $defaults?->invalidShortUrlRedirect, ); } } diff --git a/module/Rest/test/Action/Domain/DomainRedirectsActionTest.php b/module/Rest/test/Action/Domain/DomainRedirectsActionTest.php index e203fe11..8c3fab9f 100644 --- a/module/Rest/test/Action/Domain/DomainRedirectsActionTest.php +++ b/module/Rest/test/Action/Domain/DomainRedirectsActionTest.php @@ -68,13 +68,13 @@ class DomainRedirectsActionTest extends TestCase NotFoundRedirects::withRedirects( array_key_exists(DomainRedirectsInputFilter::BASE_URL_REDIRECT, $redirects) ? $redirects[DomainRedirectsInputFilter::BASE_URL_REDIRECT] - : $domain->baseUrlRedirect(), + : $domain->baseUrlRedirect, array_key_exists(DomainRedirectsInputFilter::REGULAR_404_REDIRECT, $redirects) ? $redirects[DomainRedirectsInputFilter::REGULAR_404_REDIRECT] - : $domain->regular404Redirect(), + : $domain->regular404Redirect, array_key_exists(DomainRedirectsInputFilter::INVALID_SHORT_URL_REDIRECT, $redirects) ? $redirects[DomainRedirectsInputFilter::INVALID_SHORT_URL_REDIRECT] - : $domain->invalidShortUrlRedirect(), + : $domain->invalidShortUrlRedirect, ), $apiKey, ); diff --git a/module/Rest/test/Action/Domain/Request/DomainRedirectsRequestTest.php b/module/Rest/test/Action/Domain/Request/DomainRedirectsRequestTest.php index 45faf9f2..80983064 100644 --- a/module/Rest/test/Action/Domain/Request/DomainRedirectsRequestTest.php +++ b/module/Rest/test/Action/Domain/Request/DomainRedirectsRequestTest.php @@ -51,7 +51,7 @@ class DomainRedirectsRequestTest extends TestCase yield 'some values' => [['domain' => 'foo', 'regular404Redirect' => 'bar'], null, 'foo', null, 'bar', null]; yield 'fallbacks' => [ ['domain' => 'domain', 'baseUrlRedirect' => 'bar'], - new NotFoundRedirectOptions(invalidShortUrl: 'fallback2', regular404: 'fallback'), + new NotFoundRedirectOptions(invalidShortUrlRedirect: 'fallback2', regular404Redirect: 'fallback'), 'domain', 'bar', 'fallback', @@ -59,7 +59,7 @@ class DomainRedirectsRequestTest extends TestCase ]; yield 'fallback ignored' => [ ['domain' => 'domain', 'regular404Redirect' => 'bar', 'invalidShortUrlRedirect' => null], - new NotFoundRedirectOptions(invalidShortUrl: 'fallback2', regular404: 'fallback'), + new NotFoundRedirectOptions(invalidShortUrlRedirect: 'fallback2', regular404Redirect: 'fallback'), 'domain', null, 'bar',