doublesecretagency / craft-googlemaps

Google Maps plugin for Craft CMS - Maps in minutes. Powered by the Google Maps API.
https://plugins.doublesecretagency.com/google-maps/
Other
10 stars 8 forks source link

Consider dropping the default `range` #68

Closed MoritzLost closed 3 months ago

MoritzLost commented 1 year ago

Right now, the only way to order an entry query by the distance to a custom location is to use a proximity search as mentioned in the docs. As far as I can tell, this always limits results to a specified range. There's no way to only order by distance without limiting the results to a specific radius around the target location.

I have a use-case where I want to order entries by their distance to a custom location, but don't want to exclude any entries from the query based on their distance. This looks like it should do the trick:

$results = Entry::find()
    ->myAddressField(['target' => ['lat' = 5, 'lng' => 5]]
    ->orderBy('distance')

But in reality, this will apply the default distance of 500 miles to the query and exclude any results that are farther away. This is unintuitive, and as far as I can tell the only way to prevent this is to set an arbitrarily large range like 9999 to prevent entries from being excluded. This is a bit ugly and might also result in worse performance (as opposed to not having a condition for the distance in the query at all).

I would argue in favour of dropping the unintuitive and arguably useless default of 500 miles. Instead, if the range is not specified, the query should only include the distance calculation, but not add any additional distance conditions.

lindseydiloreto commented 1 year ago

This is an interesting suggestion. The default range exists as a layer of protection, to ensure that people don't generate enormous queries. But perhaps it is overkill at this point.

We'll explore the idea of dropping the default range. It's probably pretty easy to do on a technical level.

However, we would probably hold off on releasing this until the next major version. It could be considered a breaking change, since some people may be relying on the default behavior.

Thanks for the recommendation. We'll do a little research, and probably make this change in v5.0.

MoritzLost commented 1 year ago

Definitely a good candidate for a breaking change! I don't think too many people are relying on the implicit default – 500 miles is way too broad to meaningfully limit the results if you've got thousands of locations all over the world, and for stuff like local store locators it won't limit results at all. All of our sites using the plugin either limit the query by an explicit range or don't include a range at all as it's not relevant to the query.

We'll explore the idea of dropping the default range. It's probably pretty easy to do on a technical level.

I think it would just require dropping the default in ProximitySearchHelper::_applyProximitySearch and only adding the having or andWhere conditions if the range is explicitly passed.

lindseydiloreto commented 1 year ago

Great, thanks @MoritzLost. I believe you are correct about all of that. 🙂

lindseydiloreto commented 3 months ago

Good news, this is now the default behavior in v5.0.0. Thanks again for the suggestion!

MoritzLost commented 3 months ago

@lindseydiloreto That's great, thanks Lindsey!