From c582eba753196723595e3feb3474d49ea5178a67 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 22 Apr 2023 19:44:04 +0200 Subject: [PATCH] Make sure short URL domain is resolved as null when default one is provided --- .../Command/ShortUrl/CreateShortUrlCommand.php | 9 --------- .../ShortUrl/CreateShortUrlCommandTest.php | 9 +++------ module/Core/config/dependencies.config.php | 2 +- module/Core/src/Options/UrlShortenerOptions.php | 5 +++++ .../PersistenceShortUrlRelationResolver.php | 15 ++++++++------- .../Core/src/ShortUrl/ShortUrlListService.php | 2 +- .../PersistenceShortUrlRelationResolverTest.php | 17 +++++++++++++---- 7 files changed, 31 insertions(+), 28 deletions(-) diff --git a/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php index bb332d82..a998a677 100644 --- a/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php @@ -31,7 +31,6 @@ class CreateShortUrlCommand extends Command public const NAME = 'short-url:create'; private ?SymfonyStyle $io; - private string $defaultDomain; public function __construct( private readonly UrlShortenerInterface $urlShortener, @@ -39,7 +38,6 @@ class CreateShortUrlCommand extends Command private readonly UrlShortenerOptions $options, ) { parent::__construct(); - $this->defaultDomain = $this->options->domain['hostname'] ?? ''; } protected function configure(): void @@ -121,7 +119,6 @@ class CreateShortUrlCommand extends Command protected function interact(InputInterface $input, OutputInterface $output): void { $this->verifyLongUrlArgument($input, $output); - $this->verifyDomainArgument($input); } private function verifyLongUrlArgument(InputInterface $input, OutputInterface $output): void @@ -138,12 +135,6 @@ class CreateShortUrlCommand extends Command } } - private function verifyDomainArgument(InputInterface $input): void - { - $domain = $input->getOption('domain'); - $input->setOption('domain', $domain === $this->defaultDomain ? null : $domain); - } - protected function execute(InputInterface $input, OutputInterface $output): ?int { $io = $this->getIO($input, $output); diff --git a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php index fd474007..60482138 100644 --- a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php @@ -28,8 +28,6 @@ class CreateShortUrlCommandTest extends TestCase { use CliTestUtilsTrait; - private const DEFAULT_DOMAIN = 'default.com'; - private CommandTester $commandTester; private MockObject & UrlShortenerInterface $urlShortener; private MockObject & ShortUrlStringifierInterface $stringifier; @@ -43,7 +41,7 @@ class CreateShortUrlCommandTest extends TestCase $this->urlShortener, $this->stringifier, new UrlShortenerOptions( - domain: ['hostname' => self::DEFAULT_DOMAIN, 'schema' => ''], + domain: ['hostname' => 'example.com', 'schema' => ''], defaultShortCodesLength: 5, ), ); @@ -147,9 +145,8 @@ class CreateShortUrlCommandTest extends TestCase public static function provideDomains(): iterable { yield 'no domain' => [[], null]; - yield 'non-default domain foo' => [['--domain' => 'foo.com'], 'foo.com']; - yield 'non-default domain bar' => [['-d' => 'bar.com'], 'bar.com']; - yield 'default domain' => [['--domain' => self::DEFAULT_DOMAIN], null]; + yield 'domain foo' => [['--domain' => 'foo.com'], 'foo.com']; + yield 'domain bar' => [['-d' => 'bar.com'], 'bar.com']; } #[Test, DataProvider('provideFlags')] diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 567d57ce..008db777 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -161,7 +161,7 @@ return [ ], Action\RobotsAction::class => [Crawling\CrawlingHelper::class], - ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => ['em'], + ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => ['em', Options\UrlShortenerOptions::class], ShortUrl\Helper\ShortUrlStringifier::class => ['config.url_shortener.domain', 'config.router.base_path'], ShortUrl\Helper\ShortUrlTitleResolutionHelper::class => [Util\UrlValidator::class], ShortUrl\Helper\ShortUrlRedirectionBuilder::class => [Options\TrackingOptions::class], diff --git a/module/Core/src/Options/UrlShortenerOptions.php b/module/Core/src/Options/UrlShortenerOptions.php index 6e6ac087..32b40033 100644 --- a/module/Core/src/Options/UrlShortenerOptions.php +++ b/module/Core/src/Options/UrlShortenerOptions.php @@ -26,4 +26,9 @@ final class UrlShortenerOptions { return $this->mode === ShortUrlMode::LOOSE; } + + public function defaultDomain(): string + { + return $this->domain['hostname'] ?? ''; + } } diff --git a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php index db6721d5..48ec634e 100644 --- a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php +++ b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php @@ -9,6 +9,7 @@ use Doctrine\Common\Collections\Collection; use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\Events; use Shlinkio\Shlink\Core\Domain\Entity\Domain; +use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Tag\Entity\Tag; use function Functional\map; @@ -21,15 +22,17 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt /** @var array */ private array $memoizedNewTags = []; - public function __construct(private readonly EntityManagerInterface $em) - { + public function __construct( + private readonly EntityManagerInterface $em, + private readonly UrlShortenerOptions $options = new UrlShortenerOptions(), + ) { // Registering this as an event listener will make the postFlush method to be called automatically $this->em->getEventManager()->addEventListener(Events::postFlush, $this); } public function resolveDomain(?string $domain): ?Domain { - if ($domain === null) { + if ($domain === null || $domain === $this->options->defaultDomain()) { return null; } @@ -42,9 +45,7 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt private function memoizeNewDomain(string $domain): Domain { - return $this->memoizedNewDomains[$domain] = $this->memoizedNewDomains[$domain] ?? Domain::withAuthority( - $domain, - ); + return $this->memoizedNewDomains[$domain] ??= Domain::withAuthority($domain); } /** @@ -71,7 +72,7 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt private function memoizeNewTag(string $tagName): Tag { - return $this->memoizedNewTags[$tagName] = $this->memoizedNewTags[$tagName] ?? new Tag($tagName); + return $this->memoizedNewTags[$tagName] ??= new Tag($tagName); } public function postFlush(): void diff --git a/module/Core/src/ShortUrl/ShortUrlListService.php b/module/Core/src/ShortUrl/ShortUrlListService.php index d83647f0..60f56554 100644 --- a/module/Core/src/ShortUrl/ShortUrlListService.php +++ b/module/Core/src/ShortUrl/ShortUrlListService.php @@ -25,7 +25,7 @@ class ShortUrlListService implements ShortUrlListServiceInterface */ public function listShortUrls(ShortUrlsParams $params, ?ApiKey $apiKey = null): Paginator { - $defaultDomain = $this->urlShortenerOptions->domain['hostname'] ?? ''; + $defaultDomain = $this->urlShortenerOptions->defaultDomain(); $paginator = new Paginator(new ShortUrlRepositoryAdapter($this->repo, $params, $apiKey, $defaultDomain)); $paginator->setMaxPerPage($params->itemsPerPage) ->setCurrentPage($params->page); diff --git a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php index 43f99462..d31f3d9b 100644 --- a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php +++ b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php @@ -12,6 +12,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Domain\Entity\Domain; use Shlinkio\Shlink\Core\Domain\Repository\DomainRepositoryInterface; +use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\ShortUrl\Resolver\PersistenceShortUrlRelationResolver; use Shlinkio\Shlink\Core\Tag\Entity\Tag; use Shlinkio\Shlink\Core\Tag\Repository\TagRepositoryInterface; @@ -28,14 +29,22 @@ class PersistenceShortUrlRelationResolverTest extends TestCase $this->em = $this->createMock(EntityManagerInterface::class); $this->em->method('getEventManager')->willReturn(new EventManager()); - $this->resolver = new PersistenceShortUrlRelationResolver($this->em); + $this->resolver = new PersistenceShortUrlRelationResolver($this->em, new UrlShortenerOptions( + domain: ['schema' => 'https', 'hostname' => 'default.com'], + )); } - #[Test] - public function returnsEmptyWhenNoDomainIsProvided(): void + #[Test, DataProvider('provideDomainsThatEmpty')] + public function returnsEmptyInSomeCases(?string $domain): void { $this->em->expects($this->never())->method('getRepository')->with(Domain::class); - self::assertNull($this->resolver->resolveDomain(null)); + self::assertNull($this->resolver->resolveDomain($domain)); + } + + public static function provideDomainsThatEmpty(): iterable + { + yield 'null' => [null]; + yield 'default domain' => ['default.com']; } #[Test, DataProvider('provideFoundDomains')]