From fe4b2c4ae49f0bdc3242a71a9d67cc9e8a2ba9d3 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Sep 2022 12:57:04 +0200 Subject: [PATCH] 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(), + ); + } }