I was working recently with a project that uses geocoder, but the data that it was being given by users was a URL (ok, it wasn't well set up, but that's not the point here).

I was getting lots of errors from google and things were taking a while, so I looked further. I found that the function geocoder_google() was assuming that everything it was given was to be sent to google, and yet the error handling wasn't great.

This report provides a patch that:
- returns from geocoder_google() if the provided address is empty, or if it looks like a URL.
- in case of google returning an error, includes a bit more info in the message about what went wrong.
- changes a return; to return false; to make the return value consistent.
- fixes a spelling error.

I'm not 100% confident that this is all there is to do: in particular, I can see that my check for the address being a URL could be too wide a scope, but I'm not sure where that rabbit hole leads.

CommentFileSizeAuthor
#4 improvements_to-2536624-4.patch1.1 KBfietserwin
0001-Make-geocoder-more-robust-and-useful-when-presented-.patch2.94 KBrivimey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

GemQueen’s picture

GemQueen CreditAttribution: GemQueen as a volunteer commented

thank you so much for this! I applied the patch, and it seems to be working for me.

I think this might be higher priority than minor since Google is requiring API key access and is monitoring usage. Being able to prevent unnecessary calls to the google geocoder could help with a site's quota.

Pol’s picture

Pol
the/that
Location Brussels
CreditAttribution: Pol commented
Status: Active » Needs work

Hi,

Thanks for the patch.

Could you tell me why you're doing this:

     if (empty($geometries)) {
-      return;
+      return FALSE;
     }

Thanks.

rivimey’s picture

rivimey
she/her
CreditAttribution: rivimey commented

Hi Pol, it was a year ago so I'm not sure, however I believe the motivation for change was, as I said in the report, "changes a return; to return false; to make the return value consistent.", that is, so that you can store the result and know you're not storing a NULL.

I believe that $geometries empty was an error indication, and NULL is 'falsy', so FALSE seemed to be the right value.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin as a volunteer commented
Status: Needs work » Needs review
FileSize
improvements_to-2536624-4.patch1.1 KB

I also ran into some errors and wrote my own error reporting code. After solving that, I wanted to post my changes as a patch and found this issue. I still post my patch here as the new version because:
- I reduced the scope to better error reporting only. We could log the address or even better yet, the full URI, but that could lead to privacy sensitive data in the log. So only add the error message from Google.
- The return false I kept as I prefer that functions in all cases return a value or return nothing at all. Function geocoder_cache_set() documents that it expects a FALSE or a geometry, not void/NULL.
- The typos are already fixed.
- I think that checking for an empty address should not be done by the plugin but before, in code that is executed for all plugins.

So please find attached a new version of the patch for this issue.

helmo’s picture

helmo CreditAttribution: helmo at Initfour websolutions for Aegir Cooperative commented
Status: Needs review » Reviewed & tested by the community

Thanks, I also found this after having developed a similar patch

Pol’s picture

Pol
the/that
Location Brussels
CreditAttribution: Pol commented
Assigned: Unassigned » Pol

  • Pol committed d176d7e on 7.x-1.x authored by fietserwin 
    Issue #2536624 by fietserwin: Improvements to geocoder_google() data and...
Pol’s picture

Pol
the/that
Location Brussels
CreditAttribution: Pol commented
Status: Reviewed & tested by the community » Fixed

Thanks.

Status: Fixed » Closed (fixed)

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