Use Google geocoding components filter for better results with addressfield

This could probably be adapted to work for Mapquest Nominatim service as well, if someone wants to take a look at it.

Thanks for your work on this!

After reviewing and testing a bit, I think the approach needs to be modified... The Component Filtering in the Google Geocoding service aren't meant to be used instead of an address query, but in combination with one, at least if you want non-approximate results. Component filtering only supports a very small subset of address components, and as such won't return accurate results if it's used without an address query. See https://developers.google.com/maps/documentation/geocoding/#ComponentFil.... Specifically, it only supports the following components: administrative_area, route, postal_code, country, and locality. So, I modified the patch to only use administrative_area, country, and locality components, and then I pass the address query along with this data. This fixes the wrong country issue (I was getting Romanian addresses returned as if they were in Iowa), but still returns accurate rooftop-level results.

I also rolled the patch with relative paths, which should make it easier to apply.

Hello,

I just tested the above patch (#5) and everything seems to work as expected.

Thanks!

Ok, let met test that, and I'll commit it, thanks for all the work!).

Patch #5 seems to help with the issues we started to see after moving from Geocoder 7.x-1.2 to 7.x-1.x-dev. The geocoding in 1.x-dev returns very unpredictable results, and if an address is not typed a very specific way, Geocoder will generate a point hundreds of kilometers away.

Only issue though is the patch does not apply cleanly to 7.x-1.x-dev. Was this written for 7.x-1.2, and will be moved to a -dev version?

Attached is a re-roll of the patch against the current 7.x-1.x-dev and an interdiff.

The one area that seems confusing is the code added in geocoder_google. There is already some new code in 7.x-1.x-dev listed under the comment of "Add any given biasing" which sets the value of $query['components'].

Then immediately following that is the code included in this patch, which if $components is not empty, the value of $query['components'] is reset. Also right after this is more code that resets $query['region'] if $geocoder_settings['geocoder_region_google'] is set.

Should these blocks of code be combined into one conditional?

The way the code is structured right now in geocoder_google, $components takes precedence over $options. If that is the case, I'd think the code in that block should be as follows. Thoughts?

    // Add any given biasing.
    if (!empty($options['biasing']['bounds'])) {
      $query['bounds'] = $options['biasing']['bounds'];
    }
    $geocoder_settings = variable_get("geocoder_settings", array());
    if (isset($geocoder_settings['geocoder_region_google']) && isset($geocoder_settings['geocoder_region_google'])) {
      $query['region'] = $geocoder_settings['geocoder_region_google'];
    }
    elseif (!empty($options['biasing']['region'])) {
      $query['region'] = $options['biasing']['region'];
    }
    if (!empty($components)) {
      foreach ($components as $key => $value) {
        $components[$key] = $key . ':' . $value;
      }
      $query['components'] = implode('|', $components);
    }
    elseif (!empty($options['biasing']['components'])) {
      $query['components'] = $options['biasing']['components'];
    }

After using patch #9 for the past couple of weeks, it's clear the comments mentioned in #10 regarding the conditional need to be incorporated.

Attached is a new patch for review -- this seems to be working fairly well.

+++ b/geocoder.admin.inc
@@ -10,30 +10,42 @@
-    '#description' => t('You can obtain a Yahoo PlaceFinder consumer key at') . ' ' . 'http://developer.yahoo.com/geo/placefinder/',
+    '#description' => t('You can obtain a Yahoo PlaceFinder consumer key at') . ' ' . l($yahoo_url, $yahoo_url, array('external' => TRUE)),
...
-    '#description' => t('You can obtain a Yandex API Key at ') . 'http://api.yandex.ru/maps/getkey.xml',
+    '#description' => t('You can obtain a Yandex API Key at ') . l($yandex_url, $yandex_url, array('external' => TRUE)),

Might as well fix these broken translations.

t('The text to a !link', array('!link'=> l(...)));

Gives context to the translators and won't change the string every time the URL changes.

Thanks for the feedback. I've created a new patch including feedback from @GaëlG and @joelpittet. I also included the patch I posted here (https://www.drupal.org/node/2499771)... it's a simple mapquest typo that really should be fixed.

See attached and let me know if any other thoughts.

@joelpittet, thank you for taking care of the typo in #2499771. Even though a simple fix, I prefer not to mark my own patches as reviewed.

Attached is a new version of the patch for review that does not include the mapquest fix.

Given the recent release of 7.x-1.3 and focus on the 2.x branch, I'm not certain if anyone still wants this patch? I've created a new version and it is attached for review.

I also separated the broken URL translations referenced in #13 into its own issue: https://www.drupal.org/node/2664294

#16 doesn't apply to 1.x-dev dd 5 June 2016

I've been updating this patch for over a year with no feedback. It would be good to hear from the module maintainers if this is something they plan to review and consider to be committed.

#1 I agree. I've tested this, albeit briefly. The functionality is needed but doesn't currently apply. I might not be a module maintainer but I'll gladly review your patch.

Feel free to review what exists in #16. I haven't tried it out, but might be able to still apply the patch manually to test it.

Updated patch. The only difference is the try function was removed from geocoder_google in 7.x-1.x-dev. Otherwise the patch is the same.

I've taken a quick look at the patch, generally it looks good. I've tested some basic google geocoding results. Geocoding works pretty well, but there's not a great difference between my previous geocoded results and my previous results. Couldn't test mapquest stuff. Component splitting is only specific to this service. And there is some further work to do in general.

• The $components parameter in geocoder_google() is not used. It is in the geocoder_mapquest_nominatim() function. Incomplete?
• I'd prefer a solution where there is only 1 parse_addressfields function instead of a lot of duplicate code. There is already a geocoder_widget_parse_addressfield() function. Let's refactor that instead of duplicating code.
• I think the region bias should be a widget setting, not a global setting. This way you can have field specific bias.

So, probably not the review you were hoping for but at least this is moving again. I'll see if I can add a patch myself.

$components parameter can be removed. If you read comment #12 above, it should have been removed then.

As for refactoring or adding more widgets, you can change whatever you like.

Okay, I've got a new patch. Please review. For clarification:

• Region biasing is out completely, because it already exists in the field options. See the fieldset "RESULT BIASING" for details.
• $components param on geocoder_google function is removed
• For country retrieval on addressfield parsing I've included this patch: https://www.drupal.org/node/2245189#comment-11284545
• There is now one addressfield parsing function. This makes more sense than duplicating code for parsing and preparing conjointly. To generate the components or process the address in a country specific address format order, you can submit an array of elements to the addressfield_parser function.

Marking as related to #2859851 although I'm not sure it would solve this particular issue.
I like the approach of https://www.drupal.org/project/geocoder/issues/2859851 because it leverages the addressfield module to produce correctly formatted addresses.

Hello,

What is the status of this patch ?

Thanks!

The status? It needs review, and now because of the new release, it needs work. There needs to be someone other than me and anyone else contributing to the patch to actually try it and see if it works as expected.

Once again, another geocoder patch that sits idle for years, and ultimately a new release is made that doesn't include it. Extremely frustrating.

The patches in both #21 and #24 no longer apply cleanly to the most recent release, so someone is going to need to roll a new version for review.

Please delete comment