Changed behavior of domains list so that it does not return configured redirects as redirects for default domain

CHANGELOG.md

@@ -6,6 +6,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 
 ## [Unreleased]
 ### Added
+* [#1163](https://github.com/shlinkio/shlink/issues/1163) Allowed setting not-found redirects for default domain in the same way it's done for any other domain.
+
+    This implies a few non-breaking changes:
+
+    * The domains list no longer has the values of `INVALID_SHORT_URL_REDIRECT_TO`, `REGULAR_404_REDIRECT_TO` and `BASE_URL_REDIRECT_TO` on the default domain redirects.
+    * The `GET /domains` endpoint includes a new `defaultRedirects` property in the response, with the default redirects set via config or env vars.
+    * The `INVALID_SHORT_URL_REDIRECT_TO`, `REGULAR_404_REDIRECT_TO` and `BASE_URL_REDIRECT_TO` env vars are now deprecated, and should be replaced by `DEFAULT_INVALID_SHORT_URL_REDIRECT`, `DEFAULT_REGULAR_404_REDIRECT` and `DEFAULT_BASE_URL_REDIRECT` respectively. Deprecated ones will continue to work until v3.0.0, where they will be removed.
+
 * [#1204](https://github.com/shlinkio/shlink/issues/1204) Added support for `openswoole` and migrated official docker image to `openswoole`.
 * [#1242](https://github.com/shlinkio/shlink/issues/1242) Added support to import urls and visits from YOURLS.
 

module/CLI/test/Command/Domain/DomainRedirectsCommandTest.php

@@ -126,8 +126,8 @@ class DomainRedirectsCommandTest extends TestCase
 
         $listDomains = $this->domainService->listDomains()->willReturn([
             DomainItem::forDefaultDomain('default-domain.com', new NotFoundRedirectOptions()),
-            DomainItem::forExistingDomain(Domain::withAuthority('existing-one.com')),
+            DomainItem::forNonDefaultDomain(Domain::withAuthority('existing-one.com')),
-            DomainItem::forExistingDomain(Domain::withAuthority($domainAuthority)),
+            DomainItem::forNonDefaultDomain(Domain::withAuthority($domainAuthority)),
         ]);
         $findDomain = $this->domainService->findByAuthority($domainAuthority)->willReturn($domain);
         $configureRedirects = $this->domainService->configureNotFoundRedirects(
@@ -156,8 +156,8 @@ class DomainRedirectsCommandTest extends TestCase
 
         $listDomains = $this->domainService->listDomains()->willReturn([
             DomainItem::forDefaultDomain('default-domain.com', new NotFoundRedirectOptions()),
-            DomainItem::forExistingDomain(Domain::withAuthority('existing-one.com')),
+            DomainItem::forNonDefaultDomain(Domain::withAuthority('existing-one.com')),
-            DomainItem::forExistingDomain(Domain::withAuthority('existing-two.com')),
+            DomainItem::forNonDefaultDomain(Domain::withAuthority('existing-two.com')),
         ]);
         $findDomain = $this->domainService->findByAuthority($domainAuthority)->willReturn($domain);
         $configureRedirects = $this->domainService->configureNotFoundRedirects(

module/CLI/test/Command/Domain/ListDomainsCommandTest.php

@@ -47,8 +47,8 @@ class ListDomainsCommandTest extends TestCase
                 'base_url' => 'https://foo.com/default/base',
                 'invalid_short_url' => 'https://foo.com/default/invalid',
             ])),
-            DomainItem::forExistingDomain(Domain::withAuthority('bar.com')),
+            DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com')),
-            DomainItem::forExistingDomain($bazDomain),
+            DomainItem::forNonDefaultDomain($bazDomain),
         ]);
 
         $this->commandTester->execute($input);

module/Core/config/dependencies.config.php

@@ -119,11 +119,7 @@ return [
         ],
         Service\ShortUrl\ShortUrlResolver::class => ['em'],
         Service\ShortUrl\ShortCodeHelper::class => ['em'],
-        Domain\DomainService::class => [
+        Domain\DomainService::class => ['em', 'config.url_shortener.domain.hostname'],
-            'em',
-            'config.url_shortener.domain.hostname',
-            Options\NotFoundRedirectOptions::class,
-        ],
 
         Util\UrlValidator::class => ['httpClient', Options\UrlShortenerOptions::class],
         Util\DoctrineBatchHelper::class => ['em'],

module/Core/src/Config/EmptyNotFoundRedirectConfig.php

@@ -0,0 +1,38 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Config;
+
+final class EmptyNotFoundRedirectConfig implements NotFoundRedirectConfigInterface
+{
+    public function invalidShortUrlRedirect(): ?string
+    {
+        return null;
+    }
+
+    public function hasInvalidShortUrlRedirect(): bool
+    {
+        return false;
+    }
+
+    public function regular404Redirect(): ?string
+    {
+        return null;
+    }
+
+    public function hasRegular404Redirect(): bool
+    {
+        return false;
+    }
+
+    public function baseUrlRedirect(): ?string
+    {
+        return null;
+    }
+
+    public function hasBaseUrlRedirect(): bool
+    {
+        return false;
+    }
+}

module/Core/src/Domain/DomainService.php

@@ -5,12 +5,12 @@ declare(strict_types=1);
 namespace Shlinkio\Shlink\Core\Domain;
 
 use Doctrine\ORM\EntityManagerInterface;
+use Shlinkio\Shlink\Core\Config\EmptyNotFoundRedirectConfig;
 use Shlinkio\Shlink\Core\Config\NotFoundRedirects;
 use Shlinkio\Shlink\Core\Domain\Model\DomainItem;
 use Shlinkio\Shlink\Core\Domain\Repository\DomainRepositoryInterface;
 use Shlinkio\Shlink\Core\Entity\Domain;
 use Shlinkio\Shlink\Core\Exception\DomainNotFoundException;
-use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions;
 use Shlinkio\Shlink\Rest\ApiKey\Role;
 use Shlinkio\Shlink\Rest\Entity\ApiKey;
 
@@ -20,11 +20,8 @@ use function Functional\map;
 
 class DomainService implements DomainServiceInterface
 {
-    public function __construct(
+    public function __construct(private EntityManagerInterface $em, private string $defaultDomain)
-        private EntityManagerInterface $em,
+    {
-        private string $defaultDomain,
-        private NotFoundRedirectOptions $redirectOptions,
-    ) {
     }
 
     /**
@@ -33,14 +30,14 @@ class DomainService implements DomainServiceInterface
     public function listDomains(?ApiKey $apiKey = null): array
     {
         [$default, $domains] = $this->defaultDomainAndRest($apiKey);
-        $mappedDomains = map($domains, fn (Domain $domain) => DomainItem::forExistingDomain($domain));
+        $mappedDomains = map($domains, fn (Domain $domain) => DomainItem::forNonDefaultDomain($domain));
 
         if ($apiKey?->hasRole(Role::DOMAIN_SPECIFIC)) {
             return $mappedDomains;
         }
 
         return [
-            DomainItem::forDefaultDomain($this->defaultDomain, $default ?? $this->redirectOptions),
+            DomainItem::forDefaultDomain($this->defaultDomain, $default ?? new EmptyNotFoundRedirectConfig()),
             ...$mappedDomains,
         ];
     }

module/Core/src/Domain/Model/DomainItem.php

@@ -18,7 +18,7 @@ final class DomainItem implements JsonSerializable
     ) {
     }
 
-    public static function forExistingDomain(Domain $domain): self
+    public static function forNonDefaultDomain(Domain $domain): self
     {
         return new self($domain->getAuthority(), $domain, false);
     }

module/Core/src/Options/NotFoundRedirectOptions.php

@@ -4,11 +4,10 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Core\Options;
 
-use JsonSerializable;
 use Laminas\Stdlib\AbstractOptions;
 use Shlinkio\Shlink\Core\Config\NotFoundRedirectConfigInterface;
 
-class NotFoundRedirectOptions extends AbstractOptions implements NotFoundRedirectConfigInterface, JsonSerializable
+class NotFoundRedirectOptions extends AbstractOptions implements NotFoundRedirectConfigInterface
 {
     private ?string $invalidShortUrl = null;
     private ?string $regular404 = null;
@@ -61,13 +60,4 @@ class NotFoundRedirectOptions extends AbstractOptions implements NotFoundRedirec
         $this->baseUrl = $baseUrl;
         return $this;
     }
-
-    public function jsonSerialize(): array
-    {
-        return [
-            'baseUrlRedirect' => $this->baseUrl,
-            'regular404Redirect' => $this->regular404,
-            'invalidShortUrlRedirect' => $this->invalidShortUrl,
-        ];
-    }
 }

module/Core/test/Config/EmptyNotFoundRedirectConfigTest.php

@@ -0,0 +1,29 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\Config;
+
+use PHPUnit\Framework\TestCase;
+use Shlinkio\Shlink\Core\Config\EmptyNotFoundRedirectConfig;
+
+class EmptyNotFoundRedirectConfigTest extends TestCase
+{
+    private EmptyNotFoundRedirectConfig $redirectsConfig;
+
+    protected function setUp(): void
+    {
+        $this->redirectsConfig = new EmptyNotFoundRedirectConfig();
+    }
+
+    /** @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());
+    }
+}

module/Core/test/Domain/DomainServiceTest.php

@@ -9,13 +9,13 @@ use PHPUnit\Framework\TestCase;
 use Prophecy\Argument;
 use Prophecy\PhpUnit\ProphecyTrait;
 use Prophecy\Prophecy\ObjectProphecy;
+use Shlinkio\Shlink\Core\Config\EmptyNotFoundRedirectConfig;
 use Shlinkio\Shlink\Core\Config\NotFoundRedirects;
 use Shlinkio\Shlink\Core\Domain\DomainService;
 use Shlinkio\Shlink\Core\Domain\Model\DomainItem;
 use Shlinkio\Shlink\Core\Domain\Repository\DomainRepositoryInterface;
 use Shlinkio\Shlink\Core\Entity\Domain;
 use Shlinkio\Shlink\Core\Exception\DomainNotFoundException;
-use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions;
 use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta;
 use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition;
 use Shlinkio\Shlink\Rest\Entity\ApiKey;
@@ -30,7 +30,7 @@ class DomainServiceTest extends TestCase
     public function setUp(): void
     {
         $this->em = $this->prophesize(EntityManagerInterface::class);
-        $this->domainService = new DomainService($this->em->reveal(), 'default.com', new NotFoundRedirectOptions());
+        $this->domainService = new DomainService($this->em->reveal(), 'default.com');
     }
 
     /**
@@ -52,7 +52,7 @@ class DomainServiceTest extends TestCase
 
     public function provideExcludedDomains(): iterable
     {
-        $default = DomainItem::forDefaultDomain('default.com', new NotFoundRedirectOptions());
+        $default = DomainItem::forDefaultDomain('default.com', new EmptyNotFoundRedirectConfig());
         $adminApiKey = ApiKey::create();
         $domainSpecificApiKey = ApiKey::fromMeta(
             ApiKeyMeta::withRoles(RoleDefinition::forDomain(Domain::withAuthority('')->setId('123'))),
@@ -61,15 +61,15 @@ class DomainServiceTest extends TestCase
         yield 'empty list without API key' => [[], [$default], null];
         yield 'one item without API key' => [
             [Domain::withAuthority('bar.com')],
-            [$default, DomainItem::forExistingDomain(Domain::withAuthority('bar.com'))],
+            [$default, DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com'))],
             null,
         ];
         yield 'multiple items without API key' => [
             [Domain::withAuthority('foo.com'), Domain::withAuthority('bar.com')],
             [
                 $default,
-                DomainItem::forExistingDomain(Domain::withAuthority('foo.com')),
+                DomainItem::forNonDefaultDomain(Domain::withAuthority('foo.com')),
-                DomainItem::forExistingDomain(Domain::withAuthority('bar.com')),
+                DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com')),
             ],
             null,
         ];
@@ -77,15 +77,15 @@ class DomainServiceTest extends TestCase
         yield 'empty list with admin API key' => [[], [$default], $adminApiKey];
         yield 'one item with admin API key' => [
             [Domain::withAuthority('bar.com')],
-            [$default, DomainItem::forExistingDomain(Domain::withAuthority('bar.com'))],
+            [$default, DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com'))],
             $adminApiKey,
         ];
         yield 'multiple items with admin API key' => [
             [Domain::withAuthority('foo.com'), Domain::withAuthority('bar.com')],
             [
                 $default,
-                DomainItem::forExistingDomain(Domain::withAuthority('foo.com')),
+                DomainItem::forNonDefaultDomain(Domain::withAuthority('foo.com')),
-                DomainItem::forExistingDomain(Domain::withAuthority('bar.com')),
+                DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com')),
             ],
             $adminApiKey,
         ];
@@ -93,14 +93,14 @@ class DomainServiceTest extends TestCase
         yield 'empty list with domain-specific API key' => [[], [], $domainSpecificApiKey];
         yield 'one item with domain-specific API key' => [
             [Domain::withAuthority('bar.com')],
-            [DomainItem::forExistingDomain(Domain::withAuthority('bar.com'))],
+            [DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com'))],
             $domainSpecificApiKey,
         ];
         yield 'multiple items with domain-specific API key' => [
             [Domain::withAuthority('foo.com'), Domain::withAuthority('bar.com')],
             [
-                DomainItem::forExistingDomain(Domain::withAuthority('foo.com')),
+                DomainItem::forNonDefaultDomain(Domain::withAuthority('foo.com')),
-                DomainItem::forExistingDomain(Domain::withAuthority('bar.com')),
+                DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com')),
             ],
             $domainSpecificApiKey,
         ];

module/Rest/src/Action/Domain/ListDomainsAction.php

@@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Rest\Action\Domain;
 use Laminas\Diactoros\Response\JsonResponse;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
+use Shlinkio\Shlink\Core\Config\NotFoundRedirects;
 use Shlinkio\Shlink\Core\Domain\DomainServiceInterface;
 use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions;
 use Shlinkio\Shlink\Rest\Action\AbstractRestAction;
@@ -29,7 +30,7 @@ class ListDomainsAction extends AbstractRestAction
         return new JsonResponse([
             'domains' => [
                 'data' => $domainItems,
-                'defaultRedirects' => $this->options,
+                'defaultRedirects' => NotFoundRedirects::fromConfig($this->options),
             ],
         ]);
     }

module/Rest/test/Action/Domain/ListDomainsActionTest.php

@@ -9,6 +9,7 @@ use Laminas\Diactoros\ServerRequestFactory;
 use PHPUnit\Framework\TestCase;
 use Prophecy\PhpUnit\ProphecyTrait;
 use Prophecy\Prophecy\ObjectProphecy;
+use Shlinkio\Shlink\Core\Config\NotFoundRedirects;
 use Shlinkio\Shlink\Core\Domain\DomainServiceInterface;
 use Shlinkio\Shlink\Core\Domain\Model\DomainItem;
 use Shlinkio\Shlink\Core\Entity\Domain;
@@ -37,7 +38,7 @@ class ListDomainsActionTest extends TestCase
         $apiKey = ApiKey::create();
         $domains = [
             DomainItem::forDefaultDomain('bar.com', new NotFoundRedirectOptions()),
-            DomainItem::forExistingDomain(Domain::withAuthority('baz.com')),
+            DomainItem::forNonDefaultDomain(Domain::withAuthority('baz.com')),
         ];
         $listDomains = $this->domainService->listDomains($apiKey)->willReturn($domains);
 
@@ -48,7 +49,7 @@ class ListDomainsActionTest extends TestCase
         self::assertEquals([
             'domains' => [
                 'data' => $domains,
-                'defaultRedirects' => $this->options,
+                'defaultRedirects' => NotFoundRedirects::fromConfig($this->options),
             ],
         ], $payload);
         $listDomains->shouldHaveBeenCalledOnce();