First tranche of changes. Can now find street from geolocation. #285
Conversation
There was a problem hiding this comment.
Pull request overview
Renames the image “location” concept to “neighbourhood”, and introduces async geolocation-based tagging to derive street names from GPS coordinates.
Changes:
- Renames
Image::locationtoImage::neighbourhoodacross entity, forms, templates, and stats. - Adds
streettoImageplus a Messenger-basedGeolocateImagepipeline backed by a Google geolocation client. - Updates admin UI/actions and stats UI styling to reflect the neighbourhood rename.
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/LocationServiceTest.php | Updates tests to use NeighbourhoodService. |
| tests/ImageServiceTest.php | Updates EXIF/location tests to use neighbourhood APIs. |
| templates/stats/index.html.twig | Renames “locations” UI to “neighbourhoods” and chart vars. |
| templates/admin/index.html.twig | Renames admin stats section to neighbourhoods. |
| templates/admin/image/show.html.twig | Displays image.neighbourhood and swaps form include. |
| templates/admin/image/_set_neighbourhood_form.html.twig | New admin POST form for neighbourhood auto-update. |
| templates/admin/image/_set_location_form.html.twig | Removes legacy location setter form. |
| src/Service/StatsService.php | Renames stats method/query to neighbourhood-based grouping. |
| src/Service/NeighbourhoodServiceInterface.php | Introduces neighbourhood lookup interface. |
| src/Service/NeighbourhoodService.php | Renames location service to neighbourhood service implementation. |
| src/Service/LocationTaggingServiceInterface.php | Introduces interface for location/street tagging. |
| src/Service/ImageService.php | Renames EXIF setter and switches to neighbourhood lookup. |
| src/Service/GoogleLocationTaggingService.php | Adds Google-based street lookup & persistence. |
| src/Repository/ImageRepository.php | Adds street/neighbourhood query helpers and renames location helpers. |
| src/MessageHandler/GeolocateImageHandler.php | Adds Messenger handler to run location tagging per image. |
| src/Message/GeolocateImage.php | Adds async message DTO for geolocation tagging. |
| src/Form/ImageType.php | Renames form field from location to neighbourhood. |
| src/Form/ImageFilterData.php | Renames filter data field from location to neighbourhood. |
| src/EventListener/ImageGeneralPostPersistListener.php | Dispatches geolocation job on image persist. |
| src/Entity/Image.php | Renames persisted field to neighbourhood and adds street. |
| src/Controller/Image/ImageController.php | Updates filtering to use neighbourhood repository/query. |
| src/Controller/Admin/ProblemController.php | Renames “no location” problem metric/route to neighbourhood. |
| src/Controller/Admin/ImageController.php | Renames admin action to set neighbourhood. |
| src/Controller/Admin/AdminController.php | Renames admin stats call/variable to neighbourhood stats. |
| src/Command/ImagesUpdatelocationsCommand.php | Converts location updater to queue geolocation jobs (async). |
| src/Command/ImagesTagCommand.php | Minor cleanup when dispatching recognition jobs. |
| migrations/Version20260201215733.php | DB migration to rename image.location -> image.neighbourhood. |
| migrations/Version20260201203811.php | DB migration adding image.street + messenger indexes. |
| config/services.yaml | Wires new services/interfaces and binds Google API key. |
| config/packages/messenger.yaml | Routes GeolocateImage messages to async transport. |
| composer.lock | Locks new dependency versions including geolocation client. |
| composer.json | Adds VCS repo + requires geolocation client package. |
| assets/styles/app.scss | Renames location stats styles to neighbourhood equivalents. |
Comments suppressed due to low confidence (1)
src/Service/NeighbourhoodService.php:13
NeighbourhoodServiceincludes a number of unused imports (Image,Neighbourhood,EntityManagerInterface,Client,HandlerStack,Double,LoggerInterface,RateLimiterMiddleware). Please remove them to avoid confusion about external dependencies and keep the service minimal.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use App\Exception\ThirdPartyAPIException; | ||
| use Doctrine\ORM\EntityManagerInterface; | ||
| use Exception; | ||
| use Google\Rpc\Status; | ||
| use Imagine\Gd\Imagine; | ||
| use Imagine\Image\Box; |
There was a problem hiding this comment.
Several imported classes are unused in this new service (ThirdPartyAPIException, Exception, Google\Rpc\Status, Imagine\Gd\Imagine, Imagine\Image\Box). Please remove unused use statements to keep dependencies clear.
| use App\Exception\ThirdPartyAPIException; | |
| use Doctrine\ORM\EntityManagerInterface; | |
| use Exception; | |
| use Google\Rpc\Status; | |
| use Imagine\Gd\Imagine; | |
| use Imagine\Image\Box; | |
| use Doctrine\ORM\EntityManagerInterface; |
| public function getNeighbourhood(): ?string | ||
| { | ||
| return $this->location; | ||
| return $this->neighbourhood; | ||
| } | ||
|
|
||
| public function hasLocation(): bool | ||
| public function hasNeighbourhood(): bool | ||
| { | ||
| return $this->location !== null && $this->location <> ''; | ||
| return $this->neighbourhood !== null && $this->neighbourhood <> ''; | ||
| } | ||
|
|
||
| public function setLocation(?string $location): self | ||
| public function setNeighbourhood(?string $neighbourhood): self | ||
| { | ||
| $this->location = $location; | ||
| $this->neighbourhood = $neighbourhood; | ||
|
|
There was a problem hiding this comment.
There are still runtime references to the old location field in the app (e.g. templates/image/show.html.twig uses image.location, and src/Controller/StatsController.php builds URLs with a location query param). After renaming the entity property/migration, these need updating to neighbourhood (or a backwards-compatible accessor/alias is needed) to avoid undefined property/method errors in Twig and controllers.
| $images = $this->imageRepository->findWithHasLatLng(); | ||
| $total = count($images); | ||
| $success = 0; | ||
| $failure = 0; | ||
|
|
||
| $progressBar = new ProgressBar($output, $total); | ||
| $progressBar->start(); | ||
| foreach ($images as $image) { | ||
| $neighbourhood = $this->locationService->getLocationName($image->getLatitude(), $image->getLongitude()); | ||
| if ($neighbourhood !== null) { | ||
| $image->setLocation($neighbourhood); | ||
| $this->entityManager->persist($image); | ||
| $this->entityManager->flush(); // It actually seems faster to flush in the loop, rather than afterwards. Odd. | ||
| $success++; | ||
| } else { | ||
| $failure++; | ||
| $imageid = $image->getId(); | ||
| if ($imageid === null) { | ||
| throw new \RuntimeException("Image has no ID"); | ||
| } | ||
| $this->messageBus->dispatch(new GeolocateImage($imageid, $overwrite)); | ||
| $progressBar->advance(); |
There was a problem hiding this comment.
findWithHasLatLng() queues geolocation for all images with coordinates, even when nothing needs updating. Since GoogleLocationTaggingService is a no-op when hasStreet() (unless --overwrite), consider using findWithNoStreetButHasLatLng() when overwrite is false to avoid sending a large number of unnecessary Messenger jobs / API calls.
| // Filtering form for the top of the page | ||
| $locationChoices = $this->getLocationChoices($imageRepository); | ||
| $neighbourhoodChoices = $this->getNeighbourhoodChoices($imageRepository); | ||
| $filterForm = $this->createForm( | ||
| ImageFilterType::class, | ||
| $filterData, | ||
| [ | ||
| 'locations' => array_combine($locationChoices, array_values($locationChoices)), | ||
| 'neighbourhoods' => array_combine($neighbourhoodChoices, array_values($neighbourhoodChoices)), | ||
| 'csrf_protection' => false, // We're just a GET request, and nothing bad happens no matter what you do. |
There was a problem hiding this comment.
The filter wiring here is inconsistent with the rest of the rename:
ImageFilterTypestill defines alocationfield and expects an option namedlocations.ImageFilterDatanow exposesoverrideNeighbourhoodFromUrlParam()/hasNeighbourhood().
As written, passing theneighbourhoodsoption will throw an OptionsResolver error, and the earlieroverrideLocationFromUrlParam()call will fatally error. Update the form type + query param handling to useneighbourhoodconsistently (or keep the old naming everywhere for backwards compat).
| /** | ||
| * Location was a late addition and we have a Command to update existing ones. We don't want to touch | ||
| * any other existing data, so this is a special one-off to set Location. | ||
| * Neighbourhood was a late addition and we have a Command to update existing ones. We don't want to touch | ||
| * any other existing data, so this is a special one-off to set Neighbourhood. | ||
| */ | ||
| public function setLocationFromEXIF(Image $image): void | ||
| public function setNeighbourhoodFromEXIF(Image $image): void | ||
| { |
There was a problem hiding this comment.
The standalone EXIF setter was renamed to setNeighbourhoodFromEXIF(), but there are still callers using the old name (e.g. src/Command/WanderSetImageLocationsCommand.php:71 calls setLocationFromEXIF()). Either update remaining call sites or keep a backwards-compatible wrapper method to avoid runtime fatals.
| } | ||
| return true; |
There was a problem hiding this comment.
tagImage() returns true even when no route component is found and nothing is persisted (i.e. $routeLongName stays null). This makes it hard for callers to know whether an image was actually updated; consider returning false when no street was set/persisted (and reserving true for a successful update).
| } | |
| return true; | |
| return true; | |
| } | |
| return false; |
| use Symfony\Component\Routing\RouterInterface; | ||
|
|
||
| class ImagesUpdatelocationsCommand extends Command | ||
| class ImagesUpdateLocationsCommand extends Command |
There was a problem hiding this comment.
PSR-4 autoloading will not find App\Command\ImagesUpdateLocationsCommand in this file because the filename is ImagesUpdatelocationsCommand.php. Rename the file to ImagesUpdateLocationsCommand.php (preferred) or revert the class name to match the current filename, otherwise the command/service won’t be loadable.
| class ImagesUpdateLocationsCommand extends Command | |
| class ImagesUpdatelocationsCommand extends Command |
| use App\Entity\Image; | ||
| use App\Message\GeolocateImage; | ||
| use App\Repository\ImageRepository; | ||
| use App\Service\LocationService; | ||
| use Doctrine\ORM\EntityManagerInterface; | ||
| use Symfony\Component\Console\Command\Command; | ||
| use Symfony\Component\Console\Helper\ProgressBar; | ||
| use Symfony\Component\Console\Input\InputArgument; | ||
| use Symfony\Component\Console\Input\InputInterface; | ||
| use Symfony\Component\Console\Input\InputOption; | ||
| use Symfony\Component\Console\Output\OutputInterface; | ||
| use Symfony\Component\Console\Style\SymfonyStyle; | ||
| use Symfony\Component\Messenger\MessageBusInterface; | ||
| use Symfony\Component\Routing\RouterInterface; |
There was a problem hiding this comment.
This command now injects/declares several unused dependencies/imports (EntityManagerInterface, RouterInterface, InputArgument, App\Entity\Image). Please remove the unused constructor args/properties and use statements to avoid unnecessary container wiring and keep the command focused.
| <div class="col-2 d-none d-md-block"> | ||
| <div class="headline locations"> | ||
| <div class="name">Photo Locations</div> | ||
| <div class="value">{{ imageLocationStats | length }}</div> | ||
| <div class="headline neighbourhoods"> | ||
| <div class="name">Photo Neighbourhoods</div> | ||
| <div class="value">{{ imageNeighbourhoodStats | length }}</div> | ||
| </div> | ||
| </div> | ||
| <div class="col" style="height: {{ (imageLocationStats | length) * 30 }}px;"> | ||
| {{ render_chart(locationsChart, { | ||
| 'class': 'details graph locations', | ||
| 'data-controller': 'charts--location' | ||
| <div class="col" style="height: {{ (imageNeighbourhoodStats | length) * 30 }}px;"> | ||
| {{ render_chart(neighbourhoodsChart, { | ||
| 'class': 'details graph neighbourhoods', | ||
| 'data-controller': 'charts--neighbourhood' | ||
| }) |
There was a problem hiding this comment.
This template now expects imageNeighbourhoodStats and neighbourhoodsChart, but StatsController still renders imageLocationStats and locationsChart and calls getImageLocationStats(). This will cause undefined-variable errors in Twig until the controller output is renamed to match.
| public function getImageNeighbourhoodStats(): array | ||
| { | ||
| return $this->cache->get('image_location_stats', function (ItemInterface $item) { | ||
| return $this->cache->get('image_neighbourhood_stats', function (ItemInterface $item) { | ||
| $item->tag('stats'); | ||
| $stats = $this->imageRepository | ||
| ->createQueryBuilder('i') | ||
| ->select('i.location') | ||
| ->addSelect('COUNT(i) AS locationCount') | ||
| ->groupBy('i.location') | ||
| ->Where('i.location IS NOT NULL') | ||
| ->OrderBy('i.location') | ||
| ->select('i.neighbourhood') | ||
| ->addSelect('COUNT(i) AS neighbourhoodCount') | ||
| ->groupBy('i.neighbourhood') | ||
| ->Where('i.neighbourhood IS NOT NULL') | ||
| ->OrderBy('i.neighbourhood') | ||
| ->getQuery() | ||
| ->getResult(); | ||
| return array_column($stats, 'locationCount', 'location'); | ||
| return array_column($stats, 'neighbourhoodCount', 'neighbourhood'); | ||
| }); |
There was a problem hiding this comment.
getImageLocationStats() has been renamed/removed, but StatsController still calls getImageLocationStats() (see src/Controller/StatsController.php:45). Until the controller is updated to call getImageNeighbourhoodStats() and render matching variables, the /stats page will fatally error.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 42 changed files in this pull request and generated 14 comments.
Comments suppressed due to low confidence (2)
src/Service/ImageService.php:95
- Log message typo: "EXIT" should be "EXIF".
$this->logger->info('Ignoring non-JPEG file when trying to set properties from EXIT.');
return;
src/Service/NeighbourhoodService.php:15
- There are a number of unused imports left in this service (e.g.
LoggerInterface,RateLimiterMiddleware, plus earlier imports likeGuzzleHttp\Client,EntityManagerInterface, etc.). None of them are referenced in the class body.
Please remove the unused use statements to keep the service focused and avoid misleading dependencies.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $this->addSql('ALTER TABLE image rename column location to neighbourhood'); | ||
| } | ||
|
|
||
| public function down(Schema $schema): void | ||
| { | ||
| // this down() migration is auto-generated, please modify it to your needs | ||
| $this->addSql('ALTER TABLE image rename column neighbourhood to location'); |
There was a problem hiding this comment.
This migration uses ALTER TABLE image rename column ..., which is not supported by MySQL 5.7 (the test env is configured with serverVersion=5.7). The migration will fail on that platform.
Use a MySQL 5.7-compatible column rename (e.g. CHANGE location neighbourhood VARCHAR(255) DEFAULT NULL), or use Doctrine DBAL’s platform-aware schema operations.
| $this->addSql('ALTER TABLE image rename column location to neighbourhood'); | |
| } | |
| public function down(Schema $schema): void | |
| { | |
| // this down() migration is auto-generated, please modify it to your needs | |
| $this->addSql('ALTER TABLE image rename column neighbourhood to location'); | |
| $this->addSql('ALTER TABLE image CHANGE location neighbourhood VARCHAR(255) DEFAULT NULL'); | |
| } | |
| public function down(Schema $schema): void | |
| { | |
| // this down() migration is auto-generated, please modify it to your needs | |
| $this->addSql('ALTER TABLE image CHANGE neighbourhood location VARCHAR(255) DEFAULT NULL'); |
| ->groupBy('i.location') | ||
| ->orderBy('i.location') | ||
| ->select('i.neighbourhood') | ||
| ->where('i.neighbourhood IS NOT NULL') |
There was a problem hiding this comment.
getAllNeighbourhoods() only filters out NULL, but elsewhere the repo treats '' as “missing” (see findWithNoNeighbourhood*). As written, this method can return an empty-string neighbourhood, leading to a blank choice in the filter dropdown.
Consider excluding empty strings here as well (e.g. neighbourhood <> '') to keep filtering/UI consistent.
| ->where('i.neighbourhood IS NOT NULL') | |
| ->where('i.neighbourhood IS NOT NULL') | |
| ->andWhere("i.neighbourhood <> ''") |
| ->select('i.neighbourhood') | ||
| ->addSelect('COUNT(i) AS neighbourhoodCount') | ||
| ->groupBy('i.neighbourhood') | ||
| ->Where('i.neighbourhood IS NOT NULL') | ||
| ->OrderBy('i.neighbourhood') |
There was a problem hiding this comment.
This stats query filters out NULL neighbourhoods but will still include empty strings ('') if they exist. Since other parts of the code treat '' as “missing”, this can produce an empty label/bar in the chart.
Consider adding an explicit neighbourhood <> '' condition to keep stats output clean and consistent.
| // TODO: Add option to overwrite existing tags | ||
| ->addOption('overwrite', null, InputOption::VALUE_NONE, 'Overwrite existing locations') | ||
| ; | ||
| } |
There was a problem hiding this comment.
The comment // TODO: Add option to overwrite existing tags is now stale (the --overwrite option is implemented immediately below). Also, the option description says “Overwrite existing locations” but the command currently targets street tagging (findWithNoStreetButHasLatLng).
Update/remove the TODO and consider clarifying the option/command wording so users know what will be overwritten/tagged.
| // TODO: Add option to overwrite existing tags | |
| ->addOption('overwrite', null, InputOption::VALUE_NONE, 'Overwrite existing locations') | |
| ; | |
| } | |
| ->addOption('overwrite', null, InputOption::VALUE_NONE, 'Overwrite existing street/location tags for images that already have coordinates') | |
| ; | |
| } | |
| } |
| $result = $image->getNeighbourhood(); | ||
| $this->assertEquals('Hotwells & Harbourside', $result, 'Failed to read location from image'); | ||
|
|
There was a problem hiding this comment.
The assertion messages here still say “location”, but the field under test is now neighbourhood. Updating the messages will make test failures clearer.
| public function setNeighbourhood(Request $request, Image $image, NeighbourhoodService $neighbourhoodService, EntityManagerInterface $entityManager): Response | ||
| { | ||
| if ($this->isCsrfTokenValid('set_location'.$image->getId(), (string) $request->request->get('_token'))) { | ||
| $neighbourhood = $locationService->getLocationName($image->getLatitude(), $image->getLongitude()); | ||
| if ($this->isCsrfTokenValid('set_neighbourhood'.$image->getId(), (string) $request->request->get('_token'))) { | ||
| $neighbourhood = $neighbourhoodService->getNeighbourhood($image->getLatitude(), $image->getLongitude()); |
There was a problem hiding this comment.
This action type-hints the concrete NeighbourhoodService, even though NeighbourhoodServiceInterface was introduced and registered as the DI alias. Type-hinting the interface here would keep controllers decoupled from implementations and makes swapping/mocking easier (especially for tests).
| // Queue up some image recognition and location tagging | ||
| $id = $image->getId(); | ||
| if ($id !== null) { | ||
| $this->messageBus->dispatch(new RecogniseImage($id)); | ||
| $this->messageBus->dispatch(new GeolocateImage($id)); |
There was a problem hiding this comment.
Dispatching GeolocateImage on every postPersist will execute synchronously in the test environment (because config/packages/test/messenger.yaml sets the async transport to sync://). That means tests which persist Image entities will immediately run GeolocateImageHandler and invoke the real LocationTaggingServiceInterface (Google API), which is slow/flaky and will fail without a test API key.
Consider overriding LocationTaggingServiceInterface with a dummy in services_test.yaml, routing GeolocateImage to a non-sync transport in tests, or guarding this dispatch behind an environment/config flag.
| $wanderSimplifierEpsilonMetres: '%wander.simplifier_epsilon_metres%' | ||
| $wanderGeoJsonOutputPrecision: '%wander.geojson_output_precision%' | ||
| $kernelEnvironment: '%kernel.environment%' | ||
| $googleApiKey: '%google.api_key%' |
There was a problem hiding this comment.
Binding $googleApiKey globally makes google.api_key effectively required whenever GoogleLocationTaggingService is constructed. Ensure GOOGLE_API_KEY is provided in all environments (including tests), or make the env var optional (e.g. via a default env processor) and have the tagging service no-op when unset.
Also consider overriding LocationTaggingServiceInterface in the test container (similar to ImageTaggingServiceInterface) to avoid external HTTP calls during test runs.
| ->addSelect('SUM(CASE WHEN i.title IS NULL THEN 1 ELSE 0 END) AS no_title') | ||
| ->addSelect('SUM(CASE WHEN i.latlng IS NULL THEN 1 ELSE 0 END) AS no_latlng') | ||
| ->addSelect('SUM(CASE WHEN i.location IS NULL THEN 1 ELSE 0 END) AS no_location') | ||
| ->addSelect('SUM(CASE WHEN i.neighbourhood IS NULL THEN 1 ELSE 0 END) AS no_neighbourhood') | ||
| ->addSelect('SUM(CASE WHEN i.rating IS NULL OR i.rating = 0 THEN 1 ELSE 0 END) AS no_rating') |
There was a problem hiding this comment.
The “missing neighbourhood” counts only check IS NULL. Elsewhere you treat empty strings as missing too (e.g. ImageRepository::findWithNoNeighbourhood()), so this report can undercount problems if neighbourhoods are stored as ''.
Consider counting both NULL and empty string here for consistency (and likewise in the weighted score calculation).
| {% if imagehit.highlight['images.street'] is defined %} | ||
| <p class="mt-2"><strong>Street:</strong> | ||
| {% for streethighlight in imagehit.highlight['images.street'] %} | ||
| {{ streethighlight | raw }} | ||
| {% endfor %} | ||
| </p> | ||
| {% endif %} | ||
|
|
||
| {% if imagehit.highlight['images.neighbourhood'] is defined %} | ||
| <p><strong>Neighbourhood:</strong> | ||
| {% for neighbourhoodhighlight in imagehit.highlight['images.neighbourhood'] %} | ||
| {{ neighbourhoodhighlight | raw }} | ||
| {% endfor %} | ||
| </p> | ||
| {% endif %} |
There was a problem hiding this comment.
The | raw usage when rendering streethighlight and neighbourhoodhighlight allows untrusted search highlight fragments to be injected directly into the page as HTML/JavaScript, enabling stored XSS via EXIF-derived locations, admin-entered fields, or other external data indexed in Elasticsearch. An attacker can upload an image whose EXIF location or neighbourhood field contains <script> or other HTML, have it indexed and highlighted, and then execute arbitrary script in any user's browser when they view these search results. To fix this, avoid using | raw on these highlight values and either rely on Twig’s automatic escaping or explicitly strip/whitelist HTML tags (as is already done for the wander-level highlights with striptags('<mark>')) before rendering.
No description provided.