From fcfbcc0b87b023239984e1e8bfcd7465e7ea955a Mon Sep 17 00:00:00 2001 From: edwh Date: Wed, 15 Jul 2026 10:02:04 +0100 Subject: [PATCH 1/3] RES-1858: switch address geocoding from Google to Mapbox The rest of the Mapbox geocoder plumbing (config/geocoder.php, the geocoder-php/mapbox-provider + toin0u/geocoder-laravel packages, and MAPBOX_TOKEN in .env/CI/Fly secrets) already landed on develop as part of other work and is already used for reverse geocoding in SetPlaceNetworkData. This just switches the remaining forward-geocode path, App\Helpers\Geocoder::geocode(), from a raw Google Maps HTTP call to the same app('geocoder') chain (Mapbox, falling back to GeoPlugin), matching that existing pattern. country_code now comes straight from the provider's ISO country code rather than matching the returned country name against our own country list, and the dead reverseGeocode() method (superseded by the reverseQuery() call already in SetPlaceNetworkData) is removed. Public interface, the Geocoder container binding, and the test mocks (GeocoderMock, GeocoderFailMock) are unchanged, so no other files need updating for this to work. This branch is rebuilt on current develop (~1240 commits ahead of where it last was) so CI can actually run; the frontend part of the original PR (replacing vue-google-autocomplete with a Mapbox GL widget) is intentionally not carried over - see PR comment. Co-Authored-By: Claude Fable 5 --- app/Helpers/Geocoder.php | 60 +++++++++++++++------------------------- 1 file changed, 22 insertions(+), 38 deletions(-) diff --git a/app/Helpers/Geocoder.php b/app/Helpers/Geocoder.php index 6dea4b0f96..1418ee935e 100644 --- a/app/Helpers/Geocoder.php +++ b/app/Helpers/Geocoder.php @@ -2,56 +2,40 @@ namespace App\Helpers; +use Geocoder\Provider\Mapbox\Mapbox; +use Geocoder\Query\GeocodeQuery; + class Geocoder { public function __construct() { } - private function googleKey() - { - // We have this so that we can change the key in testing. - return config('GOOGLE_API_CONSOLE_KEY') ?? env('GOOGLE_API_CONSOLE_KEY'); - } - public function geocode($location) { if ($location != 'ForceGeocodeFailure') { - $json = file_get_contents('https://maps.googleapis.com/maps/api/geocode/json?address='.urlencode($location).'&key='.$this->googleKey()); - - if ($json) { - $res = json_decode($json); - - if ($res && $res->results && count($res->results)) { - $decoded = json_decode($json)->results[0]; - - $latitude = $decoded->{'geometry'}->{'location'}->lat; - $longitude = $decoded->{'geometry'}->{'location'}->lng; - - foreach ($decoded->{'address_components'} as $component) { - if ($component->types && count($component->types) && $component->types[0] === 'country') { - $country_code = $component->short_name; - } - } - - return [ - 'latitude' => $latitude, - 'longitude' => $longitude, - 'country_code' => $country_code, - ]; - } + // Use the geocoder configured in config/geocoder.php - a chain of Mapbox (primary) and GeoPlugin + // (fallback), resolved via the container so tests can bind a mock Geocoder instead. This is the + // same pattern used by App\Console\Commands\SetPlaceNetworkData for reverse geocoding. + $geocodeResponse = app('geocoder')->geocodeQuery( + GeocodeQuery::create($location)->withData('location_type', [Mapbox::TYPE_PLACE, Mapbox::TYPE_ADDRESS]) + ); + $addressCollection = $geocodeResponse->get(); + $address = $addressCollection->get(0); + + if ($address && $address->getCoordinates()) { + // The provider gives us both the country name and its ISO code - use the code directly rather + // than trying to match the name against our own country list. + $country = $address->getCountry(); + + return [ + 'latitude' => $address->getCoordinates()->getLatitude(), + 'longitude' => $address->getCoordinates()->getLongitude(), + 'country_code' => $country ? $country->getCode() : null, + ]; } } return false; } - - public function reverseGeocode($lat, $lng) - { - $json = file_get_contents("https://maps.googleapis.com/maps/api/geocode/json?latlng=$lat,$lng&key=".$this->googleKey()); - - $decoded = json_decode($json)->results[0]; - - return $decoded; - } } From bc584114ea7138ee93cd5fa6695e8303238f2b43 Mon Sep 17 00:00:00 2001 From: edwh Date: Sat, 5 Sep 2026 22:00:29 +0100 Subject: [PATCH 2/3] Don't throw when the provider places nothing AddressCollection::get(0) raises OutOfBounds on an empty collection, so an address Mapbox and GeoPlugin both fail to place crashed the request instead of returning false the way callers expect - and the way the Google implementation did, since it counted results before indexing. Caught by rewriting GeocoderTest onto a bound provider: the nothing-found case throws with the real code. Co-Authored-By: Claude Opus 5 (1M context) --- app/Helpers/Geocoder.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/Helpers/Geocoder.php b/app/Helpers/Geocoder.php index 1418ee935e..e82dd7f947 100644 --- a/app/Helpers/Geocoder.php +++ b/app/Helpers/Geocoder.php @@ -21,7 +21,9 @@ public function geocode($location) GeocodeQuery::create($location)->withData('location_type', [Mapbox::TYPE_PLACE, Mapbox::TYPE_ADDRESS]) ); $addressCollection = $geocodeResponse->get(); - $address = $addressCollection->get(0); + // get(0) throws OutOfBounds on an empty collection, so an address the + // provider can't place would crash rather than returning false. + $address = $addressCollection->isEmpty() ? null : $addressCollection->first(); if ($address && $address->getCoordinates()) { // The provider gives us both the country name and its ISO code - use the code directly rather From 4496eca64e4cfcc77b6a925837db81e4cf073405 Mon Sep 17 00:00:00 2001 From: edwh Date: Thu, 8 Oct 2026 15:31:00 +0100 Subject: [PATCH 3/3] Geocode with Mapbox only: no GeoPlugin fallback, helper absorbs provider errors GeoPlugin geocodes IP addresses, so it could never have placed a street address; the chain only hid that. Without the chain a bad token or an unreachable Mapbox would surface as a 500, so the helper catches and logs it and returns false, as it did when Google was unreachable. CI stubs the geocoder on a missing MAPBOX_TOKEN rather than a missing Google key, and the docs and test comments say Mapbox. Co-Authored-By: Claude Fable 5.1 --- .circleci/config.yml | 6 +-- app/Helpers/Geocoder.php | 46 +++++++++++-------- app/Helpers/StubGeocoder.php | 8 ++-- config/geocoder.php | 22 ++++----- docs/local-development.md | 13 +++--- tests/Feature/Users/EditProfileTest.php | 8 +--- .../Registration/AccountCreationTest.php | 8 +--- tests/Support/GeocoderMock.php | 6 +-- tests/TestCase.php | 6 +-- tests/Unit/GeocoderTest.php | 37 +++++++++++++-- 10 files changed, 90 insertions(+), 70 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 7507114b12..ebaf94f74f 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -48,12 +48,12 @@ jobs: echo "MAPBOX_TOKEN=$MAPBOX_TOKEN" >> .env # CircleCI does not give project environment variables to jobs built - # from forked pull requests, so those runs have no geocoding key. + # from forked pull requests, so those runs have no Mapbox token. # Without this every group creation 422s on geocode_failed and the # whole Playwright suite fails at its first createGroup(), which # reads as the contributor having broken something. - if [ -z "$GOOGLE_API_CONSOLE_KEY" ]; then - echo "No geocoding key available - using the stub geocoder." + if [ -z "$MAPBOX_TOKEN" ]; then + echo "No Mapbox token available - using the stub geocoder." echo "GEOCODER_STUB=true" >> .env fi diff --git a/app/Helpers/Geocoder.php b/app/Helpers/Geocoder.php index e82dd7f947..682e7cdede 100644 --- a/app/Helpers/Geocoder.php +++ b/app/Helpers/Geocoder.php @@ -4,6 +4,7 @@ use Geocoder\Provider\Mapbox\Mapbox; use Geocoder\Query\GeocodeQuery; +use Illuminate\Support\Facades\Log; class Geocoder { @@ -14,27 +15,34 @@ public function __construct() public function geocode($location) { if ($location != 'ForceGeocodeFailure') { - // Use the geocoder configured in config/geocoder.php - a chain of Mapbox (primary) and GeoPlugin - // (fallback), resolved via the container so tests can bind a mock Geocoder instead. This is the - // same pattern used by App\Console\Commands\SetPlaceNetworkData for reverse geocoding. - $geocodeResponse = app('geocoder')->geocodeQuery( - GeocodeQuery::create($location)->withData('location_type', [Mapbox::TYPE_PLACE, Mapbox::TYPE_ADDRESS]) - ); - $addressCollection = $geocodeResponse->get(); - // get(0) throws OutOfBounds on an empty collection, so an address the - // provider can't place would crash rather than returning false. - $address = $addressCollection->isEmpty() ? null : $addressCollection->first(); + try { + // Mapbox, configured in config/geocoder.php and resolved via the container so tests can bind a + // fake in its place. There is no fallback provider. This is the same geocoder that + // App\Console\Commands\SetPlaceNetworkData uses for reverse geocoding. + $geocodeResponse = app('geocoder')->geocodeQuery( + GeocodeQuery::create($location)->withData('location_type', [Mapbox::TYPE_PLACE, Mapbox::TYPE_ADDRESS]) + ); + $addressCollection = $geocodeResponse->get(); + // get(0) throws OutOfBounds on an empty collection, so an address the + // provider can't place would crash rather than returning false. + $address = $addressCollection->isEmpty() ? null : $addressCollection->first(); - if ($address && $address->getCoordinates()) { - // The provider gives us both the country name and its ISO code - use the code directly rather - // than trying to match the name against our own country list. - $country = $address->getCountry(); + if ($address && $address->getCoordinates()) { + // The provider gives us both the country name and its ISO code - use the code directly rather + // than trying to match the name against our own country list. + $country = $address->getCountry(); - return [ - 'latitude' => $address->getCoordinates()->getLatitude(), - 'longitude' => $address->getCoordinates()->getLongitude(), - 'country_code' => $country ? $country->getCode() : null, - ]; + return [ + 'latitude' => $address->getCoordinates()->getLatitude(), + 'longitude' => $address->getCoordinates()->getLongitude(), + 'country_code' => $country ? $country->getCode() : null, + ]; + } + } catch (\Throwable $e) { + // A bad token, an exhausted quota or Mapbox being unreachable is a failed geocode as far as the + // caller is concerned: it becomes a validation message, not a 500. Log it so a misconfiguration + // is still visible. + Log::error("Geocoding '{$location}' failed: " . $e->getMessage()); } } diff --git a/app/Helpers/StubGeocoder.php b/app/Helpers/StubGeocoder.php index 6e4580baa0..1cf8b8f015 100644 --- a/app/Helpers/StubGeocoder.php +++ b/app/Helpers/StubGeocoder.php @@ -3,17 +3,17 @@ namespace App\Helpers; /** - * A Geocoder that answers from a fixed table instead of calling Google. + * A Geocoder that answers from a fixed table instead of calling Mapbox. * * CircleCI does not pass project environment variables to jobs built from - * forked pull requests, so GOOGLE_API_CONSOLE_KEY is empty on those runs. + * forked pull requests, so MAPBOX_TOKEN is empty on those runs. * Every geocode then returns false, group creation 422s on * groups.geocode_failed, and the whole Playwright suite fails at its first * createGroup() - which looks exactly like the contributor having broken * something. This stub keeps those runs meaningful, and keeps the suite off - * the live Google API generally. + * the live Mapbox API generally. * - * Only used when GEOCODER_STUB is set, which CI does when there is no key. + * Only used when GEOCODER_STUB is set, which CI does when there is no token. */ class StubGeocoder extends Geocoder { diff --git a/config/geocoder.php b/config/geocoder.php index 2e65da6ba4..256dfffc76 100644 --- a/config/geocoder.php +++ b/config/geocoder.php @@ -1,7 +1,5 @@ using('google_maps')`. + | Mapbox is the only geocoder. There is deliberately no fallback provider: + | the address geocoder in App\Helpers\Geocoder and the reverse geocoder in + | App\Console\Commands\SetPlaceNetworkData both need a provider that + | understands street addresses, and a chain that fell through to an + | IP-address geocoder could never place one. If Mapbox can't answer, the + | geocode fails and the caller says so. | | Please consult the official Geocoder documentation for more info. | https://github.com/geocoder-php/Geocoder#providers | */ 'providers' => [ - Chain::class => [ - Mapbox::class => [ - env('MAPBOX_TOKEN'), - ], - GeoPlugin::class => [], + Mapbox::class => [ + env('MAPBOX_TOKEN'), ], ], diff --git a/docs/local-development.md b/docs/local-development.md index 364c0671b7..8163228ba6 100644 --- a/docs/local-development.md +++ b/docs/local-development.md @@ -167,19 +167,18 @@ task docker:test:playwright-autocomplete ### Prerequisites for Testing -Before running tests, you need to configure the Google API key for geocoding functionality: +Geocoding (turning an address into coordinates when a group, event or profile is saved) uses Mapbox. The PHPUnit suite binds a deterministic fake geocoder, so it needs no token; the Playwright suite runs against the real application, so it does: -1. **Set Google API Key**: Add a valid Google Maps API key to your `.env` file: +1. **Set the Mapbox token**: Add a Mapbox access token to your `.env` file: ```bash - GOOGLE_API_CONSOLE_KEY=your_actual_google_api_key_here + MAPBOX_TOKEN=your_actual_mapbox_token_here ``` + Or set `GEOCODER_STUB=true` to answer from a fixed table of places instead, as CI does on forked pull requests, which get no secrets. -2. **API Key Requirements**: The key must have the following APIs enabled: - - Geocoding API - - Maps JavaScript API (for location validation) +2. **Google API key**: the maps and the address autocomplete in the browser still use Google, so `GOOGLE_API_CONSOLE_KEY` is needed for those pages to look right, but not for geocoding. > [!WARNING] -> Without a valid `GOOGLE_API_CONSOLE_KEY`, tests that create groups or events will fail with location validation errors. +> Without a valid `MAPBOX_TOKEN` (or `GEOCODER_STUB=true`), Playwright tests that create groups or events will fail with location validation errors. > [!NOTE] > The PHPUnit task will automatically upload coverage to Coveralls if the `COVERALLS_REPO_TOKEN` environment variable is set. diff --git a/tests/Feature/Users/EditProfileTest.php b/tests/Feature/Users/EditProfileTest.php index 534bf17098..14dddb6727 100644 --- a/tests/Feature/Users/EditProfileTest.php +++ b/tests/Feature/Users/EditProfileTest.php @@ -11,7 +11,6 @@ use Carbon\Carbon; use DB; use Illuminate\Foundation\Testing\RefreshDatabase; -use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Storage; use Tests\TestCase; @@ -108,10 +107,7 @@ public function test_location_update(): void $this->assertEquals(51.507, round($user->latitude, 3)); $this->assertEquals(-0.128, round($user->longitude, 3)); - $good = Config::get('GOOGLE_API_CONSOLE_KEY'); - Config::set('GOOGLE_API_CONSOLE_KEY', 'zzz'); - - // Supply the id. + // Supply the id, and a town the test geocoder doesn't know so geocoding fails. $this->post('/profile/edit-info', [ 'id' => $user->id, 'name' => $user->name, @@ -121,8 +117,6 @@ public function test_location_update(): void 'townCity' => 'ZZZZ', ]); - Config::set('GOOGLE_API_CONSOLE_KEY', $good); - $user = $user->fresh(); $this->assertNull($user->latitude); $this->assertNull($user->longitude); diff --git a/tests/Feature/Users/Registration/AccountCreationTest.php b/tests/Feature/Users/Registration/AccountCreationTest.php index 32ecec49a0..b0146264cd 100644 --- a/tests/Feature/Users/Registration/AccountCreationTest.php +++ b/tests/Feature/Users/Registration/AccountCreationTest.php @@ -6,7 +6,6 @@ use App\User; use DB; use Hash; -use Illuminate\Support\Facades\Config; use Mockery; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Tests\TestCase; @@ -49,15 +48,10 @@ public function testRegisterInvalidAddress(): void { $userAttributes = $this->userAttributes(); - // Specify an invalid city and force geocoding to fail by invalidating the Google key. - $good = Config::get('GOOGLE_API_CONSOLE_KEY'); - Config::set('GOOGLE_API_CONSOLE_KEY', 'zzz'); - + // A city the test geocoder doesn't know, so geocoding fails. $userAttributes['city'] = 'zzzzzzz'; $response = $this->post('/user/register/', $userAttributes); - Config::set('GOOGLE_API_CONSOLE_KEY', $good); - $response->assertStatus(302); $response->assertRedirect('dashboard'); $this->assertDatabaseHas('users', [ diff --git a/tests/Support/GeocoderMock.php b/tests/Support/GeocoderMock.php index 1e888d3133..53204507e8 100644 --- a/tests/Support/GeocoderMock.php +++ b/tests/Support/GeocoderMock.php @@ -7,10 +7,10 @@ /** * Deterministic, location-aware Geocoder for the test suite. * - * The real Geocoder makes a live HTTP call to the Google Maps Geocoding API, which is + * The real Geocoder makes a live HTTP call to the Mapbox Geocoding API, which is * rate-limited and non-deterministic. Tests that geocode a location were at the mercy - * of Google's responses — e.g. APIv2GroupTest::testLocales flipped between Belgium and - * the United Kingdom, and the "bad location" tests only passed while Google happened to + * of the provider's responses — e.g. APIv2GroupTest::testLocales flipped between Belgium and + * the United Kingdom, and the "bad location" tests only passed while the provider happened to * reject the gibberish string. * * This fake is bound globally in Tests\TestCase::setUp() so no test hits the live API. diff --git a/tests/TestCase.php b/tests/TestCase.php index 9fc66af8ee..66ea3d7f38 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -101,9 +101,9 @@ protected function setUp(): void $this->withoutExceptionHandling(); app('honeypot')->disable(); - // Bind a deterministic geocoder so the suite never makes live calls to the - // Google Maps Geocoding API (rate-limited / intermittently failing, which - // made any geocoding test flaky). Tests that need the geocode-failure path + // Bind a deterministic geocoder so the suite never makes live calls to + // Mapbox (rate-limited / intermittently failing, which made any geocoding + // test flaky). Tests that need the geocode-failure path // bind their own failing mock in setUp(), overriding this. $this->app->bind(Geocoder::class, function () { return new GeocoderMock(); diff --git a/tests/Unit/GeocoderTest.php b/tests/Unit/GeocoderTest.php index 75b6349388..9519904571 100644 --- a/tests/Unit/GeocoderTest.php +++ b/tests/Unit/GeocoderTest.php @@ -8,7 +8,7 @@ class GeocoderTest extends TestCase { /** * Geocoder::geocode() resolves the provider chain out of the container, so a - * test binds its own in place of the real Mapbox/GeoPlugin pair. Nothing here + * test binds its own in place of the real Mapbox provider. Nothing here * touches the network: these tests are about how we read a provider's answer, * not whether Mapbox is up. */ @@ -18,7 +18,7 @@ private function bindGeocoderReturning(array $addresses): void // instance(), not bind(): geocoder-laravel registers a singleton, and by // the time a test runs it may already be resolved - in which case bind() - // is ignored and the real Mapbox chain answers. That failure looks like a + // is ignored and the real Mapbox provider answers. That failure looks like a // wrong country code rather than a network call, so it is worth being // explicit about. $fake = new class($collection) @@ -51,12 +51,33 @@ public function get() // Both keys: geocoder-laravel binds the string 'geocoder' through to // ProviderAndDumperAggregator, which is itself a singleton, so replacing - // only the string leaves the real chain reachable - and a test that + // only the string leaves the real provider reachable - and a test that // "passes" then is really talking to Mapbox. $this->app->instance('geocoder', $fake); $this->app->instance(\Geocoder\Laravel\ProviderAndDumperAggregator::class, $fake); } + private function bindGeocoderThrowing(\Throwable $e): void + { + $fake = new class($e) + { + private $e; + + public function __construct($e) + { + $this->e = $e; + } + + public function geocodeQuery($query) + { + throw $this->e; + } + }; + + $this->app->instance('geocoder', $fake); + $this->app->instance(\Geocoder\Laravel\ProviderAndDumperAggregator::class, $fake); + } + private function address(array $over = []): Address { return Address::createFromArray(array_merge([ @@ -108,6 +129,16 @@ public function testGeocodeCopesWithAnAddressCarryingNoCountry(): void $this->assertEquals(round(51.4643585, 2), round($ret['latitude'], 2)); } + public function testGeocodeReturnsFalseWhenTheProviderFails(): void + { + // With no fallback provider there is no chain to swallow a bad token or + // an unreachable Mapbox: the helper has to, so the caller gets a failed + // geocode (a validation message) rather than a 500. + $this->bindGeocoderThrowing(new \Geocoder\Exception\InvalidCredentials('Invalid token')); + + $this->assertFalse((new \App\Helpers\Geocoder)->geocode('6 Canterbury Crescent')); + } + public function testForcedFailureShortCircuits(): void { $this->bindGeocoderReturning([$this->address()]);