From 5f87bb13f8633cb46d86eba95b5443f7c801cca8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Sep 2022 11:27:56 +0200 Subject: [PATCH 01/11] Fixed tracking config --- config/autoload/tracking.global.php | 47 ++++++++++++++++------------- config/roadrunner/.rr.dev.yml | 4 +-- 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/config/autoload/tracking.global.php b/config/autoload/tracking.global.php index 0637301a..1fc05b3b 100644 --- a/config/autoload/tracking.global.php +++ b/config/autoload/tracking.global.php @@ -4,33 +4,38 @@ declare(strict_types=1); use Shlinkio\Shlink\Core\Config\EnvVars; -return [ +return (static function (): array { + /** @var string|null $disableTrackingFrom */ + $disableTrackingFrom = EnvVars::DISABLE_TRACKING_FROM->loadFromEnv(); - 'tracking' => [ - // Tells if IP addresses should be anonymized before persisting, to fulfil data protection regulations - // This applies only if IP address tracking is enabled - 'anonymize_remote_addr' => (bool) EnvVars::ANONYMIZE_REMOTE_ADDR->loadFromEnv(true), + return [ - // Tells if visits to not-found URLs should be tracked. The disable_tracking option takes precedence - 'track_orphan_visits' => (bool) EnvVars::TRACK_ORPHAN_VISITS->loadFromEnv(true), + 'tracking' => [ + // Tells if IP addresses should be anonymized before persisting, to fulfil data protection regulations + // This applies only if IP address tracking is enabled + 'anonymize_remote_addr' => (bool) EnvVars::ANONYMIZE_REMOTE_ADDR->loadFromEnv(true), - // A query param that, if provided, will disable tracking of one particular visit. Always takes precedence - 'disable_track_param' => EnvVars::DISABLE_TRACK_PARAM->loadFromEnv(), + // Tells if visits to not-found URLs should be tracked. The disable_tracking option takes precedence + 'track_orphan_visits' => (bool) EnvVars::TRACK_ORPHAN_VISITS->loadFromEnv(true), - // If true, visits will not be tracked at all - 'disable_tracking' => (bool) EnvVars::DISABLE_TRACKING->loadFromEnv(false), + // A query param that, if provided, will disable tracking of one particular visit. Always takes precedence + 'disable_track_param' => EnvVars::DISABLE_TRACK_PARAM->loadFromEnv(), - // If true, visits will be tracked, but neither the IP address, nor the location will be resolved - 'disable_ip_tracking' => (bool) EnvVars::DISABLE_IP_TRACKING->loadFromEnv(false), + // If true, visits will not be tracked at all + 'disable_tracking' => (bool) EnvVars::DISABLE_TRACKING->loadFromEnv(false), - // If true, the referrer will not be tracked - 'disable_referrer_tracking' => (bool) EnvVars::DISABLE_REFERRER_TRACKING->loadFromEnv(false), + // If true, visits will be tracked, but neither the IP address, nor the location will be resolved + 'disable_ip_tracking' => (bool) EnvVars::DISABLE_IP_TRACKING->loadFromEnv(false), - // If true, the user agent will not be tracked - 'disable_ua_tracking' => (bool) EnvVars::DISABLE_UA_TRACKING->loadFromEnv(false), + // If true, the referrer will not be tracked + 'disable_referrer_tracking' => (bool) EnvVars::DISABLE_REFERRER_TRACKING->loadFromEnv(false), - // A list of IP addresses, patterns or CIDR blocks from which tracking is disabled by default - 'disable_tracking_from' => EnvVars::DISABLE_TRACKING_FROM->loadFromEnv(), - ], + // If true, the user agent will not be tracked + 'disable_ua_tracking' => (bool) EnvVars::DISABLE_UA_TRACKING->loadFromEnv(false), -]; + // A list of IP addresses, patterns or CIDR blocks from which tracking is disabled by default + 'disable_tracking_from' => $disableTrackingFrom === null ? [] : explode(',', $disableTrackingFrom), + ], + + ]; +})(); diff --git a/config/roadrunner/.rr.dev.yml b/config/roadrunner/.rr.dev.yml index 7adf4520..cc0bbf29 100644 --- a/config/roadrunner/.rr.dev.yml +++ b/config/roadrunner/.rr.dev.yml @@ -13,11 +13,11 @@ http: dir: '../../public' forbid: ['.php', '.htaccess'] pool: - num_workers: 16 + num_workers: 1 jobs: pool: - num_workers: 16 + num_workers: 1 timeout: 300 consume: ['shlink'] pipelines: From fe4b2c4ae49f0bdc3242a71a9d67cc9e8a2ba9d3 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Sep 2022 12:57:04 +0200 Subject: [PATCH 02/11] Migrated TrackingOptions to immutable object --- composer.json | 4 +- config/autoload/tracking.global.php | 4 +- module/CLI/src/Util/GeolocationDbUpdater.php | 2 +- .../test/Util/GeolocationDbUpdaterTest.php | 45 ++++--- module/Core/config/dependencies.config.php | 4 +- module/Core/src/Model/Visitor.php | 6 +- module/Core/src/Options/TrackingOptions.php | 114 +++--------------- .../Helper/ShortUrlRedirectionBuilder.php | 2 +- module/Core/src/Visit/RequestTracker.php | 2 +- module/Core/src/Visit/VisitsTracker.php | 12 +- module/Core/test/Model/VisitorTest.php | 10 +- .../Helper/ShortUrlRedirectionBuilderTest.php | 2 +- module/Core/test/Visit/RequestTrackerTest.php | 8 +- module/Core/test/Visit/VisitsTrackerTest.php | 24 ++-- 14 files changed, 77 insertions(+), 162 deletions(-) diff --git a/composer.json b/composer.json index a5bc5b62..8a53f16c 100644 --- a/composer.json +++ b/composer.json @@ -43,8 +43,8 @@ "php-middleware/request-id": "^4.1", "pugx/shortid-php": "^1.0", "ramsey/uuid": "^4.3", - "shlinkio/shlink-common": "^5.0", - "shlinkio/shlink-config": "dev-main#33004e6 as 2.1", + "shlinkio/shlink-common": "dev-main#c9e6474 as 5.1", + "shlinkio/shlink-config": "dev-main#12fb295 as 2.1", "shlinkio/shlink-event-dispatcher": "dev-main#48c0137 as 2.6", "shlinkio/shlink-importer": "^4.0", "shlinkio/shlink-installer": "dev-develop#a01bca9 as 8.2", diff --git a/config/autoload/tracking.global.php b/config/autoload/tracking.global.php index 1fc05b3b..4d7a6e9a 100644 --- a/config/autoload/tracking.global.php +++ b/config/autoload/tracking.global.php @@ -34,7 +34,9 @@ return (static function (): array { 'disable_ua_tracking' => (bool) EnvVars::DISABLE_UA_TRACKING->loadFromEnv(false), // A list of IP addresses, patterns or CIDR blocks from which tracking is disabled by default - 'disable_tracking_from' => $disableTrackingFrom === null ? [] : explode(',', $disableTrackingFrom), + 'disable_tracking_from' => $disableTrackingFrom === null + ? [] + : array_map(trim(...), explode(',', $disableTrackingFrom)), ], ]; diff --git a/module/CLI/src/Util/GeolocationDbUpdater.php b/module/CLI/src/Util/GeolocationDbUpdater.php index 22a3bac5..913ad438 100644 --- a/module/CLI/src/Util/GeolocationDbUpdater.php +++ b/module/CLI/src/Util/GeolocationDbUpdater.php @@ -32,7 +32,7 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface */ public function checkDbUpdate(?callable $beforeDownload = null, ?callable $handleProgress = null): void { - if ($this->trackingOptions->disableTracking() || $this->trackingOptions->disableIpTracking()) { + if ($this->trackingOptions->disableTracking || $this->trackingOptions->disableIpTracking) { return; } diff --git a/module/CLI/test/Util/GeolocationDbUpdaterTest.php b/module/CLI/test/Util/GeolocationDbUpdaterTest.php index fde39775..a884dd7c 100644 --- a/module/CLI/test/Util/GeolocationDbUpdaterTest.php +++ b/module/CLI/test/Util/GeolocationDbUpdaterTest.php @@ -29,7 +29,6 @@ class GeolocationDbUpdaterTest extends TestCase private GeolocationDbUpdater $geolocationDbUpdater; private ObjectProphecy $dbUpdater; private ObjectProphecy $geoLiteDbReader; - private TrackingOptions $trackingOptions; private ObjectProphecy $lock; protected function setUp(): void @@ -38,19 +37,10 @@ class GeolocationDbUpdaterTest extends TestCase $this->geoLiteDbReader = $this->prophesize(Reader::class); $this->trackingOptions = new TrackingOptions(); - $locker = $this->prophesize(Lock\LockFactory::class); $this->lock = $this->prophesize(Lock\LockInterface::class); $this->lock->acquire(true)->willReturn(true); $this->lock->release()->will(function (): void { }); - $locker->createLock(Argument::type('string'))->willReturn($this->lock->reveal()); - - $this->geolocationDbUpdater = new GeolocationDbUpdater( - $this->dbUpdater->reveal(), - $this->geoLiteDbReader->reveal(), - $locker->reveal(), - $this->trackingOptions, - ); } /** @test */ @@ -64,7 +54,7 @@ class GeolocationDbUpdaterTest extends TestCase $download = $this->dbUpdater->downloadFreshCopy(null)->willThrow($prev); try { - $this->geolocationDbUpdater->checkDbUpdate($mustBeUpdated); + $this->geolocationDbUpdater()->checkDbUpdate($mustBeUpdated); self::assertTrue(false); // If this is reached, the test will fail } catch (Throwable $e) { /** @var GeolocationDbUpdateFailedException $e */ @@ -94,7 +84,7 @@ class GeolocationDbUpdaterTest extends TestCase $download = $this->dbUpdater->downloadFreshCopy(null)->willThrow($prev); try { - $this->geolocationDbUpdater->checkDbUpdate(); + $this->geolocationDbUpdater()->checkDbUpdate(); self::assertTrue(false); // If this is reached, the test will fail } catch (Throwable $e) { /** @var GeolocationDbUpdateFailedException $e */ @@ -127,7 +117,7 @@ class GeolocationDbUpdaterTest extends TestCase $download = $this->dbUpdater->downloadFreshCopy(null)->will(function (): void { }); - $this->geolocationDbUpdater->checkDbUpdate(); + $this->geolocationDbUpdater()->checkDbUpdate(); $fileExists->shouldHaveBeenCalledOnce(); $getMeta->shouldHaveBeenCalledOnce(); @@ -160,7 +150,7 @@ class GeolocationDbUpdaterTest extends TestCase $getMeta->shouldBeCalledOnce(); $download->shouldNotBeCalled(); - $this->geolocationDbUpdater->checkDbUpdate(); + $this->geolocationDbUpdater()->checkDbUpdate(); } private function buildMetaWithBuildEpoch(string|int $buildEpoch): Metadata @@ -182,13 +172,9 @@ class GeolocationDbUpdaterTest extends TestCase * @test * @dataProvider provideTrackingOptions */ - public function downloadDbIsSkippedIfTrackingIsDisabled(array $props): void + public function downloadDbIsSkippedIfTrackingIsDisabled(TrackingOptions $options): void { - foreach ($props as $prop) { - $this->trackingOptions->{$prop} = true; - } - - $this->geolocationDbUpdater->checkDbUpdate(); + $this->geolocationDbUpdater($options)->checkDbUpdate(); $this->dbUpdater->databaseFileExists(Argument::cetera())->shouldNotHaveBeenCalled(); $this->geoLiteDbReader->metadata(Argument::cetera())->shouldNotHaveBeenCalled(); @@ -196,8 +182,21 @@ class GeolocationDbUpdaterTest extends TestCase public function provideTrackingOptions(): iterable { - yield 'disableTracking' => [['disableTracking']]; - yield 'disableIpTracking' => [['disableIpTracking']]; - yield 'both' => [['disableTracking', 'disableIpTracking']]; + yield 'disableTracking' => [new TrackingOptions(disableTracking: true)]; + yield 'disableIpTracking' => [new TrackingOptions(disableIpTracking: true)]; + yield 'both' => [new TrackingOptions(disableTracking: true, disableIpTracking: true)]; + } + + private function geolocationDbUpdater(?TrackingOptions $options = null): GeolocationDbUpdater + { + $locker = $this->prophesize(Lock\LockFactory::class); + $locker->createLock(Argument::type('string'))->willReturn($this->lock->reveal()); + + return new GeolocationDbUpdater( + $this->dbUpdater->reveal(), + $this->geoLiteDbReader->reveal(), + $locker->reveal(), + $options ?? new TrackingOptions(), + ); } } diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 9edc5fc2..d6cbd72a 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Core; use Laminas\ServiceManager\AbstractFactory\ConfigAbstractFactory; use Laminas\ServiceManager\Factory\InvokableFactory; use Psr\EventDispatcher\EventDispatcherInterface; +use Shlinkio\Shlink\Config\Factory\ValinorConfigFactory; use Shlinkio\Shlink\Core\ErrorHandler; use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; @@ -25,7 +26,7 @@ return [ Options\NotFoundRedirectOptions::class => ConfigAbstractFactory::class, Options\RedirectOptions::class => ConfigAbstractFactory::class, Options\UrlShortenerOptions::class => ConfigAbstractFactory::class, - Options\TrackingOptions::class => ConfigAbstractFactory::class, + Options\TrackingOptions::class => [ValinorConfigFactory::class, 'config.tracking'], Options\QrCodeOptions::class => ConfigAbstractFactory::class, Options\RabbitMqOptions::class => ConfigAbstractFactory::class, Options\WebhookOptions::class => ConfigAbstractFactory::class, @@ -90,7 +91,6 @@ return [ Options\NotFoundRedirectOptions::class => ['config.not_found_redirects'], Options\RedirectOptions::class => ['config.redirects'], Options\UrlShortenerOptions::class => ['config.url_shortener'], - Options\TrackingOptions::class => ['config.tracking'], Options\QrCodeOptions::class => ['config.qr_codes'], Options\RabbitMqOptions::class => ['config.rabbitmq'], Options\WebhookOptions::class => ['config.visits_webhooks'], diff --git a/module/Core/src/Model/Visitor.php b/module/Core/src/Model/Visitor.php index 2207fad8..61663b95 100644 --- a/module/Core/src/Model/Visitor.php +++ b/module/Core/src/Model/Visitor.php @@ -69,9 +69,9 @@ final class Visitor public function normalizeForTrackingOptions(TrackingOptions $options): self { $instance = new self( - $options->disableUaTracking() ? '' : $this->userAgent, - $options->disableReferrerTracking() ? '' : $this->referer, - $options->disableIpTracking() ? null : $this->remoteAddress, + $options->disableUaTracking ? '' : $this->userAgent, + $options->disableReferrerTracking ? '' : $this->referer, + $options->disableIpTracking ? null : $this->remoteAddress, $this->visitedUrl, ); diff --git a/module/Core/src/Options/TrackingOptions.php b/module/Core/src/Options/TrackingOptions.php index ba51b8e9..d4272374 100644 --- a/module/Core/src/Options/TrackingOptions.php +++ b/module/Core/src/Options/TrackingOptions.php @@ -4,103 +4,21 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Options; -use Laminas\Stdlib\AbstractOptions; - use function array_key_exists; -use function explode; -use function Functional\map; -use function is_array; -use function trim; -class TrackingOptions extends AbstractOptions +final class TrackingOptions { - private bool $anonymizeRemoteAddr = true; - private bool $trackOrphanVisits = true; - private ?string $disableTrackParam = null; - private bool $disableTracking = false; - private bool $disableIpTracking = false; - private bool $disableReferrerTracking = false; - private bool $disableUaTracking = false; - private array $disableTrackingFrom = []; - - public function anonymizeRemoteAddr(): bool - { - return $this->anonymizeRemoteAddr; - } - - protected function setAnonymizeRemoteAddr(bool $anonymizeRemoteAddr): void - { - $this->anonymizeRemoteAddr = $anonymizeRemoteAddr; - } - - public function trackOrphanVisits(): bool - { - return $this->trackOrphanVisits; - } - - protected function setTrackOrphanVisits(bool $trackOrphanVisits): void - { - $this->trackOrphanVisits = $trackOrphanVisits; - } - - public function getDisableTrackParam(): ?string - { - return $this->disableTrackParam; - } - - public function queryHasDisableTrackParam(array $query): bool - { - return $this->disableTrackParam !== null && array_key_exists($this->disableTrackParam, $query); - } - - protected function setDisableTrackParam(?string $disableTrackParam): void - { - $this->disableTrackParam = $disableTrackParam; - } - - public function disableTracking(): bool - { - return $this->disableTracking; - } - - protected function setDisableTracking(bool $disableTracking): void - { - $this->disableTracking = $disableTracking; - } - - public function disableIpTracking(): bool - { - return $this->disableIpTracking; - } - - protected function setDisableIpTracking(bool $disableIpTracking): void - { - $this->disableIpTracking = $disableIpTracking; - } - - public function disableReferrerTracking(): bool - { - return $this->disableReferrerTracking; - } - - protected function setDisableReferrerTracking(bool $disableReferrerTracking): void - { - $this->disableReferrerTracking = $disableReferrerTracking; - } - - public function disableUaTracking(): bool - { - return $this->disableUaTracking; - } - - protected function setDisableUaTracking(bool $disableUaTracking): void - { - $this->disableUaTracking = $disableUaTracking; - } - - public function disableTrackingFrom(): array - { - return $this->disableTrackingFrom; + public function __construct( + public readonly bool $anonymizeRemoteAddr = true, + public readonly bool $trackOrphanVisits = true, + public readonly ?string $disableTrackParam = null, + public readonly bool $disableTracking = false, + public readonly bool $disableIpTracking = false, + public readonly bool $disableReferrerTracking = false, + public readonly bool $disableUaTracking = false, + /** @var string[] */ + public readonly array $disableTrackingFrom = [], + ) { } public function hasDisableTrackingFrom(): bool @@ -108,12 +26,8 @@ class TrackingOptions extends AbstractOptions return ! empty($this->disableTrackingFrom); } - protected function setDisableTrackingFrom(string|array|null $disableTrackingFrom): void + public function queryHasDisableTrackParam(array $query): bool { - $this->disableTrackingFrom = match (true) { - is_array($disableTrackingFrom) => $disableTrackingFrom, - $disableTrackingFrom === null => [], - default => map(explode(',', $disableTrackingFrom), static fn (string $value) => trim($value)), - }; + return $this->disableTrackParam !== null && array_key_exists($this->disableTrackParam, $query); } } diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php index 3251922d..985e2a3f 100644 --- a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php +++ b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php @@ -33,7 +33,7 @@ class ShortUrlRedirectionBuilder implements ShortUrlRedirectionBuilderInterface { $hardcodedQuery = Query::parse($uri->getQuery() ?? ''); - $disableTrackParam = $this->trackingOptions->getDisableTrackParam(); + $disableTrackParam = $this->trackingOptions->disableTrackParam; if ($disableTrackParam !== null) { unset($currentQuery[$disableTrackParam]); } diff --git a/module/Core/src/Visit/RequestTracker.php b/module/Core/src/Visit/RequestTracker.php index 1887dbfd..084dfcf8 100644 --- a/module/Core/src/Visit/RequestTracker.php +++ b/module/Core/src/Visit/RequestTracker.php @@ -83,7 +83,7 @@ class RequestTracker implements RequestTrackerInterface, RequestMethodInterface } $remoteAddrParts = explode('.', $remoteAddr); - $disableTrackingFrom = $this->trackingOptions->disableTrackingFrom(); + $disableTrackingFrom = $this->trackingOptions->disableTrackingFrom; return some($disableTrackingFrom, function (string $value) use ($ip, $remoteAddrParts): bool { $range = str_contains($value, '*') diff --git a/module/Core/src/Visit/VisitsTracker.php b/module/Core/src/Visit/VisitsTracker.php index 585a0f86..1b30155c 100644 --- a/module/Core/src/Visit/VisitsTracker.php +++ b/module/Core/src/Visit/VisitsTracker.php @@ -24,7 +24,7 @@ class VisitsTracker implements VisitsTrackerInterface public function track(ShortUrl $shortUrl, Visitor $visitor): void { $this->trackVisit( - fn (Visitor $v) => Visit::forValidShortUrl($shortUrl, $v, $this->options->anonymizeRemoteAddr()), + fn (Visitor $v) => Visit::forValidShortUrl($shortUrl, $v, $this->options->anonymizeRemoteAddr), $visitor, ); } @@ -32,7 +32,7 @@ class VisitsTracker implements VisitsTrackerInterface public function trackInvalidShortUrlVisit(Visitor $visitor): void { $this->trackOrphanVisit( - fn (Visitor $v) => Visit::forInvalidShortUrl($v, $this->options->anonymizeRemoteAddr()), + fn (Visitor $v) => Visit::forInvalidShortUrl($v, $this->options->anonymizeRemoteAddr), $visitor, ); } @@ -40,7 +40,7 @@ class VisitsTracker implements VisitsTrackerInterface public function trackBaseUrlVisit(Visitor $visitor): void { $this->trackOrphanVisit( - fn (Visitor $v) => Visit::forBasePath($v, $this->options->anonymizeRemoteAddr()), + fn (Visitor $v) => Visit::forBasePath($v, $this->options->anonymizeRemoteAddr), $visitor, ); } @@ -48,14 +48,14 @@ class VisitsTracker implements VisitsTrackerInterface public function trackRegularNotFoundVisit(Visitor $visitor): void { $this->trackOrphanVisit( - fn (Visitor $v) => Visit::forRegularNotFound($v, $this->options->anonymizeRemoteAddr()), + fn (Visitor $v) => Visit::forRegularNotFound($v, $this->options->anonymizeRemoteAddr), $visitor, ); } private function trackOrphanVisit(callable $createVisit, Visitor $visitor): void { - if (! $this->options->trackOrphanVisits()) { + if (! $this->options->trackOrphanVisits) { return; } @@ -64,7 +64,7 @@ class VisitsTracker implements VisitsTrackerInterface private function trackVisit(callable $createVisit, Visitor $visitor): void { - if ($this->options->disableTracking()) { + if ($this->options->disableTracking) { return; } diff --git a/module/Core/test/Model/VisitorTest.php b/module/Core/test/Model/VisitorTest.php index 92a46a16..92c21157 100644 --- a/module/Core/test/Model/VisitorTest.php +++ b/module/Core/test/Model/VisitorTest.php @@ -82,11 +82,11 @@ class VisitorTest extends TestCase $this->generateRandomString(2000), $this->generateRandomString(2000), ); - $normalizedVisitor = $visitor->normalizeForTrackingOptions(new TrackingOptions([ - 'disableIpTracking' => true, - 'disableReferrerTracking' => true, - 'disableUaTracking' => true, - ])); + $normalizedVisitor = $visitor->normalizeForTrackingOptions(new TrackingOptions( + disableIpTracking: true, + disableReferrerTracking: true, + disableUaTracking: true, + )); self::assertNotSame($visitor, $normalizedVisitor); self::assertEmpty($normalizedVisitor->userAgent); diff --git a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php index 829d77ea..97b35f2b 100644 --- a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php +++ b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php @@ -16,7 +16,7 @@ class ShortUrlRedirectionBuilderTest extends TestCase protected function setUp(): void { - $trackingOptions = new TrackingOptions(['disable_track_param' => 'foobar']); + $trackingOptions = new TrackingOptions(disableTrackParam: 'foobar'); $this->redirectionBuilder = new ShortUrlRedirectionBuilder($trackingOptions); } diff --git a/module/Core/test/Visit/RequestTrackerTest.php b/module/Core/test/Visit/RequestTrackerTest.php index 144087ad..4634004f 100644 --- a/module/Core/test/Visit/RequestTrackerTest.php +++ b/module/Core/test/Visit/RequestTrackerTest.php @@ -38,10 +38,10 @@ class RequestTrackerTest extends TestCase $this->requestTracker = new RequestTracker( $this->visitsTracker->reveal(), - new TrackingOptions([ - 'disable_track_param' => 'foobar', - 'disable_tracking_from' => ['80.90.100.110', '192.168.10.0/24', '1.2.*.*'], - ]), + new TrackingOptions( + disableTrackParam: 'foobar', + disableTrackingFrom: ['80.90.100.110', '192.168.10.0/24', '1.2.*.*'], + ), ); $this->request = ServerRequestFactory::fromGlobals()->withAttribute( diff --git a/module/Core/test/Visit/VisitsTrackerTest.php b/module/Core/test/Visit/VisitsTrackerTest.php index 2bb13220..72028543 100644 --- a/module/Core/test/Visit/VisitsTrackerTest.php +++ b/module/Core/test/Visit/VisitsTrackerTest.php @@ -24,16 +24,11 @@ class VisitsTrackerTest extends TestCase private VisitsTracker $visitsTracker; private ObjectProphecy $em; private ObjectProphecy $eventDispatcher; - private TrackingOptions $options; protected function setUp(): void { $this->em = $this->prophesize(EntityManager::class); - $this->eventDispatcher = $this->prophesize(EventDispatcherInterface::class); - $this->options = new TrackingOptions(); - - $this->visitsTracker = new VisitsTracker($this->em->reveal(), $this->eventDispatcher->reveal(), $this->options); } /** @@ -45,7 +40,7 @@ class VisitsTrackerTest extends TestCase $persist = $this->em->persist(Argument::that(fn (Visit $visit) => $visit->setId('1')))->will(function (): void { }); - $this->visitsTracker->{$method}(...$args); + $this->visitsTracker()->{$method}(...$args); $persist->shouldHaveBeenCalledOnce(); $this->em->flush()->shouldHaveBeenCalledOnce(); @@ -58,9 +53,7 @@ class VisitsTrackerTest extends TestCase */ public function trackingIsSkippedCompletelyWhenDisabledFromOptions(string $method, array $args): void { - $this->options->disableTracking = true; - - $this->visitsTracker->{$method}(...$args); + $this->visitsTracker(new TrackingOptions(disableTracking: true))->{$method}(...$args); $this->eventDispatcher->dispatch(Argument::cetera())->shouldNotHaveBeenCalled(); $this->em->persist(Argument::cetera())->shouldNotHaveBeenCalled(); @@ -81,9 +74,7 @@ class VisitsTrackerTest extends TestCase */ public function orphanVisitsAreNotTrackedWhenDisabled(string $method): void { - $this->options->trackOrphanVisits = false; - - $this->visitsTracker->{$method}(Visitor::emptyInstance()); + $this->visitsTracker(new TrackingOptions(trackOrphanVisits: false))->{$method}(Visitor::emptyInstance()); $this->eventDispatcher->dispatch(Argument::cetera())->shouldNotHaveBeenCalled(); $this->em->persist(Argument::cetera())->shouldNotHaveBeenCalled(); @@ -96,4 +87,13 @@ class VisitsTrackerTest extends TestCase yield 'trackBaseUrlVisit' => ['trackBaseUrlVisit']; yield 'trackRegularNotFoundVisit' => ['trackRegularNotFoundVisit']; } + + private function visitsTracker(?TrackingOptions $options = null): VisitsTracker + { + return new VisitsTracker( + $this->em->reveal(), + $this->eventDispatcher->reveal(), + $options ?? new TrackingOptions(), + ); + } } From 96859298247fe3008df70ee58155893bcf07bce3 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Sep 2022 13:01:28 +0200 Subject: [PATCH 03/11] Migrated AppOptions to immutable object --- module/CLI/src/Factory/ApplicationFactory.php | 2 +- module/Core/config/dependencies.config.php | 3 +-- module/Core/src/Options/AppOptions.php | 27 ++----------------- .../NotifyVisitToWebHooksTest.php | 2 +- module/Rest/src/Action/HealthAction.php | 2 +- module/Rest/test/Action/HealthActionTest.php | 2 +- 6 files changed, 7 insertions(+), 31 deletions(-) diff --git a/module/CLI/src/Factory/ApplicationFactory.php b/module/CLI/src/Factory/ApplicationFactory.php index 262238a3..ab716f7e 100644 --- a/module/CLI/src/Factory/ApplicationFactory.php +++ b/module/CLI/src/Factory/ApplicationFactory.php @@ -17,7 +17,7 @@ class ApplicationFactory $appOptions = $container->get(AppOptions::class); $commands = $config['commands'] ?? []; - $app = new CliApp($appOptions->getName(), $appOptions->getVersion()); + $app = new CliApp($appOptions->name, $appOptions->version); $app->setCommandLoader(new ContainerCommandLoader($container, $commands)); return $app; diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index d6cbd72a..54a861e6 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -21,7 +21,7 @@ return [ ErrorHandler\NotFoundRedirectHandler::class => ConfigAbstractFactory::class, ErrorHandler\NotFoundTemplateHandler::class => InvokableFactory::class, - Options\AppOptions::class => ConfigAbstractFactory::class, + Options\AppOptions::class => [ValinorConfigFactory::class, 'config.app_options'], Options\DeleteShortUrlsOptions::class => ConfigAbstractFactory::class, Options\NotFoundRedirectOptions::class => ConfigAbstractFactory::class, Options\RedirectOptions::class => ConfigAbstractFactory::class, @@ -86,7 +86,6 @@ return [ Domain\DomainService::class, ], - Options\AppOptions::class => ['config.app_options'], Options\DeleteShortUrlsOptions::class => ['config.delete_short_urls'], Options\NotFoundRedirectOptions::class => ['config.not_found_redirects'], Options\RedirectOptions::class => ['config.redirects'], diff --git a/module/Core/src/Options/AppOptions.php b/module/Core/src/Options/AppOptions.php index e81f9fdb..ec545352 100644 --- a/module/Core/src/Options/AppOptions.php +++ b/module/Core/src/Options/AppOptions.php @@ -4,35 +4,12 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Options; -use Laminas\Stdlib\AbstractOptions; - use function sprintf; -class AppOptions extends AbstractOptions +final class AppOptions { - private string $name = 'Shlink'; - private string $version = '3.0.0'; - - public function getName(): string + public function __construct(public string $name = 'Shlink', public string $version = '3.0.0') { - return $this->name; - } - - protected function setName(string $name): self - { - $this->name = $name; - return $this; - } - - public function getVersion(): string - { - return $this->version; - } - - protected function setVersion(string $version): self - { - $this->version = $version; - return $this; } public function __toString(): string diff --git a/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php index 6be8719a..4234a188 100644 --- a/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php +++ b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php @@ -165,7 +165,7 @@ class NotifyVisitToWebHooksTest extends TestCase ['webhooks' => $webhooks, 'notify_orphan_visits_to_webhooks' => $notifyOrphanVisits], ), new ShortUrlDataTransformer(new ShortUrlStringifier([])), - new AppOptions(['name' => 'Shlink', 'version' => '1.2.3']), + new AppOptions('Shlink', '1.2.3'), ); } } diff --git a/module/Rest/src/Action/HealthAction.php b/module/Rest/src/Action/HealthAction.php index 462eb345..f3bfea98 100644 --- a/module/Rest/src/Action/HealthAction.php +++ b/module/Rest/src/Action/HealthAction.php @@ -42,7 +42,7 @@ class HealthAction extends AbstractRestAction $statusCode = $connected ? self::STATUS_OK : self::STATUS_SERVICE_UNAVAILABLE; return new JsonResponse([ 'status' => $connected ? self::STATUS_PASS : self::STATUS_FAIL, - 'version' => $this->options->getVersion(), + 'version' => $this->options->version, 'links' => [ 'about' => 'https://shlink.io', 'project' => 'https://github.com/shlinkio/shlink', diff --git a/module/Rest/test/Action/HealthActionTest.php b/module/Rest/test/Action/HealthActionTest.php index 8298b2d1..461152a4 100644 --- a/module/Rest/test/Action/HealthActionTest.php +++ b/module/Rest/test/Action/HealthActionTest.php @@ -36,7 +36,7 @@ class HealthActionTest extends TestCase $em = $this->prophesize(EntityManagerInterface::class); $em->getConnection()->willReturn($this->conn->reveal()); - $this->action = new HealthAction($em->reveal(), new AppOptions(['version' => '1.2.3'])); + $this->action = new HealthAction($em->reveal(), new AppOptions(version: '1.2.3')); } /** @test */ From 784908420e5318ca76728611a469d05b06903cf2 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Sep 2022 13:04:49 +0200 Subject: [PATCH 04/11] Migrated DeleteShortUrlsOptions to immutable object --- module/Core/config/dependencies.config.php | 3 +- .../src/Options/DeleteShortUrlsOptions.php | 31 +++---------------- .../ShortUrl/DeleteShortUrlService.php | 6 ++-- .../ShortUrl/DeleteShortUrlServiceTest.php | 8 ++--- 4 files changed, 13 insertions(+), 35 deletions(-) diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 54a861e6..1ce6414f 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -22,7 +22,7 @@ return [ ErrorHandler\NotFoundTemplateHandler::class => InvokableFactory::class, Options\AppOptions::class => [ValinorConfigFactory::class, 'config.app_options'], - Options\DeleteShortUrlsOptions::class => ConfigAbstractFactory::class, + Options\DeleteShortUrlsOptions::class => [ValinorConfigFactory::class, 'config.delete_short_urls'], Options\NotFoundRedirectOptions::class => ConfigAbstractFactory::class, Options\RedirectOptions::class => ConfigAbstractFactory::class, Options\UrlShortenerOptions::class => ConfigAbstractFactory::class, @@ -86,7 +86,6 @@ return [ Domain\DomainService::class, ], - Options\DeleteShortUrlsOptions::class => ['config.delete_short_urls'], Options\NotFoundRedirectOptions::class => ['config.not_found_redirects'], Options\RedirectOptions::class => ['config.redirects'], Options\UrlShortenerOptions::class => ['config.url_shortener'], diff --git a/module/Core/src/Options/DeleteShortUrlsOptions.php b/module/Core/src/Options/DeleteShortUrlsOptions.php index ff1c356a..a645181b 100644 --- a/module/Core/src/Options/DeleteShortUrlsOptions.php +++ b/module/Core/src/Options/DeleteShortUrlsOptions.php @@ -4,34 +4,13 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Options; -use Laminas\Stdlib\AbstractOptions; - use const Shlinkio\Shlink\DEFAULT_DELETE_SHORT_URL_THRESHOLD; -class DeleteShortUrlsOptions extends AbstractOptions +final class DeleteShortUrlsOptions { - private int $visitsThreshold = DEFAULT_DELETE_SHORT_URL_THRESHOLD; - private bool $checkVisitsThreshold = true; - - public function getVisitsThreshold(): int - { - return $this->visitsThreshold; - } - - protected function setVisitsThreshold(int $visitsThreshold): self - { - $this->visitsThreshold = $visitsThreshold; - return $this; - } - - public function doCheckVisitsThreshold(): bool - { - return $this->checkVisitsThreshold; - } - - protected function setCheckVisitsThreshold(bool $checkVisitsThreshold): self - { - $this->checkVisitsThreshold = $checkVisitsThreshold; - return $this; + public function __construct( + public readonly int $visitsThreshold = DEFAULT_DELETE_SHORT_URL_THRESHOLD, + public readonly bool $checkVisitsThreshold = true, + ) { } } diff --git a/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php b/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php index e6f2e82d..d4d6803f 100644 --- a/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php +++ b/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php @@ -32,7 +32,7 @@ class DeleteShortUrlService implements DeleteShortUrlServiceInterface $shortUrl = $this->urlResolver->resolveShortUrl($identifier, $apiKey); if (! $ignoreThreshold && $this->isThresholdReached($shortUrl)) { throw Exception\DeleteShortUrlException::fromVisitsThreshold( - $this->deleteShortUrlsOptions->getVisitsThreshold(), + $this->deleteShortUrlsOptions->visitsThreshold, $identifier, ); } @@ -43,10 +43,10 @@ class DeleteShortUrlService implements DeleteShortUrlServiceInterface private function isThresholdReached(ShortUrl $shortUrl): bool { - if (! $this->deleteShortUrlsOptions->doCheckVisitsThreshold()) { + if (! $this->deleteShortUrlsOptions->checkVisitsThreshold) { return false; } - return $shortUrl->getVisitsCount() >= $this->deleteShortUrlsOptions->getVisitsThreshold(); + return $shortUrl->getVisitsCount() >= $this->deleteShortUrlsOptions->visitsThreshold; } } diff --git a/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php b/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php index 391d52fd..87a6582f 100644 --- a/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php +++ b/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php @@ -102,9 +102,9 @@ class DeleteShortUrlServiceTest extends TestCase private function createService(bool $checkVisitsThreshold = true, int $visitsThreshold = 5): DeleteShortUrlService { - return new DeleteShortUrlService($this->em->reveal(), new DeleteShortUrlsOptions([ - 'visitsThreshold' => $visitsThreshold, - 'checkVisitsThreshold' => $checkVisitsThreshold, - ]), $this->urlResolver->reveal()); + return new DeleteShortUrlService($this->em->reveal(), new DeleteShortUrlsOptions( + $visitsThreshold, + $checkVisitsThreshold, + ), $this->urlResolver->reveal()); } } From 39693ca1fe178cdbefe4f7bffad6cd2e69369047 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Sep 2022 13:19:17 +0200 Subject: [PATCH 05/11] Added --thread=max to infection command --- composer.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 8a53f16c..a2a30bc8 100644 --- a/composer.json +++ b/composer.json @@ -61,7 +61,7 @@ "cebe/php-openapi": "^1.7", "devster/ubench": "^2.1", "dms/phpunit-arraysubset-asserts": "^0.4.0", - "infection/infection": "^0.26.5", + "infection/infection": "^0.26.15", "openswoole/ide-helper": "~4.11.1", "phpspec/prophecy-phpunit": "^2.0", "phpstan/phpstan": "^1.8", @@ -129,7 +129,7 @@ "test:cli": "APP_ENV=test DB_DRIVER=maria TEST_ENV=cli php vendor/bin/phpunit --order-by=random --colors=always --testdox -c phpunit-cli.xml --log-junit=build/coverage-cli/junit.xml", "test:cli:ci": "GENERATE_COVERAGE=yes composer test:cli", "test:cli:pretty": "GENERATE_COVERAGE=pretty composer test:cli", - "infect:ci:base": "infection --threads=4 --only-covered --only-covering-test-cases --skip-initial-tests", + "infect:ci:base": "infection --threads=max --only-covered --only-covering-test-cases --skip-initial-tests", "infect:ci:unit": "@infect:ci:base --coverage=build/coverage-unit --min-msi=84", "infect:ci:db": "@infect:ci:base --coverage=build/coverage-db --min-msi=95 --configuration=infection-db.json", "infect:ci:api": "@infect:ci:base --coverage=build/coverage-api --min-msi=80 --configuration=infection-api.json", From 20f457a3e96d148dbfc8d6cc6bafb5dfd1c67e62 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Sep 2022 13:32:40 +0200 Subject: [PATCH 06/11] Migrated NotFoundRedirectOptions to immutable object --- .../Command/Domain/ListDomainsCommandTest.php | 8 ++--- module/Core/config/dependencies.config.php | 3 +- .../src/Options/NotFoundRedirectOptions.php | 30 +++++-------------- .../Config/NotFoundRedirectResolverTest.php | 22 +++++++------- .../Request/DomainRedirectsRequestTest.php | 4 +-- 5 files changed, 25 insertions(+), 42 deletions(-) diff --git a/module/CLI/test/Command/Domain/ListDomainsCommandTest.php b/module/CLI/test/Command/Domain/ListDomainsCommandTest.php index adaa1d00..51da498b 100644 --- a/module/CLI/test/Command/Domain/ListDomainsCommandTest.php +++ b/module/CLI/test/Command/Domain/ListDomainsCommandTest.php @@ -43,10 +43,10 @@ class ListDomainsCommandTest extends TestCase )); $listDomains = $this->domainService->listDomains()->willReturn([ - DomainItem::forDefaultDomain('foo.com', new NotFoundRedirectOptions([ - 'base_url' => 'https://foo.com/default/base', - 'invalid_short_url' => 'https://foo.com/default/invalid', - ])), + DomainItem::forDefaultDomain('foo.com', new NotFoundRedirectOptions( + invalidShortUrl: 'https://foo.com/default/invalid', + baseUrl: 'https://foo.com/default/base', + )), DomainItem::forNonDefaultDomain(Domain::withAuthority('bar.com')), DomainItem::forNonDefaultDomain($bazDomain), ]); diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 1ce6414f..80926dc1 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -23,7 +23,7 @@ return [ Options\AppOptions::class => [ValinorConfigFactory::class, 'config.app_options'], Options\DeleteShortUrlsOptions::class => [ValinorConfigFactory::class, 'config.delete_short_urls'], - Options\NotFoundRedirectOptions::class => ConfigAbstractFactory::class, + Options\NotFoundRedirectOptions::class => [ValinorConfigFactory::class, 'config.not_found_redirects'], Options\RedirectOptions::class => ConfigAbstractFactory::class, Options\UrlShortenerOptions::class => ConfigAbstractFactory::class, Options\TrackingOptions::class => [ValinorConfigFactory::class, 'config.tracking'], @@ -86,7 +86,6 @@ return [ Domain\DomainService::class, ], - Options\NotFoundRedirectOptions::class => ['config.not_found_redirects'], Options\RedirectOptions::class => ['config.redirects'], Options\UrlShortenerOptions::class => ['config.url_shortener'], Options\QrCodeOptions::class => ['config.qr_codes'], diff --git a/module/Core/src/Options/NotFoundRedirectOptions.php b/module/Core/src/Options/NotFoundRedirectOptions.php index 2f2d813b..fe99ac7e 100644 --- a/module/Core/src/Options/NotFoundRedirectOptions.php +++ b/module/Core/src/Options/NotFoundRedirectOptions.php @@ -4,14 +4,16 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Options; -use Laminas\Stdlib\AbstractOptions; use Shlinkio\Shlink\Core\Config\NotFoundRedirectConfigInterface; -class NotFoundRedirectOptions extends AbstractOptions implements NotFoundRedirectConfigInterface +final class NotFoundRedirectOptions implements NotFoundRedirectConfigInterface { - private ?string $invalidShortUrl = null; - private ?string $regular404 = null; - private ?string $baseUrl = null; + public function __construct( + public readonly ?string $invalidShortUrl = null, + public readonly ?string $regular404 = null, + public readonly ?string $baseUrl = null, + ) { + } public function invalidShortUrlRedirect(): ?string { @@ -23,12 +25,6 @@ class NotFoundRedirectOptions extends AbstractOptions implements NotFoundRedirec return $this->invalidShortUrl !== null; } - protected function setInvalidShortUrl(?string $invalidShortUrl): self - { - $this->invalidShortUrl = $invalidShortUrl; - return $this; - } - public function regular404Redirect(): ?string { return $this->regular404; @@ -39,12 +35,6 @@ class NotFoundRedirectOptions extends AbstractOptions implements NotFoundRedirec return $this->regular404 !== null; } - protected function setRegular404(?string $regular404): self - { - $this->regular404 = $regular404; - return $this; - } - public function baseUrlRedirect(): ?string { return $this->baseUrl; @@ -54,10 +44,4 @@ class NotFoundRedirectOptions extends AbstractOptions implements NotFoundRedirec { return $this->baseUrl !== null; } - - protected function setBaseUrl(?string $baseUrl): self - { - $this->baseUrl = $baseUrl; - return $this; - } } diff --git a/module/Core/test/Config/NotFoundRedirectResolverTest.php b/module/Core/test/Config/NotFoundRedirectResolverTest.php index aa98d102..912e17a5 100644 --- a/module/Core/test/Config/NotFoundRedirectResolverTest.php +++ b/module/Core/test/Config/NotFoundRedirectResolverTest.php @@ -60,57 +60,57 @@ class NotFoundRedirectResolverTest extends TestCase yield 'base URL with trailing slash' => [ $uri = new Uri('/'), $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), - new NotFoundRedirectOptions(['baseUrl' => 'baseUrl']), + new NotFoundRedirectOptions(baseUrl: 'baseUrl'), 'baseUrl', ]; yield 'base URL with domain placeholder' => [ $uri = new Uri('https://doma.in'), $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), - new NotFoundRedirectOptions(['baseUrl' => 'https://redirect-here.com/{DOMAIN}']), + new NotFoundRedirectOptions(baseUrl: 'https://redirect-here.com/{DOMAIN}'), 'https://redirect-here.com/doma.in', ]; yield 'base URL with domain placeholder in query' => [ $uri = new Uri('https://doma.in'), $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), - new NotFoundRedirectOptions(['baseUrl' => 'https://redirect-here.com/?domain={DOMAIN}']), + new NotFoundRedirectOptions(baseUrl: 'https://redirect-here.com/?domain={DOMAIN}'), 'https://redirect-here.com/?domain=doma.in', ]; yield 'base URL without trailing slash' => [ $uri = new Uri(''), $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), - new NotFoundRedirectOptions(['baseUrl' => 'baseUrl']), + new NotFoundRedirectOptions(baseUrl: 'baseUrl'), 'baseUrl', ]; yield 'regular 404' => [ $uri = new Uri('/foo/bar'), $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), - new NotFoundRedirectOptions(['regular404' => 'regular404']), + new NotFoundRedirectOptions(regular404: 'regular404'), 'regular404', ]; yield 'regular 404 with path placeholder in query' => [ $uri = new Uri('/foo/bar'), $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), - new NotFoundRedirectOptions(['regular404' => 'https://redirect-here.com/?path={ORIGINAL_PATH}']), + new NotFoundRedirectOptions(regular404: 'https://redirect-here.com/?path={ORIGINAL_PATH}'), 'https://redirect-here.com/?path=%2Ffoo%2Fbar', ]; yield 'regular 404 with multiple placeholders' => [ $uri = new Uri('https://doma.in/foo/bar'), $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), - new NotFoundRedirectOptions([ - 'regular404' => 'https://redirect-here.com/{ORIGINAL_PATH}/{DOMAIN}/?d={DOMAIN}&p={ORIGINAL_PATH}', - ]), + new NotFoundRedirectOptions( + regular404: 'https://redirect-here.com/{ORIGINAL_PATH}/{DOMAIN}/?d={DOMAIN}&p={ORIGINAL_PATH}', + ), 'https://redirect-here.com/foo/bar/doma.in/?d=doma.in&p=%2Ffoo%2Fbar', ]; yield 'invalid short URL' => [ new Uri('/foo'), $this->notFoundType($this->requestForRoute(RedirectAction::class)), - new NotFoundRedirectOptions(['invalidShortUrl' => 'invalidShortUrl']), + new NotFoundRedirectOptions(invalidShortUrl: 'invalidShortUrl'), 'invalidShortUrl', ]; yield 'invalid short URL with path placeholder' => [ new Uri('/foo'), $this->notFoundType($this->requestForRoute(RedirectAction::class)), - new NotFoundRedirectOptions(['invalidShortUrl' => 'https://redirect-here.com/{ORIGINAL_PATH}']), + new NotFoundRedirectOptions(invalidShortUrl: 'https://redirect-here.com/{ORIGINAL_PATH}'), 'https://redirect-here.com/foo', ]; } diff --git a/module/Rest/test/Action/Domain/Request/DomainRedirectsRequestTest.php b/module/Rest/test/Action/Domain/Request/DomainRedirectsRequestTest.php index 05212fe7..51509047 100644 --- a/module/Rest/test/Action/Domain/Request/DomainRedirectsRequestTest.php +++ b/module/Rest/test/Action/Domain/Request/DomainRedirectsRequestTest.php @@ -55,7 +55,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(['regular404' => 'fallback', 'invalidShortUrl' => 'fallback2']), + new NotFoundRedirectOptions(invalidShortUrl: 'fallback2', regular404: 'fallback'), 'domain', 'bar', 'fallback', @@ -63,7 +63,7 @@ class DomainRedirectsRequestTest extends TestCase ]; yield 'fallback ignored' => [ ['domain' => 'domain', 'regular404Redirect' => 'bar', 'invalidShortUrlRedirect' => null], - new NotFoundRedirectOptions(['regular404' => 'fallback', 'invalidShortUrl' => 'fallback2']), + new NotFoundRedirectOptions(invalidShortUrl: 'fallback2', regular404: 'fallback'), 'domain', null, 'bar', From 0c34032fd386d8d2f0705219996a92e5e7b1ceb1 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Sep 2022 13:45:09 +0200 Subject: [PATCH 07/11] Migrated QrCodeOptions to immutable object --- composer.json | 2 + module/Core/config/dependencies.config.php | 3 +- module/Core/src/Action/Model/QrCodeParams.php | 10 +- module/Core/src/Options/QrCodeOptions.php | 65 ++--------- module/Core/test/Action/QrCodeActionTest.php | 105 ++++++++++-------- 5 files changed, 77 insertions(+), 108 deletions(-) diff --git a/composer.json b/composer.json index a2a30bc8..d6122753 100644 --- a/composer.json +++ b/composer.json @@ -13,6 +13,8 @@ ], "require": { "php": "^8.1", + "ext-curl": "*", + "ext-gd": "*", "ext-json": "*", "ext-pdo": "*", "akrabat/ip-address-middleware": "^2.1", diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 80926dc1..df63eeff 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -27,7 +27,7 @@ return [ Options\RedirectOptions::class => ConfigAbstractFactory::class, Options\UrlShortenerOptions::class => ConfigAbstractFactory::class, Options\TrackingOptions::class => [ValinorConfigFactory::class, 'config.tracking'], - Options\QrCodeOptions::class => ConfigAbstractFactory::class, + Options\QrCodeOptions::class => [ValinorConfigFactory::class, 'config.qr_codes'], Options\RabbitMqOptions::class => ConfigAbstractFactory::class, Options\WebhookOptions::class => ConfigAbstractFactory::class, @@ -88,7 +88,6 @@ return [ Options\RedirectOptions::class => ['config.redirects'], Options\UrlShortenerOptions::class => ['config.url_shortener'], - Options\QrCodeOptions::class => ['config.qr_codes'], Options\RabbitMqOptions::class => ['config.rabbitmq'], Options\WebhookOptions::class => ['config.visits_webhooks'], diff --git a/module/Core/src/Action/Model/QrCodeParams.php b/module/Core/src/Action/Model/QrCodeParams.php index 7c1f0e34..306c2b44 100644 --- a/module/Core/src/Action/Model/QrCodeParams.php +++ b/module/Core/src/Action/Model/QrCodeParams.php @@ -52,7 +52,7 @@ final class QrCodeParams private static function resolveSize(array $query, QrCodeOptions $defaults): int { - $size = (int) ($query['size'] ?? $defaults->size()); + $size = (int) ($query['size'] ?? $defaults->size); if ($size < self::MIN_SIZE) { return self::MIN_SIZE; } @@ -62,7 +62,7 @@ final class QrCodeParams private static function resolveMargin(array $query, QrCodeOptions $defaults): int { - $margin = $query['margin'] ?? (string) $defaults->margin(); + $margin = $query['margin'] ?? (string) $defaults->margin; $intMargin = (int) $margin; if ($margin !== (string) $intMargin) { return 0; @@ -74,7 +74,7 @@ final class QrCodeParams private static function resolveWriter(array $query, QrCodeOptions $defaults): WriterInterface { $qFormat = self::normalizeParam($query['format'] ?? ''); - $format = contains(self::SUPPORTED_FORMATS, $qFormat) ? $qFormat : self::normalizeParam($defaults->format()); + $format = contains(self::SUPPORTED_FORMATS, $qFormat) ? $qFormat : self::normalizeParam($defaults->format); return match ($format) { 'svg' => new SvgWriter(), @@ -84,7 +84,7 @@ final class QrCodeParams private static function resolveErrorCorrection(array $query, QrCodeOptions $defaults): ErrorCorrectionLevelInterface { - $errorCorrectionLevel = self::normalizeParam($query['errorCorrection'] ?? $defaults->errorCorrection()); + $errorCorrectionLevel = self::normalizeParam($query['errorCorrection'] ?? $defaults->errorCorrection); return match ($errorCorrectionLevel) { 'h' => new ErrorCorrectionLevelHigh(), 'q' => new ErrorCorrectionLevelQuartile(), @@ -97,7 +97,7 @@ final class QrCodeParams { $doNotRoundBlockSize = isset($query['roundBlockSize']) ? $query['roundBlockSize'] === 'false' - : ! $defaults->roundBlockSize(); + : ! $defaults->roundBlockSize; return $doNotRoundBlockSize ? new RoundBlockSizeModeNone() : new RoundBlockSizeModeMargin(); } diff --git a/module/Core/src/Options/QrCodeOptions.php b/module/Core/src/Options/QrCodeOptions.php index 3dfc9a53..1b10c280 100644 --- a/module/Core/src/Options/QrCodeOptions.php +++ b/module/Core/src/Options/QrCodeOptions.php @@ -4,69 +4,20 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Options; -use Laminas\Stdlib\AbstractOptions; - use const Shlinkio\Shlink\DEFAULT_QR_CODE_ERROR_CORRECTION; use const Shlinkio\Shlink\DEFAULT_QR_CODE_FORMAT; use const Shlinkio\Shlink\DEFAULT_QR_CODE_MARGIN; use const Shlinkio\Shlink\DEFAULT_QR_CODE_ROUND_BLOCK_SIZE; use const Shlinkio\Shlink\DEFAULT_QR_CODE_SIZE; -class QrCodeOptions extends AbstractOptions +final class QrCodeOptions { - private int $size = DEFAULT_QR_CODE_SIZE; - private int $margin = DEFAULT_QR_CODE_MARGIN; - private string $format = DEFAULT_QR_CODE_FORMAT; - private string $errorCorrection = DEFAULT_QR_CODE_ERROR_CORRECTION; - private bool $roundBlockSize = DEFAULT_QR_CODE_ROUND_BLOCK_SIZE; - - public function size(): int - { - return $this->size; - } - - protected function setSize(int $size): void - { - $this->size = $size; - } - - public function margin(): int - { - return $this->margin; - } - - protected function setMargin(int $margin): void - { - $this->margin = $margin; - } - - public function format(): string - { - return $this->format; - } - - protected function setFormat(string $format): void - { - $this->format = $format; - } - - public function errorCorrection(): string - { - return $this->errorCorrection; - } - - protected function setErrorCorrection(string $errorCorrection): void - { - $this->errorCorrection = $errorCorrection; - } - - public function roundBlockSize(): bool - { - return $this->roundBlockSize; - } - - protected function setRoundBlockSize(bool $roundBlockSize): void - { - $this->roundBlockSize = $roundBlockSize; + public function __construct( + public readonly int $size = DEFAULT_QR_CODE_SIZE, + public readonly int $margin = DEFAULT_QR_CODE_MARGIN, + public readonly string $format = DEFAULT_QR_CODE_FORMAT, + public readonly string $errorCorrection = DEFAULT_QR_CODE_ERROR_CORRECTION, + public readonly bool $roundBlockSize = DEFAULT_QR_CODE_ROUND_BLOCK_SIZE, + ) { } } diff --git a/module/Core/test/Action/QrCodeActionTest.php b/module/Core/test/Action/QrCodeActionTest.php index 1962fdc7..1f71975f 100644 --- a/module/Core/test/Action/QrCodeActionTest.php +++ b/module/Core/test/Action/QrCodeActionTest.php @@ -7,7 +7,6 @@ namespace ShlinkioTest\Shlink\Core\Action; use Laminas\Diactoros\Response; use Laminas\Diactoros\ServerRequest; use Laminas\Diactoros\ServerRequestFactory; -use Mezzio\Router\RouterInterface; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; @@ -35,24 +34,11 @@ class QrCodeActionTest extends TestCase private const WHITE = 0xFFFFFF; private const BLACK = 0x0; - private QrCodeAction $action; private ObjectProphecy $urlResolver; - private QrCodeOptions $options; protected function setUp(): void { - $router = $this->prophesize(RouterInterface::class); - $router->generateUri(Argument::cetera())->willReturn('/foo/bar'); - $this->urlResolver = $this->prophesize(ShortUrlResolverInterface::class); - $this->options = new QrCodeOptions(); - - $this->action = new QrCodeAction( - $this->urlResolver->reveal(), - new ShortUrlStringifier(['domain' => 'doma.in']), - new NullLogger(), - $this->options, - ); } /** @test */ @@ -65,7 +51,7 @@ class QrCodeActionTest extends TestCase $delegate = $this->prophesize(RequestHandlerInterface::class); $process = $delegate->handle(Argument::any())->willReturn(new Response()); - $this->action->process((new ServerRequest())->withAttribute('shortCode', $shortCode), $delegate->reveal()); + $this->action()->process((new ServerRequest())->withAttribute('shortCode', $shortCode), $delegate->reveal()); $process->shouldHaveBeenCalledOnce(); } @@ -79,7 +65,7 @@ class QrCodeActionTest extends TestCase ->shouldBeCalledOnce(); $delegate = $this->prophesize(RequestHandlerInterface::class); - $resp = $this->action->process( + $resp = $this->action()->process( (new ServerRequest())->withAttribute('shortCode', $shortCode), $delegate->reveal(), ); @@ -98,7 +84,6 @@ class QrCodeActionTest extends TestCase array $query, string $expectedContentType, ): void { - $this->options->setFromArray(['format' => $defaultFormat]); $code = 'abc123'; $this->urlResolver->resolveEnabledShortUrl(ShortUrlIdentifier::fromShortCodeAndDomain($code, ''))->willReturn( ShortUrl::createEmpty(), @@ -106,7 +91,7 @@ class QrCodeActionTest extends TestCase $delegate = $this->prophesize(RequestHandlerInterface::class); $req = (new ServerRequest())->withAttribute('shortCode', $code)->withQueryParams($query); - $resp = $this->action->process($req, $delegate->reveal()); + $resp = $this->action(new QrCodeOptions(format: $defaultFormat))->process($req, $delegate->reveal()); self::assertEquals($expectedContentType, $resp->getHeaderLine('Content-Type')); } @@ -128,18 +113,17 @@ class QrCodeActionTest extends TestCase * @dataProvider provideRequestsWithSize */ public function imageIsReturnedWithExpectedSize( - array $defaults, + QrCodeOptions $defaultOptions, ServerRequestInterface $req, int $expectedSize, ): void { - $this->options->setFromArray($defaults); $code = 'abc123'; $this->urlResolver->resolveEnabledShortUrl(ShortUrlIdentifier::fromShortCodeAndDomain($code, ''))->willReturn( ShortUrl::createEmpty(), ); $delegate = $this->prophesize(RequestHandlerInterface::class); - $resp = $this->action->process($req->withAttribute('shortCode', $code), $delegate->reveal()); + $resp = $this->action($defaultOptions)->process($req->withAttribute('shortCode', $code), $delegate->reveal()); [$size] = getimagesizefromstring($resp->getBody()->__toString()); self::assertEquals($expectedSize, $size); @@ -148,52 +132,64 @@ class QrCodeActionTest extends TestCase public function provideRequestsWithSize(): iterable { yield 'different margin and size defaults' => [ - ['size' => 660, 'margin' => 40], + new QrCodeOptions(size: 660, margin: 40), ServerRequestFactory::fromGlobals(), 740, ]; - yield 'no size' => [[], ServerRequestFactory::fromGlobals(), 300]; - yield 'no size, different default' => [['size' => 500], ServerRequestFactory::fromGlobals(), 500]; - yield 'size in query' => [[], ServerRequestFactory::fromGlobals()->withQueryParams(['size' => '123']), 123]; + yield 'no size' => [new QrCodeOptions(), ServerRequestFactory::fromGlobals(), 300]; + yield 'no size, different default' => [new QrCodeOptions(size: 500), ServerRequestFactory::fromGlobals(), 500]; + yield 'size in query' => [ + new QrCodeOptions(), + ServerRequestFactory::fromGlobals()->withQueryParams(['size' => '123']), + 123, + ]; yield 'size in query, default margin' => [ - ['margin' => 25], + new QrCodeOptions(margin: 25), ServerRequestFactory::fromGlobals()->withQueryParams(['size' => '123']), 173, ]; - yield 'margin' => [[], ServerRequestFactory::fromGlobals()->withQueryParams(['margin' => '35']), 370]; + yield 'margin' => [ + new QrCodeOptions(), + ServerRequestFactory::fromGlobals()->withQueryParams(['margin' => '35']), + 370, + ]; yield 'margin and different default' => [ - ['size' => 400], + new QrCodeOptions(size: 400), ServerRequestFactory::fromGlobals()->withQueryParams(['margin' => '35']), 470, ]; yield 'margin and size' => [ - [], + new QrCodeOptions(), ServerRequestFactory::fromGlobals()->withQueryParams(['margin' => '100', 'size' => '200']), 400, ]; - yield 'negative margin' => [[], ServerRequestFactory::fromGlobals()->withQueryParams(['margin' => '-50']), 300]; + yield 'negative margin' => [ + new QrCodeOptions(), + ServerRequestFactory::fromGlobals()->withQueryParams(['margin' => '-50']), + 300, + ]; yield 'negative margin, default margin' => [ - ['margin' => 10], + new QrCodeOptions(margin: 10), ServerRequestFactory::fromGlobals()->withQueryParams(['margin' => '-50']), 300, ]; yield 'non-numeric margin' => [ - [], + new QrCodeOptions(), ServerRequestFactory::fromGlobals()->withQueryParams(['margin' => 'foo']), 300, ]; yield 'negative margin and size' => [ - [], + new QrCodeOptions(), ServerRequestFactory::fromGlobals()->withQueryParams(['margin' => '-1', 'size' => '150']), 150, ]; yield 'negative margin and size, default margin' => [ - ['margin' => 5], + new QrCodeOptions(margin: 5), ServerRequestFactory::fromGlobals()->withQueryParams(['margin' => '-1', 'size' => '150']), 150, ]; yield 'non-numeric margin and size' => [ - [], + new QrCodeOptions(), ServerRequestFactory::fromGlobals()->withQueryParams(['margin' => 'foo', 'size' => '538']), 538, ]; @@ -204,11 +200,10 @@ class QrCodeActionTest extends TestCase * @dataProvider provideRoundBlockSize */ public function imageCanRemoveExtraMarginWhenBlockRoundIsDisabled( - array $defaults, + QrCodeOptions $defaultOptions, ?string $roundBlockSize, int $expectedColor, ): void { - $this->options->setFromArray($defaults); $code = 'abc123'; $req = ServerRequestFactory::fromGlobals() ->withQueryParams(['size' => 250, 'roundBlockSize' => $roundBlockSize]) @@ -219,7 +214,7 @@ class QrCodeActionTest extends TestCase ); $delegate = $this->prophesize(RequestHandlerInterface::class); - $resp = $this->action->process($req, $delegate->reveal()); + $resp = $this->action($defaultOptions)->process($req, $delegate->reveal()); $image = imagecreatefromstring($resp->getBody()->__toString()); $color = imagecolorat($image, 1, 1); @@ -228,11 +223,33 @@ class QrCodeActionTest extends TestCase public function provideRoundBlockSize(): iterable { - yield 'no round block param' => [[], null, self::WHITE]; - yield 'no round block param, but disabled by default' => [['round_block_size' => false], null, self::BLACK]; - yield 'round block: "true"' => [[], 'true', self::WHITE]; - yield 'round block: "true", but disabled by default' => [['round_block_size' => false], 'true', self::WHITE]; - yield 'round block: "false"' => [[], 'false', self::BLACK]; - yield 'round block: "false", but enabled by default' => [['round_block_size' => true], 'false', self::BLACK]; + yield 'no round block param' => [new QrCodeOptions(), null, self::WHITE]; + yield 'no round block param, but disabled by default' => [ + new QrCodeOptions(roundBlockSize: false), + null, + self::BLACK, + ]; + yield 'round block: "true"' => [new QrCodeOptions(), 'true', self::WHITE]; + yield 'round block: "true", but disabled by default' => [ + new QrCodeOptions(roundBlockSize: false), + 'true', + self::WHITE, + ]; + yield 'round block: "false"' => [new QrCodeOptions(), 'false', self::BLACK]; + yield 'round block: "false", but enabled by default' => [ + new QrCodeOptions(roundBlockSize: true), + 'false', + self::BLACK, + ]; + } + + public function action(?QrCodeOptions $options = null): QrCodeAction + { + return new QrCodeAction( + $this->urlResolver->reveal(), + new ShortUrlStringifier(['domain' => 'doma.in']), + new NullLogger(), + $options ?? new QrCodeOptions(), + ); } } From 8f680788358781a08b7a6afd698666e33eadaa06 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Sep 2022 13:56:59 +0200 Subject: [PATCH 08/11] Migrated RabbitMqOptions to immutable object --- module/Core/config/dependencies.config.php | 3 +- .../RabbitMq/NotifyNewShortUrlToRabbitMq.php | 2 +- .../RabbitMq/NotifyVisitToRabbitMq.php | 4 +- module/Core/src/Options/RabbitMqOptions.php | 37 +++--------------- .../NotifyNewShortUrlToRabbitMqTest.php | 32 ++++++++-------- .../RabbitMq/NotifyVisitToRabbitMqTest.php | 38 +++++++++---------- 6 files changed, 42 insertions(+), 74 deletions(-) diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index df63eeff..8c06001e 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -28,7 +28,7 @@ return [ Options\UrlShortenerOptions::class => ConfigAbstractFactory::class, Options\TrackingOptions::class => [ValinorConfigFactory::class, 'config.tracking'], Options\QrCodeOptions::class => [ValinorConfigFactory::class, 'config.qr_codes'], - Options\RabbitMqOptions::class => ConfigAbstractFactory::class, + Options\RabbitMqOptions::class => [ValinorConfigFactory::class, 'config.rabbitmq'], Options\WebhookOptions::class => ConfigAbstractFactory::class, Service\UrlShortener::class => ConfigAbstractFactory::class, @@ -88,7 +88,6 @@ return [ Options\RedirectOptions::class => ['config.redirects'], Options\UrlShortenerOptions::class => ['config.url_shortener'], - Options\RabbitMqOptions::class => ['config.rabbitmq'], Options\WebhookOptions::class => ['config.visits_webhooks'], Service\UrlShortener::class => [ diff --git a/module/Core/src/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMq.php b/module/Core/src/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMq.php index 488247d7..daa7cafb 100644 --- a/module/Core/src/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMq.php +++ b/module/Core/src/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMq.php @@ -26,7 +26,7 @@ class NotifyNewShortUrlToRabbitMq extends AbstractNotifyNewShortUrlListener protected function isEnabled(): bool { - return $this->options->isEnabled(); + return $this->options->enabled; } protected function getRemoteSystem(): RemoteSystem diff --git a/module/Core/src/EventDispatcher/RabbitMq/NotifyVisitToRabbitMq.php b/module/Core/src/EventDispatcher/RabbitMq/NotifyVisitToRabbitMq.php index 0faa795c..989de0a5 100644 --- a/module/Core/src/EventDispatcher/RabbitMq/NotifyVisitToRabbitMq.php +++ b/module/Core/src/EventDispatcher/RabbitMq/NotifyVisitToRabbitMq.php @@ -35,7 +35,7 @@ class NotifyVisitToRabbitMq extends AbstractNotifyVisitListener protected function determineUpdatesForVisit(Visit $visit): array { // Once the two deprecated cases below have been removed, make parent method private - if (! $this->options->legacyVisitsPublishing()) { + if (! $this->options->legacyVisitsPublishing) { return parent::determineUpdatesForVisit($visit); } @@ -61,7 +61,7 @@ class NotifyVisitToRabbitMq extends AbstractNotifyVisitListener protected function isEnabled(): bool { - return $this->options->isEnabled(); + return $this->options->enabled; } protected function getRemoteSystem(): RemoteSystem diff --git a/module/Core/src/Options/RabbitMqOptions.php b/module/Core/src/Options/RabbitMqOptions.php index 388cd2ea..cc25f3bf 100644 --- a/module/Core/src/Options/RabbitMqOptions.php +++ b/module/Core/src/Options/RabbitMqOptions.php @@ -4,37 +4,12 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Options; -use Laminas\Stdlib\AbstractOptions; - -class RabbitMqOptions extends AbstractOptions +final class RabbitMqOptions { - protected $__strictMode__ = false; // phpcs:ignore - - private bool $enabled = false; - /** @deprecated */ - private bool $legacyVisitsPublishing = false; - - public function isEnabled(): bool - { - return $this->enabled; - } - - protected function setEnabled(bool $enabled): self - { - $this->enabled = $enabled; - return $this; - } - - /** @deprecated */ - public function legacyVisitsPublishing(): bool - { - return $this->legacyVisitsPublishing; - } - - /** @deprecated */ - protected function setLegacyVisitsPublishing(bool $legacyVisitsPublishing): self - { - $this->legacyVisitsPublishing = $legacyVisitsPublishing; - return $this; + public function __construct( + public readonly bool $enabled = false, + /** @deprecated */ + public readonly bool $legacyVisitsPublishing = false, + ) { } } diff --git a/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php b/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php index 9cf44977..5365fe0e 100644 --- a/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php +++ b/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php @@ -27,12 +27,10 @@ class NotifyNewShortUrlToRabbitMqTest extends TestCase { use ProphecyTrait; - private NotifyNewShortUrlToRabbitMq $listener; private ObjectProphecy $helper; private ObjectProphecy $updatesGenerator; private ObjectProphecy $em; private ObjectProphecy $logger; - private RabbitMqOptions $options; protected function setUp(): void { @@ -40,23 +38,12 @@ class NotifyNewShortUrlToRabbitMqTest extends TestCase $this->updatesGenerator = $this->prophesize(PublishingUpdatesGeneratorInterface::class); $this->em = $this->prophesize(EntityManagerInterface::class); $this->logger = $this->prophesize(LoggerInterface::class); - $this->options = new RabbitMqOptions(['enabled' => true]); - - $this->listener = new NotifyNewShortUrlToRabbitMq( - $this->helper->reveal(), - $this->updatesGenerator->reveal(), - $this->em->reveal(), - $this->logger->reveal(), - $this->options, - ); } /** @test */ public function doesNothingWhenTheFeatureIsNotEnabled(): void { - $this->options->enabled = false; - - ($this->listener)(new ShortUrlCreated('123')); + ($this->listener(false))(new ShortUrlCreated('123')); $this->em->find(Argument::cetera())->shouldNotHaveBeenCalled(); $this->logger->warning(Argument::cetera())->shouldNotHaveBeenCalled(); @@ -74,7 +61,7 @@ class NotifyNewShortUrlToRabbitMqTest extends TestCase ['shortUrlId' => $shortUrlId, 'name' => 'RabbitMQ'], ); - ($this->listener)(new ShortUrlCreated($shortUrlId)); + ($this->listener())(new ShortUrlCreated($shortUrlId)); $find->shouldHaveBeenCalledOnce(); $logWarning->shouldHaveBeenCalledOnce(); @@ -92,7 +79,7 @@ class NotifyNewShortUrlToRabbitMqTest extends TestCase $update, ); - ($this->listener)(new ShortUrlCreated($shortUrlId)); + ($this->listener())(new ShortUrlCreated($shortUrlId)); $find->shouldHaveBeenCalledOnce(); $generateUpdate->shouldHaveBeenCalledOnce(); @@ -114,7 +101,7 @@ class NotifyNewShortUrlToRabbitMqTest extends TestCase ); $publish = $this->helper->publishUpdate($update)->willThrow($e); - ($this->listener)(new ShortUrlCreated($shortUrlId)); + ($this->listener())(new ShortUrlCreated($shortUrlId)); $this->logger->debug( 'Error while trying to notify {name} with new short URL. {e}', @@ -131,4 +118,15 @@ class NotifyNewShortUrlToRabbitMqTest extends TestCase yield [new Exception('Exception Error')]; yield [new DomainException('DomainException Error')]; } + + private function listener(bool $enabled = true): NotifyNewShortUrlToRabbitMq + { + return new NotifyNewShortUrlToRabbitMq( + $this->helper->reveal(), + $this->updatesGenerator->reveal(), + $this->em->reveal(), + $this->logger->reveal(), + new RabbitMqOptions($enabled), + ); + } } diff --git a/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php b/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php index 05ee7568..59f9c26a 100644 --- a/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php +++ b/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php @@ -35,12 +35,10 @@ class NotifyVisitToRabbitMqTest extends TestCase { use ProphecyTrait; - private NotifyVisitToRabbitMq $listener; private ObjectProphecy $helper; private ObjectProphecy $updatesGenerator; private ObjectProphecy $em; private ObjectProphecy $logger; - private RabbitMqOptions $options; protected function setUp(): void { @@ -48,24 +46,12 @@ class NotifyVisitToRabbitMqTest extends TestCase $this->updatesGenerator = $this->prophesize(PublishingUpdatesGeneratorInterface::class); $this->em = $this->prophesize(EntityManagerInterface::class); $this->logger = $this->prophesize(LoggerInterface::class); - $this->options = new RabbitMqOptions(['enabled' => true, 'legacy_visits_publishing' => false]); - - $this->listener = new NotifyVisitToRabbitMq( - $this->helper->reveal(), - $this->updatesGenerator->reveal(), - $this->em->reveal(), - $this->logger->reveal(), - new OrphanVisitDataTransformer(), - $this->options, - ); } /** @test */ public function doesNothingWhenTheFeatureIsNotEnabled(): void { - $this->options->enabled = false; - - ($this->listener)(new VisitLocated('123')); + ($this->listener(new RabbitMqOptions(enabled: false)))(new VisitLocated('123')); $this->em->find(Argument::cetera())->shouldNotHaveBeenCalled(); $this->logger->warning(Argument::cetera())->shouldNotHaveBeenCalled(); @@ -83,7 +69,7 @@ class NotifyVisitToRabbitMqTest extends TestCase ['visitId' => $visitId, 'name' => 'RabbitMQ'], ); - ($this->listener)(new VisitLocated($visitId)); + ($this->listener())(new VisitLocated($visitId)); $findVisit->shouldHaveBeenCalledOnce(); $logWarning->shouldHaveBeenCalledOnce(); @@ -105,7 +91,7 @@ class NotifyVisitToRabbitMqTest extends TestCase )->shouldBeCalledOnce(); }); - ($this->listener)(new VisitLocated($visitId)); + ($this->listener())(new VisitLocated($visitId)); $findVisit->shouldHaveBeenCalledOnce(); $this->helper->publishUpdate(Argument::type(Update::class))->shouldHaveBeenCalledTimes( @@ -144,7 +130,7 @@ class NotifyVisitToRabbitMqTest extends TestCase ); $publish = $this->helper->publishUpdate(Argument::cetera())->willThrow($e); - ($this->listener)(new VisitLocated($visitId)); + ($this->listener())(new VisitLocated($visitId)); $this->logger->debug( 'Error while trying to notify {name} with new visit. {e}', @@ -172,13 +158,11 @@ class NotifyVisitToRabbitMqTest extends TestCase callable $assert, callable $setup, ): void { - $this->options->legacyVisitsPublishing = $legacy; - $visitId = '123'; $findVisit = $this->em->find(Visit::class, $visitId)->willReturn($visit); $setup($this->updatesGenerator); - ($this->listener)(new VisitLocated($visitId)); + ($this->listener(new RabbitMqOptions(true, $legacy)))(new VisitLocated($visitId)); $findVisit->shouldHaveBeenCalledOnce(); $assert($this->helper, $this->updatesGenerator); @@ -247,4 +231,16 @@ class NotifyVisitToRabbitMqTest extends TestCase }, ]; } + + private function listener(?RabbitMqOptions $options = null): NotifyVisitToRabbitMq + { + return new NotifyVisitToRabbitMq( + $this->helper->reveal(), + $this->updatesGenerator->reveal(), + $this->em->reveal(), + $this->logger->reveal(), + new OrphanVisitDataTransformer(), + $options ?? new RabbitMqOptions(enabled: true, legacyVisitsPublishing: false), + ); + } } From 42af057316cef4e85e2d157b4d65f7da1076a664 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Sep 2022 15:36:40 +0200 Subject: [PATCH 09/11] Migrated RedirectOptions to immutable object --- module/Core/config/dependencies.config.php | 3 +- module/Core/src/Options/RedirectOptions.php | 37 +++++-------------- .../Core/src/Util/RedirectResponseHelper.php | 4 +- .../test/Util/RedirectResponseHelperTest.php | 19 ++++------ 4 files changed, 20 insertions(+), 43 deletions(-) diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 8c06001e..79192c16 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -24,7 +24,7 @@ return [ Options\AppOptions::class => [ValinorConfigFactory::class, 'config.app_options'], Options\DeleteShortUrlsOptions::class => [ValinorConfigFactory::class, 'config.delete_short_urls'], Options\NotFoundRedirectOptions::class => [ValinorConfigFactory::class, 'config.not_found_redirects'], - Options\RedirectOptions::class => ConfigAbstractFactory::class, + Options\RedirectOptions::class => [ValinorConfigFactory::class, 'config.redirects'], Options\UrlShortenerOptions::class => ConfigAbstractFactory::class, Options\TrackingOptions::class => [ValinorConfigFactory::class, 'config.tracking'], Options\QrCodeOptions::class => [ValinorConfigFactory::class, 'config.qr_codes'], @@ -86,7 +86,6 @@ return [ Domain\DomainService::class, ], - Options\RedirectOptions::class => ['config.redirects'], Options\UrlShortenerOptions::class => ['config.url_shortener'], Options\WebhookOptions::class => ['config.visits_webhooks'], diff --git a/module/Core/src/Options/RedirectOptions.php b/module/Core/src/Options/RedirectOptions.php index 5479c59b..9a1fedac 100644 --- a/module/Core/src/Options/RedirectOptions.php +++ b/module/Core/src/Options/RedirectOptions.php @@ -4,40 +4,23 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Options; -use Laminas\Stdlib\AbstractOptions; - use function Functional\contains; use const Shlinkio\Shlink\DEFAULT_REDIRECT_CACHE_LIFETIME; use const Shlinkio\Shlink\DEFAULT_REDIRECT_STATUS_CODE; -class RedirectOptions extends AbstractOptions +final class RedirectOptions { - private int $redirectStatusCode = DEFAULT_REDIRECT_STATUS_CODE; - private int $redirectCacheLifetime = DEFAULT_REDIRECT_CACHE_LIFETIME; + public readonly int $redirectStatusCode; + public readonly int $redirectCacheLifetime; - public function redirectStatusCode(): int - { - return $this->redirectStatusCode; - } - - protected function setRedirectStatusCode(int $redirectStatusCode): void - { - $this->redirectStatusCode = $this->normalizeRedirectStatusCode($redirectStatusCode); - } - - private function normalizeRedirectStatusCode(int $statusCode): int - { - return contains([301, 302], $statusCode) ? $statusCode : DEFAULT_REDIRECT_STATUS_CODE; - } - - public function redirectCacheLifetime(): int - { - return $this->redirectCacheLifetime; - } - - protected function setRedirectCacheLifetime(int $redirectCacheLifetime): void - { + public function __construct( + int $redirectStatusCode = DEFAULT_REDIRECT_STATUS_CODE, + int $redirectCacheLifetime = DEFAULT_REDIRECT_CACHE_LIFETIME, + ) { + $this->redirectStatusCode = contains([301, 302], $redirectStatusCode) + ? $redirectStatusCode + : DEFAULT_REDIRECT_STATUS_CODE; $this->redirectCacheLifetime = $redirectCacheLifetime > 0 ? $redirectCacheLifetime : DEFAULT_REDIRECT_CACHE_LIFETIME; diff --git a/module/Core/src/Util/RedirectResponseHelper.php b/module/Core/src/Util/RedirectResponseHelper.php index 312c2a95..dfc87480 100644 --- a/module/Core/src/Util/RedirectResponseHelper.php +++ b/module/Core/src/Util/RedirectResponseHelper.php @@ -19,9 +19,9 @@ class RedirectResponseHelper implements RedirectResponseHelperInterface public function buildRedirectResponse(string $location): ResponseInterface { - $statusCode = $this->options->redirectStatusCode(); + $statusCode = $this->options->redirectStatusCode; $headers = $statusCode === StatusCodeInterface::STATUS_FOUND ? [] : [ - 'Cache-Control' => sprintf('private,max-age=%s', $this->options->redirectCacheLifetime()), + 'Cache-Control' => sprintf('private,max-age=%s', $this->options->redirectCacheLifetime), ]; return new RedirectResponse($location, $statusCode, $headers); diff --git a/module/Core/test/Util/RedirectResponseHelperTest.php b/module/Core/test/Util/RedirectResponseHelperTest.php index 651d4bc7..fc2b89a2 100644 --- a/module/Core/test/Util/RedirectResponseHelperTest.php +++ b/module/Core/test/Util/RedirectResponseHelperTest.php @@ -11,15 +11,6 @@ use Shlinkio\Shlink\Core\Util\RedirectResponseHelper; class RedirectResponseHelperTest extends TestCase { - private RedirectResponseHelper $helper; - private RedirectOptions $shortenerOpts; - - protected function setUp(): void - { - $this->shortenerOpts = new RedirectOptions(); - $this->helper = new RedirectResponseHelper($this->shortenerOpts); - } - /** * @test * @dataProvider provideRedirectConfigs @@ -30,10 +21,9 @@ class RedirectResponseHelperTest extends TestCase int $expectedStatus, ?string $expectedCacheControl, ): void { - $this->shortenerOpts->redirectStatusCode = $configuredStatus; - $this->shortenerOpts->redirectCacheLifetime = $configuredLifetime; + $options = new RedirectOptions($configuredStatus, $configuredLifetime); - $response = $this->helper->buildRedirectResponse('destination'); + $response = $this->helper($options)->buildRedirectResponse('destination'); self::assertInstanceOf(RedirectResponse::class, $response); self::assertEquals($expectedStatus, $response->getStatusCode()); @@ -52,4 +42,9 @@ class RedirectResponseHelperTest extends TestCase yield 'status 301 with zero expiration' => [301, 0, 301, 'private,max-age=30']; yield 'status 301 with negative expiration' => [301, -20, 301, 'private,max-age=30']; } + + private function helper(?RedirectOptions $options = null): RedirectResponseHelper + { + return new RedirectResponseHelper($options ?? new RedirectOptions()); + } } From 8d244c8d340da64c8c8f5870fa278ef30e84f603 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Sep 2022 15:54:43 +0200 Subject: [PATCH 10/11] Migrated UrlShortenerOptions to immutable object --- config/test/test_config.global.php | 1 - .../ShortUrl/CreateShortUrlCommand.php | 6 +- .../ShortUrl/CreateShortUrlCommandTest.php | 2 +- module/Core/config/dependencies.config.php | 3 +- .../Core/src/Options/UrlShortenerOptions.php | 70 +++---------------- .../ExtraPathRedirectMiddleware.php | 8 +-- module/Core/src/Util/UrlValidator.php | 4 +- .../ExtraPathRedirectMiddlewareTest.php | 39 ++++++----- module/Core/test/Util/UrlValidatorTest.php | 36 +++++----- .../Action/ShortUrl/CreateShortUrlAction.php | 2 +- 10 files changed, 58 insertions(+), 113 deletions(-) diff --git a/config/test/test_config.global.php b/config/test/test_config.global.php index 9b338d7a..678e1b05 100644 --- a/config/test/test_config.global.php +++ b/config/test/test_config.global.php @@ -130,7 +130,6 @@ return [ 'schema' => 'http', 'hostname' => 'doma.in', ], - 'validate_url' => true, ], 'mezzio-swoole' => [ diff --git a/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php index 6b4cce1a..666dea5b 100644 --- a/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/CreateShortUrlCommand.php @@ -40,7 +40,7 @@ class CreateShortUrlCommand extends Command private readonly UrlShortenerOptions $options, ) { parent::__construct(); - $this->defaultDomain = $this->options->domain()['hostname'] ?? ''; + $this->defaultDomain = $this->options->domain['hostname'] ?? ''; } protected function configure(): void @@ -158,7 +158,7 @@ class CreateShortUrlCommand extends Command $tags = unique(flatten(array_map($explodeWithComma, $input->getOption('tags')))); $customSlug = $input->getOption('custom-slug'); $maxVisits = $input->getOption('max-visits'); - $shortCodeLength = $input->getOption('short-code-length') ?? $this->options->defaultShortCodesLength(); + $shortCodeLength = $input->getOption('short-code-length') ?? $this->options->defaultShortCodesLength; $doValidateUrl = $input->getOption('validate-url'); try { @@ -175,7 +175,7 @@ class CreateShortUrlCommand extends Command ShortUrlInputFilter::TAGS => $tags, ShortUrlInputFilter::CRAWLABLE => $input->getOption('crawlable'), ShortUrlInputFilter::FORWARD_QUERY => !$input->getOption('no-forward-query'), - EnvVars::MULTI_SEGMENT_SLUGS_ENABLED->value => $this->options->multiSegmentSlugsEnabled(), + EnvVars::MULTI_SEGMENT_SLUGS_ENABLED->value => $this->options->multiSegmentSlugsEnabled, ])); $io->writeln([ diff --git a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php index e529f0ad..733f6b72 100644 --- a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php @@ -39,7 +39,7 @@ class CreateShortUrlCommandTest extends TestCase $command = new CreateShortUrlCommand( $this->urlShortener->reveal(), $this->stringifier->reveal(), - new UrlShortenerOptions(['defaultShortCodesLength' => 5, 'domain' => ['hostname' => self::DEFAULT_DOMAIN]]), + new UrlShortenerOptions(domain: ['hostname' => self::DEFAULT_DOMAIN], defaultShortCodesLength: 5), ); $this->commandTester = $this->testerForCommand($command); } diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 79192c16..9855e2aa 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -25,7 +25,7 @@ return [ Options\DeleteShortUrlsOptions::class => [ValinorConfigFactory::class, 'config.delete_short_urls'], Options\NotFoundRedirectOptions::class => [ValinorConfigFactory::class, 'config.not_found_redirects'], Options\RedirectOptions::class => [ValinorConfigFactory::class, 'config.redirects'], - Options\UrlShortenerOptions::class => ConfigAbstractFactory::class, + Options\UrlShortenerOptions::class => [ValinorConfigFactory::class, 'config.url_shortener'], Options\TrackingOptions::class => [ValinorConfigFactory::class, 'config.tracking'], Options\QrCodeOptions::class => [ValinorConfigFactory::class, 'config.qr_codes'], Options\RabbitMqOptions::class => [ValinorConfigFactory::class, 'config.rabbitmq'], @@ -86,7 +86,6 @@ return [ Domain\DomainService::class, ], - Options\UrlShortenerOptions::class => ['config.url_shortener'], Options\WebhookOptions::class => ['config.visits_webhooks'], Service\UrlShortener::class => [ diff --git a/module/Core/src/Options/UrlShortenerOptions.php b/module/Core/src/Options/UrlShortenerOptions.php index 38e185c2..dd7fdc8d 100644 --- a/module/Core/src/Options/UrlShortenerOptions.php +++ b/module/Core/src/Options/UrlShortenerOptions.php @@ -4,69 +4,17 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Options; -use Laminas\Stdlib\AbstractOptions; - use const Shlinkio\Shlink\DEFAULT_SHORT_CODES_LENGTH; -class UrlShortenerOptions extends AbstractOptions +final class UrlShortenerOptions { - protected $__strictMode__ = false; // phpcs:ignore - - private array $domain = []; - private int $defaultShortCodesLength = DEFAULT_SHORT_CODES_LENGTH; - private bool $autoResolveTitles = false; - private bool $appendExtraPath = false; - private bool $multiSegmentSlugsEnabled = false; - - public function domain(): array - { - return $this->domain; - } - - protected function setDomain(array $domain): self - { - $this->domain = $domain; - return $this; - } - - public function defaultShortCodesLength(): int - { - return $this->defaultShortCodesLength; - } - - protected function setDefaultShortCodesLength(int $defaultShortCodesLength): self - { - $this->defaultShortCodesLength = $defaultShortCodesLength; - return $this; - } - - public function autoResolveTitles(): bool - { - return $this->autoResolveTitles; - } - - protected function setAutoResolveTitles(bool $autoResolveTitles): void - { - $this->autoResolveTitles = $autoResolveTitles; - } - - public function appendExtraPath(): bool - { - return $this->appendExtraPath; - } - - protected function setAppendExtraPath(bool $appendExtraPath): void - { - $this->appendExtraPath = $appendExtraPath; - } - - public function multiSegmentSlugsEnabled(): bool - { - return $this->multiSegmentSlugsEnabled; - } - - protected function setMultiSegmentSlugsEnabled(bool $multiSegmentSlugsEnabled): void - { - $this->multiSegmentSlugsEnabled = $multiSegmentSlugsEnabled; + public function __construct( + /** @var array{schema: ?string, hostname: ?string} */ + public readonly array $domain = ['schema' => null, 'hostname' => null], + public readonly int $defaultShortCodesLength = DEFAULT_SHORT_CODES_LENGTH, + public readonly bool $autoResolveTitles = false, + public readonly bool $appendExtraPath = false, + public readonly bool $multiSegmentSlugsEnabled = false, + ) { } } diff --git a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php index bb350aa2..3fead5f2 100644 --- a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php +++ b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php @@ -49,16 +49,16 @@ class ExtraPathRedirectMiddleware implements MiddlewareInterface private function shouldApplyLogic(?NotFoundType $notFoundType): bool { - if ($notFoundType === null || ! $this->urlShortenerOptions->appendExtraPath()) { + if ($notFoundType === null || ! $this->urlShortenerOptions->appendExtraPath) { return false; } return ( // If multi-segment slugs are enabled, the appropriate not-found type is "invalid_short_url" - $this->urlShortenerOptions->multiSegmentSlugsEnabled() && $notFoundType->isInvalidShortUrl() + $this->urlShortenerOptions->multiSegmentSlugsEnabled && $notFoundType->isInvalidShortUrl() ) || ( // If multi-segment slugs are disabled, the appropriate not-found type is "regular_404" - ! $this->urlShortenerOptions->multiSegmentSlugsEnabled() && $notFoundType->isRegularNotFound() + ! $this->urlShortenerOptions->multiSegmentSlugsEnabled && $notFoundType->isRegularNotFound() ); } @@ -79,7 +79,7 @@ class ExtraPathRedirectMiddleware implements MiddlewareInterface $longUrl = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $query, $extraPath); return $this->redirectResponseHelper->buildRedirectResponse($longUrl); } catch (ShortUrlNotFoundException) { - if ($extraPath === null || ! $this->urlShortenerOptions->multiSegmentSlugsEnabled()) { + if ($extraPath === null || ! $this->urlShortenerOptions->multiSegmentSlugsEnabled) { return $handler->handle($request); } diff --git a/module/Core/src/Util/UrlValidator.php b/module/Core/src/Util/UrlValidator.php index 2e2965b1..0057660a 100644 --- a/module/Core/src/Util/UrlValidator.php +++ b/module/Core/src/Util/UrlValidator.php @@ -46,11 +46,11 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface public function validateUrlWithTitle(string $url, bool $doValidate): ?string { - if (! $doValidate && ! $this->options->autoResolveTitles()) { + if (! $doValidate && ! $this->options->autoResolveTitles) { return null; } - if (! $this->options->autoResolveTitles()) { + if (! $this->options->autoResolveTitles) { $this->validateUrlAndGetResponse($url, self::METHOD_HEAD); return null; } diff --git a/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php b/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php index 4099faea..3cd2adef 100644 --- a/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php +++ b/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php @@ -34,12 +34,10 @@ class ExtraPathRedirectMiddlewareTest extends TestCase { use ProphecyTrait; - private ExtraPathRedirectMiddleware $middleware; private ObjectProphecy $resolver; private ObjectProphecy $requestTracker; private ObjectProphecy $redirectionBuilder; private ObjectProphecy $redirectResponseHelper; - private UrlShortenerOptions $options; private ObjectProphecy $handler; protected function setUp(): void @@ -48,16 +46,6 @@ class ExtraPathRedirectMiddlewareTest extends TestCase $this->requestTracker = $this->prophesize(RequestTrackerInterface::class); $this->redirectionBuilder = $this->prophesize(ShortUrlRedirectionBuilderInterface::class); $this->redirectResponseHelper = $this->prophesize(RedirectResponseHelperInterface::class); - $this->options = new UrlShortenerOptions(['append_extra_path' => true]); - - $this->middleware = new ExtraPathRedirectMiddleware( - $this->resolver->reveal(), - $this->requestTracker->reveal(), - $this->redirectionBuilder->reveal(), - $this->redirectResponseHelper->reveal(), - $this->options, - ); - $this->handler = $this->prophesize(RequestHandlerInterface::class); $this->handler->handle(Argument::cetera())->willReturn(new RedirectResponse('')); } @@ -71,10 +59,12 @@ class ExtraPathRedirectMiddlewareTest extends TestCase bool $multiSegmentEnabled, ServerRequestInterface $request, ): void { - $this->options->appendExtraPath = $appendExtraPath; - $this->options->multiSegmentSlugsEnabled = $multiSegmentEnabled; + $options = new UrlShortenerOptions( + appendExtraPath: $appendExtraPath, + multiSegmentSlugsEnabled: $multiSegmentEnabled, + ); - $this->middleware->process($request, $this->handler->reveal()); + $this->middleware($options)->process($request, $this->handler->reveal()); $this->handler->handle($request)->shouldHaveBeenCalledOnce(); $this->resolver->resolveEnabledShortUrl(Argument::cetera())->shouldNotHaveBeenCalled(); @@ -123,7 +113,7 @@ class ExtraPathRedirectMiddlewareTest extends TestCase bool $multiSegmentEnabled, int $expectedResolveCalls, ): void { - $this->options->multiSegmentSlugsEnabled = $multiSegmentEnabled; + $options = new UrlShortenerOptions(appendExtraPath: true, multiSegmentSlugsEnabled: $multiSegmentEnabled); $type = $this->prophesize(NotFoundType::class); $type->isRegularNotFound()->willReturn(true); @@ -135,7 +125,7 @@ class ExtraPathRedirectMiddlewareTest extends TestCase Argument::that(fn (ShortUrlIdentifier $identifier) => str_starts_with($identifier->shortCode, 'shortCode')), )->willThrow(ShortUrlNotFoundException::class); - $this->middleware->process($request, $this->handler->reveal()); + $this->middleware($options)->process($request, $this->handler->reveal()); $resolve->shouldHaveBeenCalledTimes($expectedResolveCalls); $this->requestTracker->trackIfApplicable(Argument::cetera())->shouldNotHaveBeenCalled(); @@ -152,7 +142,7 @@ class ExtraPathRedirectMiddlewareTest extends TestCase int $expectedResolveCalls, ?string $expectedExtraPath, ): void { - $this->options->multiSegmentSlugsEnabled = $multiSegmentEnabled; + $options = new UrlShortenerOptions(appendExtraPath: true, multiSegmentSlugsEnabled: $multiSegmentEnabled); $type = $this->prophesize(NotFoundType::class); $type->isRegularNotFound()->willReturn(true); @@ -181,7 +171,7 @@ class ExtraPathRedirectMiddlewareTest extends TestCase new RedirectResponse(''), ); - $this->middleware->process($request, $this->handler->reveal()); + $this->middleware($options)->process($request, $this->handler->reveal()); $resolve->shouldHaveBeenCalledTimes($expectedResolveCalls); $buildLongUrl->shouldHaveBeenCalledOnce(); @@ -194,4 +184,15 @@ class ExtraPathRedirectMiddlewareTest extends TestCase yield [false, 1, '/bar/baz']; yield [true, 3, null]; } + + private function middleware(?UrlShortenerOptions $options = null): ExtraPathRedirectMiddleware + { + return new ExtraPathRedirectMiddleware( + $this->resolver->reveal(), + $this->requestTracker->reveal(), + $this->redirectionBuilder->reveal(), + $this->redirectResponseHelper->reveal(), + $options ?? new UrlShortenerOptions(appendExtraPath: true), + ); + } } diff --git a/module/Core/test/Util/UrlValidatorTest.php b/module/Core/test/Util/UrlValidatorTest.php index de5cad23..cc13bd2c 100644 --- a/module/Core/test/Util/UrlValidatorTest.php +++ b/module/Core/test/Util/UrlValidatorTest.php @@ -23,15 +23,11 @@ class UrlValidatorTest extends TestCase { use ProphecyTrait; - private UrlValidator $urlValidator; private ObjectProphecy $httpClient; - private UrlShortenerOptions $options; protected function setUp(): void { $this->httpClient = $this->prophesize(ClientInterface::class); - $this->options = new UrlShortenerOptions(['validate_url' => true]); - $this->urlValidator = new UrlValidator($this->httpClient->reveal(), $this->options); } /** @test */ @@ -42,7 +38,7 @@ class UrlValidatorTest extends TestCase $request->shouldBeCalledOnce(); $this->expectException(InvalidUrlException::class); - $this->urlValidator->validateUrl('http://foobar.com/12345/hello?foo=bar', true); + $this->urlValidator()->validateUrl('http://foobar.com/12345/hello?foo=bar', true); } /** @test */ @@ -65,7 +61,7 @@ class UrlValidatorTest extends TestCase }), )->willReturn(new Response()); - $this->urlValidator->validateUrl($expectedUrl, true); + $this->urlValidator()->validateUrl($expectedUrl, true); $request->shouldHaveBeenCalledOnce(); } @@ -75,7 +71,7 @@ class UrlValidatorTest extends TestCase { $request = $this->httpClient->request(Argument::cetera())->willReturn(new Response()); - $this->urlValidator->validateUrl('', false); + $this->urlValidator()->validateUrl('', false); $request->shouldNotHaveBeenCalled(); } @@ -84,9 +80,8 @@ class UrlValidatorTest extends TestCase public function validateUrlWithTitleReturnsNullWhenRequestFailsAndValidationIsDisabled(): void { $request = $this->httpClient->request(Argument::cetera())->willThrow(ClientException::class); - $this->options->autoResolveTitles = true; - $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', false); + $result = $this->urlValidator(true)->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', false); self::assertNull($result); $request->shouldHaveBeenCalledOnce(); @@ -96,9 +91,8 @@ class UrlValidatorTest extends TestCase public function validateUrlWithTitleReturnsNullWhenAutoResolutionIsDisabled(): void { $request = $this->httpClient->request(Argument::cetera())->willReturn($this->respWithTitle()); - $this->options->autoResolveTitles = false; - $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', false); + $result = $this->urlValidator()->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', false); self::assertNull($result); $request->shouldNotHaveBeenCalled(); @@ -110,9 +104,8 @@ class UrlValidatorTest extends TestCase $request = $this->httpClient->request(RequestMethodInterface::METHOD_HEAD, Argument::cetera())->willReturn( $this->respWithTitle(), ); - $this->options->autoResolveTitles = false; - $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true); + $result = $this->urlValidator()->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true); self::assertNull($result); $request->shouldHaveBeenCalledOnce(); @@ -124,9 +117,8 @@ class UrlValidatorTest extends TestCase $request = $this->httpClient->request(RequestMethodInterface::METHOD_GET, Argument::cetera())->willReturn( $this->respWithTitle(), ); - $this->options->autoResolveTitles = true; - $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true); + $result = $this->urlValidator(true)->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true); self::assertEquals('Resolved "title"', $result); $request->shouldHaveBeenCalledOnce(); @@ -138,9 +130,8 @@ class UrlValidatorTest extends TestCase $request = $this->httpClient->request(RequestMethodInterface::METHOD_GET, Argument::cetera())->willReturn( new Response('php://memory', 200, ['Content-Type' => 'application/octet-stream']), ); - $this->options->autoResolveTitles = true; - $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true); + $result = $this->urlValidator(true)->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true); self::assertNull($result); $request->shouldHaveBeenCalledOnce(); @@ -152,9 +143,8 @@ class UrlValidatorTest extends TestCase $request = $this->httpClient->request(RequestMethodInterface::METHOD_GET, Argument::cetera())->willReturn( new Response($this->createStreamWithContent('No title'), 200, ['Content-Type' => 'text/html']), ); - $this->options->autoResolveTitles = true; - $result = $this->urlValidator->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true); + $result = $this->urlValidator(true)->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true); self::assertNull($result); $request->shouldHaveBeenCalledOnce(); @@ -174,4 +164,12 @@ class UrlValidatorTest extends TestCase return $body; } + + public function urlValidator(bool $autoResolveTitles = false): UrlValidator + { + return new UrlValidator( + $this->httpClient->reveal(), + new UrlShortenerOptions(autoResolveTitles: $autoResolveTitles), + ); + } } diff --git a/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php index 376c6bec..46fff970 100644 --- a/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php @@ -23,7 +23,7 @@ class CreateShortUrlAction extends AbstractCreateShortUrlAction { $payload = (array) $request->getParsedBody(); $payload[ShortUrlInputFilter::API_KEY] = AuthenticationMiddleware::apiKeyFromRequest($request); - $payload[EnvVars::MULTI_SEGMENT_SLUGS_ENABLED->value] = $this->urlShortenerOptions->multiSegmentSlugsEnabled(); + $payload[EnvVars::MULTI_SEGMENT_SLUGS_ENABLED->value] = $this->urlShortenerOptions->multiSegmentSlugsEnabled; return ShortUrlMeta::fromRawData($payload); } From 24088296270db3af2f6502703008e4e60fd6a433 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Sep 2022 15:55:54 +0200 Subject: [PATCH 11/11] Updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8d58fb8..f85af37f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#1339](https://github.com/shlinkio/shlink/issues/1339) Added new test suite for CLI E2E tests. * [#1503](https://github.com/shlinkio/shlink/issues/1503) Drastically improved build time in GitHub Actions, by optimizing parallelization and adding php extensions cache. * [#1525](https://github.com/shlinkio/shlink/issues/1525) Migrated to custom doctrine CLI entry point. +* [#1492](https://github.com/shlinkio/shlink/issues/1492) Migrated to immutable options objects, mapped with [cuyz/valinor](https://github.com/CuyZ/Valinor). ### Deprecated * *Nothing*