Skip to content

RES-1858 Switch to use mapbox for geocoding#644

Open
ngm wants to merge 1 commit into
developfrom
RES-1858_mapbox_geocode
Open

RES-1858 Switch to use mapbox for geocoding#644
ngm wants to merge 1 commit into
developfrom
RES-1858_mapbox_geocode

Conversation

@ngm

@ngm ngm commented May 3, 2023

Copy link
Copy Markdown
Contributor
  1. Add configuration for Mapbox token. The Google API key is still used for the maps (which haven't yet been replaced) so that stays.
  2. Switch to use the standard PHP geocoder and add a basic test..
  3. For both group and event add/edit, remove use of vue-google-autocomplete and instead use the Mapbox one.

(new PR after cherry picking some files for #640 )

@sonarqubecloud

sonarqubecloud Bot commented May 3, 2023

Copy link
Copy Markdown

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 2 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.6% 0.6% Duplication

@sonarqubecloud

sonarqubecloud Bot commented Aug 7, 2023

Copy link
Copy Markdown

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 2 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.6% 0.6% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

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 <noreply@anthropic.com>
@edwh
edwh force-pushed the RES-1858_mapbox_geocode branch from 18af27c to fcfbcc0 Compare July 15, 2026 09:02
@edwh

edwh commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Rebuilt this branch on current develop (it was ~1240 commits behind, from before CI was reworked, so no checks were running at all).

While rebasing this, it turned out most of the Mapbox plumbing this PR wanted had already landed on develop separately: config/geocoder.php (Mapbox + GeoPlugin chain), the geocoder-php/mapbox-provider / toin0u/geocoder-laravel packages, and MAPBOX_TOKEN in .env/CircleCI/Fly secrets are all already there and already used for reverse geocoding in App\Console\Commands\SetPlaceNetworkData.

So the only remaining piece of this PR's intent was the forward geocode path, App\Helpers\Geocoder::geocode() (used by group/event/user location editing), which was still calling the Google Maps HTTP API directly. That's now switched to app('geocoder'), the same Mapbox-then-GeoPlugin chain, matching the pattern already used by SetPlaceNetworkData. country_code is now taken directly from the provider's ISO country code rather than matching the country name against our own list. The unused reverseGeocode() method is removed (nothing calls it any more; reverse geocoding goes through SetPlaceNetworkData's direct reverseQuery() call). The public interface, container binding, and existing test mocks (GeocoderMock, GeocoderFailMock) are unchanged, and tests/Unit/GeocoderTest.php on develop already expects exactly this interface, so no other files needed changes.

Infra/build files (Dockerfile, docker-compose.yml, docker_run.sh, package.json/package-lock.json) are taken as-is from develop - the versions in this branch were three years stale and would only have caused conflicts.

Not carried over: the third part of the original PR description - replacing vue-google-autocomplete with a Mapbox GL JS geocoder widget in GroupLocation.vue/VenueAddress.vue. Those two components have been substantially reworked on develop since this branch was created (a shared PlacesAutocomplete.vue wrapper, Leaflet-based map display, and Playwright-testability watchers that are specifically built around the current Google Autocomplete widget's DOM behaviour). Redoing that swap properly is a real frontend rewrite in its own right, not a rebase, and risks breaking the existing Playwright coverage for group/event location editing. Recommend splitting that into its own follow-up PR if it's still wanted, rather than bundling it here.

Needs manual verification:

  • A live Mapbox geocode against a real MAPBOX_TOKEN - CI should exercise this via tests/Unit/GeocoderTest.php (the CircleCI secret should be a real token; local dev's .env only has the placeholder 1234).
  • That the production/Fly MAPBOX_TOKEN secret is actually set (docs say it should be, as a shared secret, but worth double-checking before this merges).

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants