To me, I would not add all these interfaces, because there are not different implementations for most of them (example theBoundInterface but also the collection one). If we provide a way to add user/custom data in an address object (from a provider), then there is no need for extra interfaces, because no one will change that.

I get that interfaces are often good for coupling, but there I don't really see the use case honestly (and I likethe SOLID stuff). Of course, this is my very own point of view.

To be more SOLID our value objects should have interfaces

Usually, interfaces for VOs is a sign of bad design.

Value objects are self-sufficient and very definite from behaviour point of view. There is no place for multiple behaviours here.

This will allow others to write providers that may return more data than we have defined in our models.

It makes client-code coupled with specific implementation and it defeats the original purpose of the abstraction.

It makes sense to ask what exactly@egeloen wants to see in theAddressCollection.

Thank you for valid input.. I've continued the work on this.. Will finish this week.

I'm done with this PR now and would like to have feedback.

The major changes I've done is to introduce aGeocoderResult and aPosition interface. Those are implemented by theAddressCollection andAddress object respectively.

I've also make sure that thePosition::getCoordinates,Position::getCountry andPosition::getBounds always returns an object. These objects have aisDefined function.

I think it is a good idea to removePosition::getLatitude,Position::getLongitude andPosition::getCountryCode because they are redundant.

I decided not to add interfaces for Bounds, Coordinates, Country, AdminLevel and AdminLevelCollection. I assume (maybe falsely) that they are not subject to change. Ever... Please challenge me on this if you feel it's wrong.

I would like to have much feedback on this. What do@egeloen and @geocoder-php/geocoder think?

Future work:

One could add vendor specificPositions ieGooglePosition to include some extra data just available for Google. (Suggested by@unkind here:#523 (comment))

One could add vendor specific Positions ie GooglePosition to include some extra data just available for Google

I'm not sure I understand how it is supposed to work. Interface is limited to specific method list, there is no way to get an extra data.if ($position instanceof GooglePosition)? Well, at least, it should not be called "to be more SOLID", but "we make this hack for practical purposes". :)

Suggested by@unkind here:#523 (comment)

I suggested to make completely independent interfaces (or even classes), e.g.

interface GoogleGeocoder{/**     * @return GoogleAddress[]     */publicfunctiongeocode(...);}

I mean, if you really want to provide vendor-specific address objects, you would have almost same amount of work in terms of lines of code (at least, I think so), but you'd have clean API for every major providers.

I've also make sure that the Position::getCoordinates, Position::getCountry and Position::getBounds always returns an object. These objects have a isDefined function.

Offtopic:isDefined looks nasty, it's like having$datetime->isValid(). You either have valid object (with valid lat/lng) or you should not have it at all. Invalid object doesn't make sense.

To make clear my point:

finalclass GoogleGeocoder{publicfunction__construct(bool$useSsl, ...)    {    }publicfunctiongeocode(string$address):GoogleAddress    {// ...    }publicfunctiongeocodeWithin(string$address,BoundingBox$bbox):GoogleAddress    {// This method is possible, because Google API allows this kind of query    }}final GoogleMapsimplements Provider{publicfunction__construct(GoogleGeocoder$googleGeocoder)    {// ...    }publicfunctiongeocode()    {// Converts GoogleAddress[] to AddressCollection    }}

Users will configure theGoogleGeocoder and use it directly if they need some Google-specific methods/data;GoogleMaps will be simple adapter for genericGeocoder, it works as usual.

I'm not sure I understand how it is supposed to work. Interface is limited to specific method list, there is no way to get an extra data.

The idea is that you can always be sure thatGeocoder::geocode will give you aGeocoderResult withPositions. That is a contract all geocoders MUST follow. There is nothing that forbids theGoogleGeocoder::geocode to return an object like:

class GoogleGeocoderResultimplements GeocoderResult {// ... All method in the GeocoderResult interfacepublicfunctiongetQueryTime() {// Return the time it took to query the API  }}

And thePosition object returned from GoogleGeocoder could also include more methods in the same way.

if ($position instanceof GooglePosition)?

Yes, you would have to doif ($position instanceof GooglePosition). But introducing interfaces like this would allow us to create other vendor specific interfaces.

interface GoogleGeocoder implements Geocoder{/**     * @return GoogleGeocoderResult[]     */publicfunctiongeocode(...);}interface GoogleGeocoderResult implements GeocoderResult{// ...publicfunctiongetQueryTime();}

If an application author decides that he is only interested in using the Google geocoder he/she can type hint directly at theGoogleGeocoder interface. and noif ($position instanceof GooglePosition) is needed. At the same time third party libraries can use the same geocoder because it will still respect theGeocoder interface.

Well, at least, it should not be called "to be more SOLID", but "we make this hack for practical purposes". :)

I can rename the issue title if you want =)

Offtopic: isDefined looks nasty, it's like having $datetime->isValid(). You either have valid object (with valid lat/lng) or you should not have it at all. Invalid object doesn't make sense.

Im not happy with that function either. It did already exist in theBounds object.

There is nothing that forbidsGoogleGeocoder::geocode to return an object like

Sure, but you open this knowledge to users like new kind of API, you legalize the magic withinstanceof.

But introducing interfaces like this would allow us to create other vendor specific interfaces.

I wouldn't try to make inheritance tree like this. In practice, it becomes harder to understand what's going on and reduces flexibility: you can't change contracts of different providers independently. You lock your provider with methodgetCountry: country { code, name }, but what if some provider is allowed to provide more info of a country? In my example, I'd haveGoogleMaps/Country value object with all possible country's data. Sure, you can add justgetExtraCountry method which returnsGoogleMaps/Country or something like this, but it hurts API. Just think aboutGoogleGeocoder interface like you want to build it from scratch. Do you need to inheritGeocoder to limit yourself? I don't think so.

Honestly, I'm not user of the library, so it's not fair for me to judge it. I can find reasons why it's not good in theory, but your approach may be way more practical.

I like having this discussion with you. Thank you for sharing your thoughts.

The purpose of this library is to have a common contract for the geocoded result. If one feel that geocoder X is giving me poor results one should easily be able to switch to geocoder Y. (This is the Liskov substitution principle.) This is something we strongly believe in and should not be up for change. This contract is enforced byGeocodedResult interface (currently withAddressCollection object).

The reason I introduce the interfaces is that I want to open up the possibility for others to create a more specific result. If the application author feels it is important, he/she can "trade" Liskovs substitution principle to have this specific result. This will increase flexibility of the library.

At the same time, he/she can still use theAwesomeGeocoderResultLogger::log(GeocodedResult $result) and all other possible third party libraries and/or plugins.

I wouldn't try to make inheritance tree like this. In practice, it becomes harder to understand what's going on and reduces flexibility.

The inheritance if only for interfaces. And yes, it reduces flexibility because we want to follow theGeocodedResult.

Im not surewe want to introduce this extraGoogleResult interface. But other packages might. (#523)

The purpose of this library is to have a common contract for the geocoded result. If one feel that geocoder X is giving me poor results one should easily be able to switch to geocoder Y. (This is the Liskov substitution principle.)

I think you missed my point, I never suggested to return different objects to break LSP.

My point is if user wants to get more vendor-specific data/methods/abilities, he should use vendor-specific geocoder.

Let's say we want to query city's coordinates by city name within specific country.

Instead of

finalclass UserlandService{publicfunction__construct(Geocoder$geocoder)    {// ...    }publicfunctionfindTheBestResult(string$cityName,string$countryCode):Coordinates    {// Order is not guaranteed.foreach ($this->geocoder->geocode($cityName)as$position) {if ($positioninstanceof GooglePosition                &&$position->getCoordinates()->isDefined()                &&$position->getCountryCode() ===$countryCode            ) {return$position->getCoordinates();            }if ($positioninstanceof OpenStreetMapPosition                &&$position->getCoordinates()->isDefined()                &&$position->getCountryCode() ===$countryCode) {return$position->getCoordinates();            }        }    }}

I prefer

finalclass UserlandService{publicfunction__construct(GoogleGeocoder$google,OpenStreetMapGeocoder$osm)    {// ...    }publicfunctionfindTheBestResult(string$cityName,string$countryCode):Coordinates    {try {return$this->google->findFirstMatchWithCountry($cityName,$countryCode);        }catch (NoResults$e) {foreach ($this->osm->geocode($cityName)as$address) {if ($address->getCountry()->getCode() ===$countryCode) {return$address->getCoordinates();                }            }        }throw NoResults::cityWithinCountry($cityName,$countryCode);    }}

Yes, I have 2 dependencies, but it makes code more explicit and more efficient. The providers have different abilities, different API, different methods. Google can directly execute my query, but OSM does not. Also, I can make very optimized queries for my use cases if specific provider allows it. The generic Geocoder, on other hand, run all possible providers with the same query. It makes him less efficient for this kind of task. Different cases may require different tools.

Do you mind to provide the use case when the generic Geocoder would be better to work with vendor-specific stuff?

I think you missed my point

I think I did miss your point. I very much agree with your last post. That is my intention as well. That is exactly what Im trying to achieve with this PR. I do also want to allow something like this:

finalclass UserlandService {// ...$googleResult =$this->google->geocode('foo');return$googleResult->getFirstResultThatAllowStreetView();}

With the current code in master your preferred example and my example above is not possible. Ibelieve that this PR makes it possible with the best solution.

I like these changes!

Possible improvements:

• interface Position feels a bit specific, I wouldn't necessarily connect an address to it for example. I feel like Location might be a better name.
• getAdminLevels() feels out of place in the Position interface, I'd try to extract that out.

In the radical case you'd end up with a marker interface:

interface Location{}interface Geocoder{publicfunctiongeocode(...):Location;}

It can be literally anything. Very flexible, so awesome.

$googleResult->getFirstResultThatAllowStreetView()

How it is supposed to work? I mean, you can't make custom request here, because result is already here, right? Lazy loading magic?

By introducing interfaces weallow ourselves and third party libraries to extend the functionality whilestill keeping the contract of a provider.

MyAwesomeGoogleProvider may return an class like:

class SuperCollectionimplements \Geocoder\Collection{// ... Implement methods in the Collection interfacepublicfunctiongetFirstResultThatAllowStreetView(){/** @var AwesomeLocation $item */foreach ($this->itemsas$item) {if ($item->hasStreetView()) {return$item;       }     }returnnull;  }}class AwesomeLocationimplements \Geocoder\Location{// ... Implement methods in the Location interfacepublicfunctionhasStreetView() {// ..    }}

Yeah, I understand it.

However, one of my original points with different interfaces of Geocoder was

Also, I can make very optimized queries for my use cases if specific provider allows it

It means that if Google, for example, providesGET /search?street_view=true&limit=1 method, I can utilize it by calling$googleGeocoder->geocodeWithStreeViewOnly(..., 1): GoogleResult, but you suggest to use generic method$geocoder->geocode(...): Result andthen call custom one ($googleResult->getFirstResultThatAllowStreetView()). That makes me wonder: what doesResult contain right after callinggeocode()? Further, doesgetFirstResultThatAllowStreetView make an HTTP-request?

Your value object (GeocodingResult/AddressCollection) becomes a service. It makes him way more complicated. Are you sure this approach is better than just different provider interfaces?

It means that if Google, for example, provide GET /search?street_view=true&limit=1 method, I can utilize it by calling $googleGeocoder->geocodeWithStreeViewOnly(..., 1): GoogleResult

You could add that method, no worries. =)

Your value object (GeocodingResult/AddressCollection) becomes a service.

The interface defines a value object. Third party implementation may do whatever as long as they follow the contract by the interface.

Are you sure this approach is better than just different provider interfaces?

One does not exclude the other.

My point is, and has always been, that if you introduce interfaces you will increase flexibility for the providers (open for extension). At the same time the interfaces specifies a contract which makes providers respect the Liskov substitution principle.

I would be happy to discuss a PR where you introduce aLocationWithStreetview interface or an interface for providers that supports asetBounds function.

But to create one interface for Google, one interface for Bing, one for ... etc is no good solution at all. Those interfaces are too specific and does not help interoperability.

My point is, and has always been, that if you introduce interfaces you will increase flexibility for the providers (open for extension)

The thing is you want to makeinstanceof an extension point in fact. I think it's a hack.

But to create one interface for Google, one interface for Bing, one for ... etc is no good solution at all.

You want to hide same methods behind the generic interface, but when I suggest to make them explicit, you call it "not good solution at all". Do I miss something? :)

Those interfaces are too specific and does not help interoperability.

When one wants to use vendor-specific stuff, he doesn't need interoperability.

You still can make generic and simpleGeocoder::geocode() method, but add also vendor-specific geocoders:

final GoogleGeocoderimplements Geocoder{publicfunctiongeocode():AddressCollection    {// Usual generic contract    }publicfunctiongeocodeSpecific():GoogleAddressCollection    {// Same, but with extended methods    }// More Google-specific methods}// orfinal GoogleGeocoder {publicfunctiongeocode(...): GoogleAddressCollection    {// ...    }// More Google-specific methods}final GoogleGeocoderAdapterimplements Geocoder{publicfunctiongeocode():AddressCollection    {return$this->convertToGeneric($this->googleGeocoder->geocode(...));    }}

When one wants to use generic geocoder, he usesGeocoder contract; when someone wants to acquire vendor-specific abilities, he may want to inject vendor-specific providers.

The thing is you want to make instanceof an extension point in fact. I think it's a hack.

No I do not. You type hint for what you need. Do you need aGeocoder or do you need aGeocoderThatSupportsSetBounds? You would never need both. Application authors knows what provider that is provided and third party libraries should use theGeocoder interface.

But to create one interface for Google, one interface for Bing, one for ... etc is no good solution at all.

You want to hide same methods behind the generic interface, but when I suggest to make them explicit, you call it "not good solution at all". Do I miss something? :)

I put the common things in theGeocoder interface. Nothing more because that would violate the interface segration principle. I agree with you that creating specified interfaces that cover 3-5 provider or a very specific use case are good. But to create 15 different interfaces for 15 different providers serves no purpose.

Ie "vendor specific" are all wrong but "feature specific" are encouraged.

Those interfaces are too specific and does not help interoperability.

When one wants to use vendor-specific stuff, he doesn't need interoperability.

True and I very much agree. But in that case he doesn't need an interface either. =)

But in that case he doesn't need an interface either

Yes, as you can see myGoogleGeocoder is a concrete class, I'd inject it directly and type-hinted against it if I need Google-specific stuff. I said "interface" in a wider sense (class has its own interface).

GeocoderThatSupportsSetBounds

Unfortunately, this class always will return the genericResult from interface point of view, you can't make

finalclass GeocoderThatSupportsSetBoundsimplements Geocoder{publicfunctiongeocode():CustomAddressCollection    {// This implementation is broken    }}

But to create 15 different interfaces for 15 different providers serves no purpose.

Wow, don't create 15 different providers if you don't need them. Create specific providers for most popular vendors.

I said "interface" in a wider sense (class has its own interface).

Sure, my bad. I was thinking about actual interfaces when you said "interface". I did not consider "classes and interfaces".

Unfortunately, this class always will return the generic Result from interface point of view, you can't make

That is not really how you would use it...

interface GeocoderThatSupportsSetBounds {publicfunctionsetBounds(Bounds$bounds);}finalclass MyGeocoderimplements Geocoder, GeocoderThatSupportsSetBounds{publicfunctionsetBounds(Bounds$bounds) {    }publicfunctiongeocode():Collection    {    }}

You must always respect theGeocoder interface. And of course, MyGeocoder::geocode can always return something that extends theCollection.

But to create 15 different interfaces for 15 different providers serves no purpose.

Wow, don't create 15 different providers if you don't need them. Create specific providers for most popular vendors.

We currently have 19 providers in this package and 10 more in the geocoder-extra package..

We currently have 19 providers in this package and 10 more in the geocoder-extra package..

I see. I didn't suggest to make 19 specific providers. As I said, only popular ones (in other words, those providers which you want to extend with specific result objects). The rest would just simply implement the onlygeocode() method as they already do.

That is not really how you would use it...

I wouldn't suggest to make mutable services, honestly, i.e.geocodeWithBounds(...) is usually would be better than->setBounds()->geocode().

And of course, MyGeocoder::geocode can always return something that extends the Collection

And hereinstanceof takes a place? Or IDE hacks like/** @var $result GoogleCollection */? How would you use vendor-specificCollection methods?

Also, by replacing VOs with an interface, you make them harder to understand, to serialize, to replace-on-the-fly ($address = new Address($address->getName(), ..., 'changed value') <- you don't know original type of the VO).

I see. I didn't suggest to make 19 specific providers. As I said, only popular ones (in other words, those providers which you want to extend with specific result objects). The rest would just simply implement the only geocode() method as they already do.

Why? What is the win for that?
I said thatfeature specific interfaces are good. Butwhy vendor specific?

I wouldn't suggest to make mutable services, honestly, i.e. geocodeWithBounds(...) is usually would be better than ->setBounds()->geocode().

This PR does not include such change. It just allows the possibility. Feel free to argue about the implementation details of my example in when that PR comes around.

And here instanceof takes a place?

Nope.

class MyGeocoderimplements Geocoder{publicfunctiongeocode():MyCollection {//.. }}class MyCollectionimplements Collection {// ..publicfunctionextraFeature();}class Consumer {publicfunction__construct(MyGeocoder$gecoder) {//  }}

Also, by replacing VOs with an interface, you make them harder to understand, to serialize, to replace-on-the-fly ....

No no no. Do you mean that Guzzles Request object is harder to understand, serialize and "replace-on-the-fly" (?) because of PSR7? That is just ridiculous and it feels like you are reaching for arguments not to add 2 interfaces...

Why? What is the win for that?

Abstractions limit your models. I mean, it's OK to have more features to your generic Geocoder model, but sometimes it's OK to couple on specific provider. No limitations, precise vendor-specific API.

Some providers can behave a little differently, have different arguments, return different results. For example, you can avoid nullable fields if you clearly know that the Foo provider doesn't supportBounds inAddress, so yourFooAddress won't contain it.

Abstraction has its cost and sometimes you don't want to pay it.

I said that feature specific interfaces are good. But why vendor specific?

Because people want exactly it: they want to stick with specific provider(s) sometimes. They want to know origin of a data (they want explicitly say "first I need to check Google, then OSM"), they want to stick to google_location_id, they want to get station name from Yandex, etc.

Nope.

Youcan't do that:https://3v4l.org/kv9pn

Fatal error: Declaration of MyGeocoder::geocode(): MyCollection must be compatible with Geocoder::geocode(): Collection in /in/kv9pn on line 16

Do you mean that Guzzles Request object is harder to understand, serialize and "replace-on-the-fly" (?) because of PSR7?

Not Guzzle Request by itself, but I think that PSR-7 is not example of great design, you are right. However, I don't think we can discuss it here.

To summarise, I see 3 options:

1. Your proposal.

   Advantanges:

   • no adapters needed;
   • it's possible to query the generic Geocoder (withGeocoderAggregator) andthen get provider-specific stuff withinstanceof from array of results (so-so advantage).

   Disadvantages:

   • additional interfaces for VOs;
   • hackish (in my opinion) way to deal with return type hint ofgeocode().

   Example:

   finalclass GoogleMapsResultimplements Collection{// ...}finalclass GoogleMapsimplements Geocoder{publicfunctiongeocode(...):Collection    {// This method also can return specific collection,// but in order to use it, you need to use instanceof.    }publicfunctiongeocodeWithExtendedGoogleSpecificStuff(...):GoogleMapsResult    {// Google-specific stuff    }}

2. My first proposal.

   Advantages:

   • no interfaces for VO needed (true value objects without possible magic inside of them);
   • providers are not forced to be coupled with the generic Geocoder, you can moveGoogleProvider to external package as a standalone solution for Google Maps API;

   Disadvantages:

   • more code to write (adapters) for providers with additional abilities.

   Example:

   finalclass GoogleMapsResult{// There is no Collection interface to implement.}finalclass GoogleMapsProvider{publicfunctiongeocodeLocation(...):GoogleMapsResult    {// Google-specific stuff    }}finalclass GoogleMapsAdapterimplements Geocoder{publicfunction__construct(GoogleMapsProvider$googleProvider)    {// ...    }publicfunctiongeocode(...):AddressCollection    {return$this->convertToGeneric($this->googleProvider->geocodeLocation(...));    }}

3. My second proposal.

   Advantages:

   • no interfaces for VO needed (true value objects without possible magic inside of them);

   Disadvantages:

   • more code to write (adapters) for providers with additional abilities.
   • providers would have the genericgeocode() method and you have to think how to name your provider-specific method (geocodeWithGoogleStuff?geocodeExtended?).

   Example:

   finalclass GoogleMapsResult{// There is no Collection interface to implement.}finalclass GoogleMapsProviderimplements Geocoder{publicfunctiongeocode(...):AddressCollection    {// ...    }publicfunctiongeocodeWithExtendedGoogleSpecificStuff():GoogleMapsResult    {// ...    }}

As I can see, there is no big difference in our solutions (especially with my second proposal).

Hey! I reviewed both the comments and the changes.

I see interesting things in "both sides", but let me introduce something else: why not adding a "raw" attribute to theGeocoderResult (value) object? That way:

1. no much change in the current code base
2. no interfaces on VOs (which is kinda weird)
3. no interfaces/new methods for each provider
4. people get what they want: access to the underlying data

Sure we don't have a solid contract but,heh, that's not the purpose of the lib anyway. 90% of the issues related to this PR are: "I'd like to get this information from Google Maps".

Thank you.

I can agree with adding araw function. But this does not stop us from introducing these 2 interfaces.

I do aslo think I know ways of addressing the issues of 90% of those PRs. But this PR is a blocker for that.

Some interesting discussion about interfaces for value objects:https://gist.github.com/Ocramius/a9d6713867929fe23f36b35981befb0d

If we go the "raw" way, then do we really need interfaces? Especially on VOs...?

Interfaces are never a bad thing. And Im not sure these are actually value objects. From Wikipedia:

In computer science, a value object is a small object that represents a simple entity whose equality is not based on identity

This is not a "small object" nor a "simple entity". Theneed for araw function is evidence of that.

The only interface would rather beRawAware something then.

I would like to argue for making RawAware a complementary interface or a part of an existing one. In other words: I do not like to see that RawAware is ouronly interface.

Interfaces are never a bad thing

It's a strong statement. Interfaces can be sign of wrong abstraction. We don't need interface forDateTime, for example (note that existingDateTimeInterface is a hack, not a real interface and no one can implement it). We expect GregorianDateTime and we don't expect something else. The behaviour must be the same, there is no room for different implementations, interface is harmful here.

And Im not sure these are actually value objects

What exactly is not a VO?AddressCollection? It is a VO in the same sense asHttpResponse is a VO. It is response of the Geocoder. If you modify it, you'd have different response.

raw, by the way, is similar toheaders inHttpResponse.

In fact, I'm talking about 1-to-1 interfaces. I can agree if you have another methods, but I still don't see how would you use it without hacks likeinstanceof.

@unkind I wrote the definition of a value object. Yes, interfacescould be a sign of wrong abstraction... But Im not inventing new interfaces, Im extracting interfaces from the models we got.

I feel like you are not trying to move this PR to either be accepted or rejected. This discussion has been long as it is, there is no point of having a general (out of topic) discussion about interfaces in general. Im sorry if I have misunderstood or missinterpided your messages.

About how one should use interfaces and different kinds of feature interfaces for the providers:https://3v4l.org/HdCVp

I want to accept or reject this PR. I think all arguments are on the table. After this PR I want to make sure to complete all tasks before we can tag a stable release. I do not want to merge my own PR that's why I ask other maintainers to make the decision.

Interfaces are never a bad thing
Yes, interfaces could be a sign of wrong abstraction... But Im not inventing new interfaces, Im extracting interfaces from the models we got

So, basically, you say "we have modelFoo. ExtractingFooInterface is never a bad idea", right? So we haveDateTime. ExtractingDateTimeInterface is never a bad idea? I don't think so.

I feel like you are not trying to move this PR to either be accepted or rejected

I'm definitely trying to show arguments against this PR. I like the idea about key-valueraw attribute though.

Now let's continue the journey towards Geocoder 4.0 😉

Thank you for merging