diff --git a/CHANGELOG.md b/CHANGELOG.md index 46e47408..7eb2fa25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#1300](https://github.com/shlinkio/shlink/issues/1300) Changed default ordering for short URLs list, returning always from newest to oldest. * [#1299](https://github.com/shlinkio/shlink/issues/1299) Updated to the latest base docker images, based in PHP 8.1.1, and bumped openswoole to v4.9.1. * [#1282](https://github.com/shlinkio/shlink/issues/1282) Env vars now have precedence over installer options. +* [#1328](https://github.com/shlinkio/shlink/issues/1328) Refactored ShortUrlsRepository to use DTOs in methods with too many arguments. ### Deprecated * [#1315](https://github.com/shlinkio/shlink/issues/1315) Deprecated `GET /tags?withStats=true` endpoint. Use `GET /tags/stats` instead. diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index f644d0d1..9852c530 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -127,7 +127,7 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU return $qb; } - public function findOneWithDomainFallback(string $shortCode, ?string $domain = null): ?ShortUrl + public function findOneWithDomainFallback(ShortUrlIdentifier $identifier): ?ShortUrl { // When ordering DESC, Postgres puts nulls at the beginning while the rest of supported DB engines put them at // the bottom @@ -146,8 +146,8 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU $query = $this->getEntityManager()->createQuery($dql); $query->setMaxResults(1) ->setParameters([ - 'shortCode' => $shortCode, - 'domain' => $domain, + 'shortCode' => $identifier->shortCode(), + 'domain' => $identifier->domain(), ]); // Since we ordered by domain, we will have first the URL matching provided domain, followed by the one diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index 4a8d04df..cfc36e0e 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -20,8 +20,7 @@ interface ShortUrlRepositoryInterface extends ObjectRepository, EntitySpecificat public function countList(ShortUrlsCountFiltering $filtering): int; - // TODO Use ShortUrlIdentifier here - public function findOneWithDomainFallback(string $shortCode, ?string $domain = null): ?ShortUrl; + public function findOneWithDomainFallback(ShortUrlIdentifier $identifier): ?ShortUrl; public function findOne(ShortUrlIdentifier $identifier, ?Specification $spec = null): ?ShortUrl; diff --git a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php index 61c57d36..3abd90c8 100644 --- a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php +++ b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php @@ -39,7 +39,7 @@ class ShortUrlResolver implements ShortUrlResolverInterface { /** @var ShortUrlRepository $shortUrlRepo */ $shortUrlRepo = $this->em->getRepository(ShortUrl::class); - $shortUrl = $shortUrlRepo->findOneWithDomainFallback($identifier->shortCode(), $identifier->domain()); + $shortUrl = $shortUrlRepo->findOneWithDomainFallback($identifier); if (! $shortUrl?->isEnabled()) { throw ShortUrlNotFoundException::fromNotFound($identifier); } diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index ed7cd0ab..30b95774 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -57,25 +57,32 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - self::assertSame($regularOne, $this->repo->findOneWithDomainFallback($regularOne->getShortCode())); self::assertSame($regularOne, $this->repo->findOneWithDomainFallback( - $withDomainDuplicatingRegular->getShortCode(), + ShortUrlIdentifier::fromShortCodeAndDomain($regularOne->getShortCode()), + )); + self::assertSame($regularOne, $this->repo->findOneWithDomainFallback( + ShortUrlIdentifier::fromShortCodeAndDomain($withDomainDuplicatingRegular->getShortCode()), )); self::assertSame($withDomain, $this->repo->findOneWithDomainFallback( - $withDomain->getShortCode(), - 'example.com', + ShortUrlIdentifier::fromShortCodeAndDomain($withDomain->getShortCode(), 'example.com'), )); self::assertSame( $withDomainDuplicatingRegular, - $this->repo->findOneWithDomainFallback($withDomainDuplicatingRegular->getShortCode(), 'doma.in'), + $this->repo->findOneWithDomainFallback( + ShortUrlIdentifier::fromShortCodeAndDomain($withDomainDuplicatingRegular->getShortCode(), 'doma.in'), + ), ); - self::assertSame( - $regularOne, - $this->repo->findOneWithDomainFallback($withDomainDuplicatingRegular->getShortCode(), 'other-domain.com'), - ); - self::assertNull($this->repo->findOneWithDomainFallback('invalid')); - self::assertNull($this->repo->findOneWithDomainFallback($withDomain->getShortCode())); - self::assertNull($this->repo->findOneWithDomainFallback($withDomain->getShortCode(), 'other-domain.com')); + self::assertSame($regularOne, $this->repo->findOneWithDomainFallback(ShortUrlIdentifier::fromShortCodeAndDomain( + $withDomainDuplicatingRegular->getShortCode(), + 'other-domain.com', + ))); + self::assertNull($this->repo->findOneWithDomainFallback(ShortUrlIdentifier::fromShortCodeAndDomain('invalid'))); + self::assertNull($this->repo->findOneWithDomainFallback( + ShortUrlIdentifier::fromShortCodeAndDomain($withDomain->getShortCode()), + )); + self::assertNull($this->repo->findOneWithDomainFallback( + ShortUrlIdentifier::fromShortCodeAndDomain($withDomain->getShortCode(), 'other-domain.com'), + )); } /** @test */ diff --git a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php index 41f2b492..70857e5e 100644 --- a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php +++ b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php @@ -86,7 +86,9 @@ class ShortUrlResolverTest extends TestCase $shortCode = $shortUrl->getShortCode(); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn($shortUrl); + $findOneByShortCode = $repo->findOneWithDomainFallback( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), + )->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $result = $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode)); @@ -105,7 +107,9 @@ class ShortUrlResolverTest extends TestCase $shortCode = $shortUrl->getShortCode(); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn($shortUrl); + $findOneByShortCode = $repo->findOneWithDomainFallback( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), + )->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->expectException(ShortUrlNotFoundException::class);