Skip to content

Conversation

@MattKetmo
Copy link
Contributor

One thing I'm not sure to understand is why there are both Geocoder and Provider interfaces.

Those 2 interfaces are almost identical (geocode() vs getGeocodedData() / reverse vs getReversedData()) and then bring no real value.

What I'm suggesting here is to make the Providers extending the Geocoders. Thus the ProviderBasedGeocoder just acts as an aggregator and each provider can be used individually as a valid Geocoder instance.

It facilitates the use of specific provider options and then it allows to bypass the level of abstraction I described at #322 (comment), which is for me the most blocking thing that prevent me from using this lib correctly.

In this PR I only fixed the GoogleMaps provider for the demo, and add a setRegion() method to it:

$adapter = new Ivory\HttpAdapter\CurlHttpAdapter();
$geocoder = new Geocoder\Provider\GoogleMaps($adapter);

$data = $geocoder->geocode('Toledo');
echo $data[0]->getCountry()->getCode(); // US

$geocoder->setRegion('ES');
$data = $geocoder->geocode('Toledo');
echo $data[0]->getCountry()->getCode(); // ES

Let me know what you think about it.

@willdurand
Copy link
Member

Really interesting, thank you for this proposal, I like it!

@willdurand willdurand added this to the 3.0.0 milestone Oct 28, 2014
@toin0u
Copy link
Member

toin0u commented Oct 31, 2014

Very cool ! I like it as well 👍

@MattKetmo
Copy link
Contributor Author

In fact I wonder if it's worthwhile to keep the Provider interface. We could drop the getName() method and specify it into the ProviderBasedGeocoder::registerProvider($name, Geocoder $geocoder) method (which could be renamed AggregateGeocoder::registerGeocoder($name, Geocoder $geocoder) ?).

@willdurand
Copy link
Member

I was thinking about the AggregateGeocoder (or something like registry/aggregator/etc.)

@willdurand
Copy link
Member

I don't like the registerProvider($name, $geocoder) signature, in fact, I'd like to avoid people to specify a "name"..

@willdurand
Copy link
Member

Please, see #359

@willdurand willdurand closed this Nov 2, 2014
willdurand added a commit that referenced this pull request Nov 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants