Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey’s picture

Status: Active » Needs review
FileSize
548 bytes

Attached is a (pretty trivial) patch.

attiks’s picture

FileSize
862 bytes

I needed support for filtering as well

drunken monkey’s picture

Why not just assume that the passed parameter is already in the format used by Google?

Either way, your current code shouldn't be committed, as it uses a closure and PHP 5.3 isn't required for Drupal 7.

attiks’s picture

#3 For a user it's much nicer/easier to write

  $point = geocoder('google', $locality, array(
    'components' => array(
      'country' => $country,
      'locality' => $locality,
    ),
  ));

instead of (untested)

  $point = geocoder('google', $locality, array(
    'components' => "country=$country|locality=$locality",
    ),
  ));

Once we agree on the format of the parameter I'll reroll for the closure

drunken monkey’s picture

OK, that of course makes sense. In the end, the maintainer(s) will have to decide this, I guess, so you should probably just do a re-roll now and wait for feedback.

attiks’s picture

FileSize
1.26 KB

new patch without closure

drunken monkey’s picture

Just realized that plugins have settings forms! Don't know how that escaped me previously.
So, adding bounds to the plugin settings, too.

drunken monkey’s picture

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, do we need a setting for the components as well?

drunken monkey’s picture

Status: Reviewed & tested by the community » Needs review

Would probably be good, yes. I don't know if it will be committed without them.

drunken monkey’s picture

Status: Needs review » Reviewed & tested by the community

Oops, didn't mean to change the status.

esclapes’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.23 KB

I was about to open a new feature request when saw your patch and was really happy to see how easy was to do this. I was looking for the region bias (without knowing it) and thought that it would make sense to implement both Bounding Box and region as different bias options, and let the user decide (with a select).

drunken monkey: very sorry for highjacking your issue, not sure if this is good etiquete or just rude. Let me know if you are happy with the result. I did it with the best intention. I would be happy to move my patch to a different issue if you prefer so.

attiks: I wasn't sure about what to do with filters, because they behave in a different way (filtering vs biasing) and because there are different filters available, options form will also be a bit more complex than what is necessary for biasing. How do you see it fit?

Patch attached. Change to needs review, although changes are not really that big

PS: Looking at the patch it seems my texteditor changed some lines (whitespace or tabs, i suppouse)

attiks’s picture

#12 I'm using this from code, so I have no idea what kind of settings form is needed, I think it will be complex and with not that much of benefits. My use case was to reliably search for cities in certain countries, either by postal code or by city name, without filters I sometimes got strange results, so that's why I added them.

I think we should support both options (as in your patch) but without adding too much settings forms

drunken monkey’s picture

Status: Needs review » Needs work

No problem, your patch is a valuable addition to this issue and I'm sure we should rather get this right the first time than later come and change the options again. Your changes make sense to me, could be useful.
However, I think it might be a bit unclear to end users that you enter the region into the field named "Bounding box". You should change the name and description of that form element, too, in my opinion.

drunken monkey’s picture

Re-roll of #7, as that one doesn't apply anymore.
Would still need a config form for the components parameter, in my opinion, so leaving at “needs work”.

attiks’s picture

FYI: the components is rather complex, so no idea if a settings form is desirable, I think most people who are going to use this, will use it from code, docs at https://developers.google.com/maps/documentation/geocoding/#ComponentFil...

esclapes’s picture

I just opened an issue to address this without bloating the interface. #2019419: Adding extra query parameters to google geocoder

May be you can help review it.

attiks’s picture

#17 Your approach in #2019419: Adding extra query parameters to google geocoder is a bit different, it means you have to construct the query outside of geocoder, making it harder for people to use.

I still think adding an UI for this is off topic and not really needed, but that's just my 2c

andrewbelcher’s picture

Status: Needs work » Needs review
FileSize
2.61 KB

Hope you don't mind me weighing in with another subtly different patch. This one also includes support for region biasing and simplifies the UI by putting it into a collapsed fieldset.

However, I have stripped out some of the processing of the options to keep the form simple, which means that people configuring (either by form or code) have to pass in the correctly formatted parameters as per the linked documentation.

Sam152’s picture

Issue summary: View changes
FileSize
3.24 KB

#19 no longer applied for me. I have rerolled and attached a new patch.

Sam152’s picture

For adding region biasing to the geofield views plugin: #2155799: Add support for region biasing in views

Paul Lomax’s picture

As a fairly quick addition you can use without patches, this works.

/**
 * Implements hook_url_outbound_alter().
 */
function MODULE_url_outbound_alter(&$path, &$options, $original_path) {
  // Modify reguests to Google geocoder API so we focus on results in the UK.
  if ($path == 'http://maps.googleapis.com/maps/api/geocode/json') {
    $options['query']['region'] = 'UK';
  }
}
pcambra’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.62 KB

Reroll of the patch, setting as RTBC because solves the issue described

Simon Georges’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks for the reroll and the review. If you find the time to review other patch, I'd be happy to commit them ;-)

pcambra’s picture

Thanks! :)

Status: Fixed » Closed (fixed)

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

fonant’s picture

Thanks Paul for #22, this works for geofield proximity searching too.

You do need to manually empty the cache_geocoder table in the database, though, to see the improvement.