Problem/Motivation

Previously, when we tried to geocode an invalid address, we had some kind of feedback:

[warning] Could not execute query "https://maps.googleapis.com/maps/api/geocode/json?address=ADDRESS&language=&region=&key=APIKEY".

With the 3.x version, the only feedback we get is an empty string:
[error]

Plus, when there are more than one provider, errors are thrown even if one of them succeed. It can lead to misunderstanding when trying to understand what happened.

Proposed resolution

  1. Add more feedback in the exception thrown when the geocoding or the reverse geocoding gave no result.
  2. Only log the errors if no provider can handle the data.
CommentFileSizeAuthor
#3 geocoder-3164356-3.patch950 bytesitamair
#2 geocoder-3164356-2.patch1.85 KBduaelfr

Comments

DuaelFr created an issue. See original summary.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Status: Active » Needs review
StatusFileSize
new geocoder-3164356-2.patch1.85 KB

With this patch, the errors look like:

[error]  Unable to geocode "ADDRESS" with the googlemaps provider.
itamair’s picture

itamair commented
StatusFileSize
new geocoder-3164356-3.patch950 bytes

thanks @DuaelFr for this. I made some better testing and checked that with this simpler patch the same exceptions logs will be triggered/reported but with less code
Going to commit this into dev, but crediting you anyway for that.

  • itamair committed 627b542 on 8.x-3.x authored by DuaelFr 
    Issue #3164356 by DuaelFr, itamair: Provide a better feedback when...
itamair’s picture

itamair commented
Status: Needs review » Fixed
duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented

Thanks a lot! I might open a follow-up for those errors.

The thing is that if you have more than one provider, the geocoding/reverse can fail for one of them but work for the other one. In the end, the coordinates/address has been found. If the error message is shown to the end users, they could think something wrong happened when it's not the case. If the operation runs inside a CI (that's my case), the build can fail because an error has been raised when there was no reason to.
That's why I added the code to show the error only if no provider had succeeded.
What do you think about it? Should I open another issue?

itamair’s picture

itamair commented

For what I have tested with the actual code the error notice will be shown to the user only if none of the Geocoder Providers succeded ("Unable to Geocode [location]"). Only the DB log will be written for each failed geocoder provider.
This kind of behaviour LGTM ...
If you don't think so (or I am missing something) feel free to open a new issue, detailing the specific use case you want to fix, and how to reproduce it, and eventually post your new patch.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.