From 6aaea2ac26e13fcf62983cf1292f5ea33f6411cf Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 15 Nov 2024 09:00:59 +0100 Subject: [PATCH] Simplify logic in RedirectRule when checking geolocation conditions --- module/Core/functions/functions.php | 7 +++++- .../RedirectRule/Entity/RedirectCondition.php | 16 +++++------- .../Entity/RedirectConditionTest.php | 25 ++----------------- 3 files changed, 14 insertions(+), 34 deletions(-) diff --git a/module/Core/functions/functions.php b/module/Core/functions/functions.php index 1d989c75..6ccc42e2 100644 --- a/module/Core/functions/functions.php +++ b/module/Core/functions/functions.php @@ -293,5 +293,10 @@ function ipAddressFromRequest(ServerRequestInterface $request): string|null function geolocationFromRequest(ServerRequestInterface $request): Location|null { - return $request->getAttribute(Location::class); + $geolocation = $request->getAttribute(Location::class); + if ($geolocation !== null && ! $geolocation instanceof Location) { + // TODO Throw exception + } + + return $geolocation; } diff --git a/module/Core/src/RedirectRule/Entity/RedirectCondition.php b/module/Core/src/RedirectRule/Entity/RedirectCondition.php index 9468d582..cf1e134b 100644 --- a/module/Core/src/RedirectRule/Entity/RedirectCondition.php +++ b/module/Core/src/RedirectRule/Entity/RedirectCondition.php @@ -9,11 +9,10 @@ use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\RedirectRule\Model\RedirectConditionType; use Shlinkio\Shlink\Core\RedirectRule\Model\Validation\RedirectRulesInputFilter; use Shlinkio\Shlink\Core\Util\IpAddressUtils; -use Shlinkio\Shlink\Core\Visit\Entity\VisitLocation; -use Shlinkio\Shlink\IpGeolocation\Model\Location; use function Shlinkio\Shlink\Core\acceptLanguageToLocales; use function Shlinkio\Shlink\Core\ArrayUtils\some; +use function Shlinkio\Shlink\Core\geolocationFromRequest; use function Shlinkio\Shlink\Core\ipAddressFromRequest; use function Shlinkio\Shlink\Core\normalizeLocale; use function Shlinkio\Shlink\Core\splitLocale; @@ -134,9 +133,8 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable private function matchesGeolocationCountryCode(ServerRequestInterface $request): bool { - $geolocation = $request->getAttribute(Location::class); - // TODO We should eventually rely on `Location` type only - if (! ($geolocation instanceof Location) && ! ($geolocation instanceof VisitLocation)) { + $geolocation = geolocationFromRequest($request); + if ($geolocation === null) { return false; } @@ -145,14 +143,12 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable private function matchesGeolocationCityName(ServerRequestInterface $request): bool { - $geolocation = $request->getAttribute(Location::class); - // TODO We should eventually rely on `Location` type only - if (! ($geolocation instanceof Location) && ! ($geolocation instanceof VisitLocation)) { + $geolocation = geolocationFromRequest($request); + if ($geolocation === null) { return false; } - $cityName = $geolocation instanceof Location ? $geolocation->city : $geolocation->cityName; - return strcasecmp($cityName, $this->matchValue) === 0; + return strcasecmp($geolocation->city, $this->matchValue) === 0; } public function jsonSerialize(): array diff --git a/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php b/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php index d22b632d..2ae5df18 100644 --- a/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php +++ b/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php @@ -10,7 +10,6 @@ use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Common\Middleware\IpAddressMiddlewareFactory; use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\RedirectRule\Entity\RedirectCondition; -use Shlinkio\Shlink\Core\Visit\Entity\VisitLocation; use Shlinkio\Shlink\IpGeolocation\Model\Location; use const ShlinkioTest\Shlink\ANDROID_USER_AGENT; @@ -99,7 +98,7 @@ class RedirectConditionTest extends TestCase #[Test, DataProvider('provideVisitsWithCountry')] public function matchesGeolocationCountryCode( - Location|VisitLocation|null $location, + Location|null $location, string $countryCodeToMatch, bool $expected, ): void { @@ -114,21 +113,11 @@ class RedirectConditionTest extends TestCase yield 'non-matching location' => [new Location(countryCode: 'ES'), 'US', false]; yield 'matching location' => [new Location(countryCode: 'US'), 'US', true]; yield 'matching case-insensitive' => [new Location(countryCode: 'US'), 'us', true]; - yield 'matching visit location' => [ - VisitLocation::fromGeolocation(new Location(countryCode: 'US')), - 'US', - true, - ]; - yield 'matching visit case-insensitive' => [ - VisitLocation::fromGeolocation(new Location(countryCode: 'es')), - 'ES', - true, - ]; } #[Test, DataProvider('provideVisitsWithCity')] public function matchesGeolocationCityName( - Location|VisitLocation|null $location, + Location|null $location, string $cityNameToMatch, bool $expected, ): void { @@ -143,15 +132,5 @@ class RedirectConditionTest extends TestCase yield 'non-matching location' => [new Location(city: 'Los Angeles'), 'New York', false]; yield 'matching location' => [new Location(city: 'Madrid'), 'Madrid', true]; yield 'matching case-insensitive' => [new Location(city: 'Los Angeles'), 'los angeles', true]; - yield 'matching visit location' => [ - VisitLocation::fromGeolocation(new Location(city: 'New York')), - 'New York', - true, - ]; - yield 'matching visit case-insensitive' => [ - VisitLocation::fromGeolocation(new Location(city: 'barcelona')), - 'Barcelona', - true, - ]; } }