Changed VisitLocator signature so that it expects an object implementing an interface instead of two arbitrary callbacks

module/CLI/src/Command/Visit/LocateVisitsCommand.php

@@ -13,6 +13,7 @@ use Shlinkio\Shlink\Common\Util\IpAddress;
 use Shlinkio\Shlink\Core\Entity\Visit;
 use Shlinkio\Shlink\Core\Entity\VisitLocation;
 use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
+use Shlinkio\Shlink\Core\Visit\VisitGeolocationHelperInterface;
 use Shlinkio\Shlink\Core\Visit\VisitLocatorInterface;
 use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException;
 use Shlinkio\Shlink\IpGeolocation\Model\Location;
@@ -27,7 +28,7 @@ use Throwable;
 
 use function sprintf;
 
-class LocateVisitsCommand extends AbstractLockedCommand
+class LocateVisitsCommand extends AbstractLockedCommand implements VisitGeolocationHelperInterface
 {
     public const NAME = 'visit:locate';
 
@@ -73,20 +74,13 @@ class LocateVisitsCommand extends AbstractLockedCommand
     {
         $this->io = new SymfonyStyle($input, $output);
         $retry = $input->getOption('retry');
-        $geolocateVisit = [$this, 'getGeolocationDataForVisit'];
-        $notifyVisitWithLocation = static function (VisitLocation $location) use ($output): void {
-            $message = ! $location->isEmpty()
-                ? sprintf(' [<info>Address located in "%s"</info>]', $location->getCountryName())
-                : ' [<comment>Address not found</comment>]';
-            $output->writeln($message);
-        };
 
         try {
             $this->checkDbUpdate();
 
-            $this->visitLocator->locateUnlocatedVisits($geolocateVisit, $notifyVisitWithLocation);
+            $this->visitLocator->locateUnlocatedVisits($this);
             if ($retry) {
-                $this->visitLocator->locateVisitsWithEmptyLocation($geolocateVisit, $notifyVisitWithLocation);
+                $this->visitLocator->locateVisitsWithEmptyLocation($this);
             }
 
             $this->io->success('Finished processing all IPs');
@@ -101,7 +95,10 @@ class LocateVisitsCommand extends AbstractLockedCommand
         }
     }
 
-    public function getGeolocationDataForVisit(Visit $visit): Location
+    /**
+     * @throws IpCannotBeLocatedException
+     */
+    public function geolocateVisit(Visit $visit): Location
     {
         if (! $visit->hasRemoteAddr()) {
             $this->io->writeln(
@@ -130,6 +127,14 @@ class LocateVisitsCommand extends AbstractLockedCommand
         }
     }
 
+    public function onVisitLocated(VisitLocation $visitLocation, Visit $visit): void
+    {
+        $message = ! $visitLocation->isEmpty()
+            ? sprintf(' [<info>Address located in "%s"</info>]', $visitLocation->getCountryName())
+            : ' [<comment>Address not found</comment>]';
+        $this->io->writeln($message);
+    }
+
     private function checkDbUpdate(): void
     {
         try {

module/CLI/test/Command/Visit/LocateVisitsCommandTest.php

@@ -15,6 +15,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl;
 use Shlinkio\Shlink\Core\Entity\Visit;
 use Shlinkio\Shlink\Core\Entity\VisitLocation;
 use Shlinkio\Shlink\Core\Model\Visitor;
+use Shlinkio\Shlink\Core\Visit\VisitGeolocationHelperInterface;
 use Shlinkio\Shlink\Core\Visit\VisitLocator;
 use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException;
 use Shlinkio\Shlink\IpGeolocation\Model\Location;
@@ -24,7 +25,6 @@ use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Tester\CommandTester;
 use Symfony\Component\Lock;
 
-use function array_shift;
 use function sprintf;
 
 class LocateVisitsCommandTest extends TestCase
@@ -68,13 +68,7 @@ class LocateVisitsCommandTest extends TestCase
         $location = new VisitLocation(Location::emptyInstance());
 
         $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will(
-            function (array $args) use ($visit, $location): void {
+            $this->invokeHelperMethods($visit, $location),
-                $firstCallback = array_shift($args);
-                $firstCallback($visit);
-
-                $secondCallback = array_shift($args);
-                $secondCallback($location, $visit);
-            },
         );
         $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn(
             Location::emptyInstance(),
@@ -98,13 +92,7 @@ class LocateVisitsCommandTest extends TestCase
         $location = new VisitLocation(Location::emptyInstance());
 
         $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will(
-            function (array $args) use ($visit, $location): void {
+            $this->invokeHelperMethods($visit, $location),
-                $firstCallback = array_shift($args);
-                $firstCallback($visit);
-
-                $secondCallback = array_shift($args);
-                $secondCallback($location, $visit);
-            },
         );
         $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn(
             Location::emptyInstance(),
@@ -137,13 +125,7 @@ class LocateVisitsCommandTest extends TestCase
         $location = new VisitLocation(Location::emptyInstance());
 
         $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will(
-            function (array $args) use ($visit, $location): void {
+            $this->invokeHelperMethods($visit, $location),
-                $firstCallback = array_shift($args);
-                $firstCallback($visit);
-
-                $secondCallback = array_shift($args);
-                $secondCallback($location, $visit);
-            },
         );
         $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willThrow(WrongIpException::class);
 
@@ -156,6 +138,17 @@ class LocateVisitsCommandTest extends TestCase
         $resolveIpLocation->shouldHaveBeenCalledOnce();
     }
 
+    private function invokeHelperMethods(Visit $visit, VisitLocation $location): callable
+    {
+        return function (array $args) use ($visit, $location): void {
+            /** @var VisitGeolocationHelperInterface $helper */
+            [$helper] = $args;
+
+            $helper->geolocateVisit($visit);
+            $helper->onVisitLocated($location, $visit);
+        };
+    }
+
     /** @test */
     public function noActionIsPerformedIfLockIsAcquired(): void
     {

module/Core/src/Visit/VisitGeolocationHelperInterface.php

@@ -0,0 +1,20 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Visit;
+
+use Shlinkio\Shlink\Core\Entity\Visit;
+use Shlinkio\Shlink\Core\Entity\VisitLocation;
+use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
+use Shlinkio\Shlink\IpGeolocation\Model\Location;
+
+interface VisitGeolocationHelperInterface
+{
+    /**
+     * @throws IpCannotBeLocatedException
+     */
+    public function geolocateVisit(Visit $visit): Location;
+
+    public function onVisitLocated(VisitLocation $visitLocation, Visit $visit): void;
+}

module/Core/src/Visit/VisitLocator.php

@@ -8,36 +8,37 @@ use Doctrine\ORM\EntityManagerInterface;
 use Shlinkio\Shlink\Core\Entity\Visit;
 use Shlinkio\Shlink\Core\Entity\VisitLocation;
 use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
-use Shlinkio\Shlink\Core\Repository\VisitRepository;
+use Shlinkio\Shlink\Core\Repository\VisitRepositoryInterface;
 use Shlinkio\Shlink\IpGeolocation\Model\Location;
 
 class VisitLocator implements VisitLocatorInterface
 {
     private EntityManagerInterface $em;
+    private VisitRepositoryInterface $repo;
 
     public function __construct(EntityManagerInterface $em)
     {
         $this->em = $em;
+
+        /** @var VisitRepositoryInterface $repo */
+        $repo = $em->getRepository(Visit::class);
+        $this->repo = $repo;
     }
 
-    public function locateUnlocatedVisits(callable $geolocateVisit, callable $notifyVisitWithLocation): void
+    public function locateUnlocatedVisits(VisitGeolocationHelperInterface $helper): void
     {
-        /** @var VisitRepository $repo */
+        $this->locateVisits($this->repo->findUnlocatedVisits(), $helper);
-        $repo = $this->em->getRepository(Visit::class);
-        $this->locateVisits($repo->findUnlocatedVisits(), $geolocateVisit, $notifyVisitWithLocation);
     }
 
-    public function locateVisitsWithEmptyLocation(callable $geolocateVisit, callable $notifyVisitWithLocation): void
+    public function locateVisitsWithEmptyLocation(VisitGeolocationHelperInterface $helper): void
     {
-        /** @var VisitRepository $repo */
+        $this->locateVisits($this->repo->findVisitsWithEmptyLocation(), $helper);
-        $repo = $this->em->getRepository(Visit::class);
-        $this->locateVisits($repo->findVisitsWithEmptyLocation(), $geolocateVisit, $notifyVisitWithLocation);
     }
 
     /**
      * @param iterable|Visit[] $results
      */
-    private function locateVisits(iterable $results, callable $geolocateVisit, callable $notifyVisitWithLocation): void
+    private function locateVisits(iterable $results, VisitGeolocationHelperInterface $helper): void
     {
         $count = 0;
         $persistBlock = 200;
@@ -46,8 +47,7 @@ class VisitLocator implements VisitLocatorInterface
             $count++;
 
             try {
-                /** @var Location $location */
+                $location = $helper->geolocateVisit($visit);
-                $location = $geolocateVisit($visit);
             } catch (IpCannotBeLocatedException $e) {
                 if (! $e->isNonLocatableAddress()) {
                     // Skip if the visit's IP could not be located because of an error
@@ -59,7 +59,7 @@ class VisitLocator implements VisitLocatorInterface
             }
 
             $location = new VisitLocation($location);
-            $this->locateVisit($visit, $location, $notifyVisitWithLocation);
+            $this->locateVisit($visit, $location, $helper);
 
             // Flush and clear after X iterations
             if ($count % $persistBlock === 0) {
@@ -72,11 +72,11 @@ class VisitLocator implements VisitLocatorInterface
         $this->em->clear();
     }
 
-    private function locateVisit(Visit $visit, VisitLocation $location, callable $notifyVisitWithLocation): void
+    private function locateVisit(Visit $visit, VisitLocation $location, VisitGeolocationHelperInterface $helper): void
     {
         $visit->locate($location);
         $this->em->persist($visit);
 
-        $notifyVisitWithLocation($location, $visit);
+        $helper->onVisitLocated($location, $visit);
     }
 }

module/Core/src/Visit/VisitLocatorInterface.php

@@ -6,7 +6,7 @@ namespace Shlinkio\Shlink\Core\Visit;
 
 interface VisitLocatorInterface
 {
-    public function locateUnlocatedVisits(callable $geolocateVisit, callable $notifyVisitWithLocation): void;
+    public function locateUnlocatedVisits(VisitGeolocationHelperInterface $helper): void;
 
-    public function locateVisitsWithEmptyLocation(callable $geolocateVisit, callable $notifyVisitWithLocation): void;
+    public function locateVisitsWithEmptyLocation(VisitGeolocationHelperInterface $helper): void;
 }

module/Core/test/Visit/VisitLocatorTest.php

@@ -6,6 +6,7 @@ namespace ShlinkioTest\Shlink\Core\Visit;
 
 use Doctrine\ORM\EntityManager;
 use Exception;
+use PHPUnit\Framework\Assert;
 use PHPUnit\Framework\TestCase;
 use Prophecy\Argument;
 use Prophecy\Prophecy\ObjectProphecy;
@@ -14,7 +15,8 @@ use Shlinkio\Shlink\Core\Entity\Visit;
 use Shlinkio\Shlink\Core\Entity\VisitLocation;
 use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
 use Shlinkio\Shlink\Core\Model\Visitor;
-use Shlinkio\Shlink\Core\Repository\VisitRepository;
+use Shlinkio\Shlink\Core\Repository\VisitRepositoryInterface;
+use Shlinkio\Shlink\Core\Visit\VisitGeolocationHelperInterface;
 use Shlinkio\Shlink\Core\Visit\VisitLocator;
 use Shlinkio\Shlink\IpGeolocation\Model\Location;
 
@@ -30,10 +32,14 @@ class VisitLocatorTest extends TestCase
 {
     private VisitLocator $visitService;
     private ObjectProphecy $em;
+    private ObjectProphecy $repo;
 
     public function setUp(): void
     {
         $this->em = $this->prophesize(EntityManager::class);
+        $this->repo = $this->prophesize(VisitRepositoryInterface::class);
+        $this->em->getRepository(Visit::class)->willReturn($this->repo->reveal());
+
         $this->visitService = new VisitLocator($this->em->reveal());
     }
 
@@ -45,9 +51,7 @@ class VisitLocatorTest extends TestCase
             fn (int $i) => new Visit(new ShortUrl(sprintf('short_code_%s', $i)), Visitor::emptyInstance()),
         );
 
-        $repo = $this->prophesize(VisitRepository::class);
+        $findUnlocatedVisits = $this->repo->findUnlocatedVisits()->willReturn($unlocatedVisits);
-        $findUnlocatedVisits = $repo->findUnlocatedVisits()->willReturn($unlocatedVisits);
-        $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal());
 
         $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void {
         });
@@ -56,15 +60,22 @@ class VisitLocatorTest extends TestCase
         $clear = $this->em->clear()->will(function (): void {
         });
 
-        $this->visitService->locateUnlocatedVisits(fn () => Location::emptyInstance(), function (): void {
+        $this->visitService->locateUnlocatedVisits(new class implements VisitGeolocationHelperInterface {
-            $args = func_get_args();
+            public function geolocateVisit(Visit $visit): Location
+            {
+                return Location::emptyInstance();
+            }
 
-            $this->assertInstanceOf(VisitLocation::class, array_shift($args));
+            public function onVisitLocated(VisitLocation $visitLocation, Visit $visit): void
-            $this->assertInstanceOf(Visit::class, array_shift($args));
+            {
+                $args = func_get_args();
+
+                Assert::assertInstanceOf(VisitLocation::class, array_shift($args));
+                Assert::assertInstanceOf(Visit::class, array_shift($args));
+            }
         });
 
         $findUnlocatedVisits->shouldHaveBeenCalledOnce();
-        $getRepo->shouldHaveBeenCalledOnce();
         $persist->shouldHaveBeenCalledTimes(count($unlocatedVisits));
         $flush->shouldHaveBeenCalledTimes(floor(count($unlocatedVisits) / 200) + 1);
         $clear->shouldHaveBeenCalledTimes(floor(count($unlocatedVisits) / 200) + 1);
@@ -80,9 +91,7 @@ class VisitLocatorTest extends TestCase
             new Visit(new ShortUrl('foo'), Visitor::emptyInstance()),
         ];
 
-        $repo = $this->prophesize(VisitRepository::class);
+        $findUnlocatedVisits = $this->repo->findUnlocatedVisits()->willReturn($unlocatedVisits);
-        $findUnlocatedVisits = $repo->findUnlocatedVisits()->willReturn($unlocatedVisits);
-        $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal());
 
         $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void {
         });
@@ -91,15 +100,29 @@ class VisitLocatorTest extends TestCase
         $clear = $this->em->clear()->will(function (): void {
         });
 
-        $this->visitService->locateUnlocatedVisits(function () use ($isNonLocatableAddress): void {
+        $this->visitService->locateUnlocatedVisits(
-            throw $isNonLocatableAddress
+            new class ($isNonLocatableAddress) implements VisitGeolocationHelperInterface {
-                ? new IpCannotBeLocatedException('Cannot be located')
+                private bool $isNonLocatableAddress;
-                : IpCannotBeLocatedException::forError(new Exception(''));
+
-        }, static function (): void {
+                public function __construct(bool $isNonLocatableAddress)
-        });
+                {
+                    $this->isNonLocatableAddress = $isNonLocatableAddress;
+                }
+
+                public function geolocateVisit(Visit $visit): Location
+                {
+                    throw $this->isNonLocatableAddress
+                        ? new IpCannotBeLocatedException('Cannot be located')
+                        : IpCannotBeLocatedException::forError(new Exception(''));
+                }
+
+                public function onVisitLocated(VisitLocation $visitLocation, Visit $visit): void
+                {
+                }
+            },
+        );
 
         $findUnlocatedVisits->shouldHaveBeenCalledOnce();
-        $getRepo->shouldHaveBeenCalledOnce();
         $persist->shouldHaveBeenCalledTimes($isNonLocatableAddress ? 1 : 0);
         $flush->shouldHaveBeenCalledOnce();
         $clear->shouldHaveBeenCalledOnce();