search

geocoder-php / Geocoder

The most featured Geocoder library written in PHP.
https://geocoder-php.org
MIT License
3.95k stars 516 forks source link

Google Maps component filtering #896

Closed lswinblad closed 5 years ago

lswinblad commented 5 years ago

Hi!

First of all, thanks for a great library.

I noticed that component filtering was missing for the Google Maps provider, so i took the liberty of implementing it. Not sure if this can cause any issues with backwards compatibility.

jbelien commented 5 years ago

Hello @Farskies ,

Thanks a lot for your contribution ! 👍

I'll have a look at your PR ASAP ! It looks very interesting !

Could you first make your PR pass the Travis build ? Thanks.

lswinblad commented 5 years ago

Hi!

I've rebased master to fix the failing tests. I've also added additional tests to check that the actual uri format is correct.

I'm not sure about the test using the real API, since it might not be reliable.

Tell me if you want anything changed.

lswinblad commented 5 years ago

You're right. If you want the library to either throw an exception on invalid keys or just plain out filtering them out, that's definitely possible.

Another possibility would be letting the API handle the invalid keys. That way, the library doesn't need to be updated when keys are added and/or changed. I'm unsure whether the API silently filters invalid keys or returns an error response, but I'll investigate it.

Which do you think is better?

jbelien commented 5 years ago

If the API handles correctly (silent filter) I guess that's okay to let the Geocoder Provider send everything without any check. But if the API returns an error if there is an unexpected component, I think it's better to filter it before sending the request to the API.

@Nyholm @willdurand what's your opinion on this ? :)

jbelien commented 5 years ago

@Farskies Did you have time to check how the API handles invalid keys ?

Thanks !

lswinblad commented 5 years ago

I've been super busy lately, but I'll have a look at it today and update the pull-request accordingly!

lswinblad commented 5 years ago

Hi @jbelien!

Sorry for the big delay. I've confirmed that the Google Maps API silently filters out any invalid component names.

curl -G --data-urlencode "address=high st hasting" --data-urlencode "components=country:GB|invalid:invalid" --data-urlencode "key=x" https://maps.googleapis.com/maps/api/geocode/json

Returns the same data with and without the invalid component name.

I've also removed .idea from the gitignore, and rebased my branch.

jbelien commented 5 years ago

Awesome ! Thanks a lot @Farskies ! 👍

I'll review it and merge it ASAP :)