To me, I would not add all these interfaces, because there are not different implementations for most of them (example the BoundInterface 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 like the 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 the AddressCollection.

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 a GeocoderResult and a Position interface. Those are implemented by the AddressCollection and Address object respectively.

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

I think it is a good idea to remove Position::getLatitude, Position::getLongitude and Position::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 specific Positions ie GooglePosition 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[]
     */
    public function geocode(...);
}

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:

final class GoogleGeocoder
{
    public function __construct(bool $useSsl, ...)
    {

    }

    public function geocode(string $address): GoogleAddress
    {
        // ...
    }

    public function geocodeWithin(string $address, BoundingBox $bbox): GoogleAddress
    {
        // This method is possible, because Google API allows this kind of query
    }
}

final GoogleMaps implements Provider
{
    public function __construct(GoogleGeocoder $googleGeocoder)
    {
        // ...
    }

    public function geocode()
    {
        // Converts GoogleAddress[] to AddressCollection
    }
}

Users will configure the GoogleGeocoder and use it directly if they need some Google-specific methods/data; GoogleMaps will be simple adapter for generic Geocoder, 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 that Geocoder::geocode will give you a GeocoderResult with Positions. That is a contract all geocoders MUST follow. There is nothing that forbids the GoogleGeocoder::geocode to return an object like:

class GoogleGeocoderResult implements GeocoderResult {
  // ... All method in the GeocoderResult interface

  public function getQueryTime() {
    // Return the time it took to query the API
  }
}

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

if ($position instanceof GooglePosition)?

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

interface GoogleGeocoder implements Geocoder
{
    /**
     * @return GoogleGeocoderResult[]
     */
    public function geocode(...);
}

interface GoogleGeocoderResult implements GeocoderResult
{
    // ...

   public function getQueryTime();
}

If an application author decides that he is only interested in using the Google geocoder he/she can type hint directly at the GoogleGeocoder interface. and no if ($position instanceof GooglePosition) is needed. At the same time third party libraries can use the same geocoder because it will still respect the Geocoder 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 the Bounds object.

There is nothing that forbids GoogleGeocoder::geocode to return an object like

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

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 method getCountry: country { code, name }, but what if some provider is allowed to provide more info of a country? In my example, I'd have GoogleMaps/Country value object with all possible country's data. Sure, you can add just getExtraCountry method which returns GoogleMaps/Country or something like this, but it hurts API. Just think about GoogleGeocoder interface like you want to build it from scratch. Do you need to inherit Geocoder 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 by GeocodedResult interface (currently with AddressCollection 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 the AwesomeGeocoderResultLogger::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 the GeocodedResult.

Im not sure we want to introduce this extra GoogleResult 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

final class UserlandService
{
    public function __construct(Geocoder $geocoder)
    {
        // ...
    }

    public function findTheBestResult(string $cityName, string $countryCode): Coordinates
    {
        // Order is not guaranteed.
        foreach ($this->geocoder->geocode($cityName) as $position) {
            if (
                $position instanceof GooglePosition
                && $position->getCoordinates()->isDefined()
                && $position->getCountryCode() === $countryCode
            ) {
                return $position->getCoordinates();
            }

            if ($position instanceof OpenStreetMapPosition
                && $position->getCoordinates()->isDefined()
                && $position->getCountryCode() === $countryCode) {
                return $position->getCoordinates();
            }
        }
    }
}

I prefer

final class UserlandService
{
    public function __construct(GoogleGeocoder $google, OpenStreetMapGeocoder $osm)
    {
        // ...
    }

    public function findTheBestResult(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:

final class 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. I believe 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
{
    public function geocode(...): 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 we allow ourselves and third party libraries to extend the functionality while still keeping the contract of a provider.

MyAwesomeGoogleProvider may return an class like:

class SuperCollection implements \Geocoder\Collection
{
  // ... Implement methods in the Collection interface

  public function getFirstResultThatAllowStreetView(){
     /** @var AwesomeLocation $item */
     foreach ($this->items as $item) {
        if ($item->hasStreetView()) {
          return $item;
       }
     }
     return null;
  }
}

class AwesomeLocation implements \Geocoder\Location
{
    // ... Implement methods in the Location interface

    public function hasStreetView() {
       // ..
    }
}

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, provides GET /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 and then call custom one ($googleResult->getFirstResultThatAllowStreetView()). That makes me wonder: what does Result contain right after calling geocode()? Further, does getFirstResultThatAllowStreetView 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 a LocationWithStreetview interface or an interface for providers that supports a setBounds 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 make instanceof 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 simple Geocoder::geocode() method, but add also vendor-specific geocoders:

final GoogleGeocoder implements Geocoder
{
    public function geocode(): AddressCollection
    {
        // Usual generic contract
    }

    public function geocodeSpecific(): GoogleAddressCollection
    {
        // Same, but with extended methods
    }

    // More Google-specific methods
}

// or

final GoogleGeocoder 
{
    public function geocode(...): GoogleAddressCollection
    {
        // ...
    }

    // More Google-specific methods
} 

final GoogleGeocoderAdapter implements Geocoder
{
    public function geocode(): AddressCollection
    {
        return $this->convertToGeneric($this->googleGeocoder->geocode(...));
    }
}

When one wants to use generic geocoder, he uses Geocoder 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 a Geocoder or do you need a GeocoderThatSupportsSetBounds? You would never need both. Application authors knows what provider that is provided and third party libraries should use the Geocoder 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 the Geocoder 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 my GoogleGeocoder 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 generic Result from interface point of view, you can't make

final class GeocoderThatSupportsSetBounds implements Geocoder
{
    public function geocode(): 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 {
  public function setBounds(Bounds $bounds);
}

final class MyGeocoder implements Geocoder, GeocoderThatSupportsSetBounds
{
    public function setBounds(Bounds $bounds) {

    }

    public function geocode(): Collection
    {

    }
}

You must always respect the Geocoder interface. And of course, MyGeocoder::geocode can always return something that extends the Collection.

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 only geocode() 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 here instanceof takes a place? Or IDE hacks like /** @var $result GoogleCollection */? How would you use vendor-specific Collection 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 that feature specific interfaces are good. But why 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 MyGeocoder implements Geocoder{
  public function geocode(): MyCollection { //.. }
}
class MyCollection implements Collection {
  // ..
  public function extraFeature();
}

class Consumer {
  public function __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 support Bounds in Address, so your FooAddress 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.

You can'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 (with GeocoderAggregator) and then get provider-specific stuff with instanceof from array of results (so-so advantage).

   Disadvantages:

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

   Example:

   final class GoogleMapsResult implements Collection
   {
       // ...
   }
   
   final class GoogleMaps implements Geocoder
   {
       public function geocode(...): Collection
       {
           // This method also can return specific collection,
           // but in order to use it, you need to use instanceof.
       }
   
       public function geocodeWithExtendedGoogleSpecificStuff(...): 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 move GoogleProvider to external package as a standalone solution for Google Maps API;

   Disadvantages:

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

   Example:

   final class GoogleMapsResult
   {
       // There is no Collection interface to implement.
   }
   
   final class GoogleMapsProvider
   {
       public function geocodeLocation(...): GoogleMapsResult
       {
           // Google-specific stuff
       }
   }
   
   final class GoogleMapsAdapter implements Geocoder
   {
       public function __construct(GoogleMapsProvider $googleProvider)
       {
           // ...
       }
   
       public function geocode(...): 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 generic geocode() method and you have to think how to name your provider-specific method (geocodeWithGoogleStuff? geocodeExtended?).

   Example:

   final class GoogleMapsResult
   {
       // There is no Collection interface to implement.
   }
   
   final class GoogleMapsProvider implements Geocoder
   {
       public function geocode(...): AddressCollection
       {
           // ...
       }
   
       public function geocodeWithExtendedGoogleSpecificStuff(): 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 the GeocoderResult (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 a raw 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". The need for a raw function is evidence of that.

The only interface would rather be RawAware 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 our only interface.

Interfaces are never a bad thing

It's a strong statement. Interfaces can be sign of wrong abstraction. We don't need interface for DateTime, for example (note that existing DateTimeInterface is a hack, not a real interface and no one can implement it). We expect Gregorian DateTime 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 as HttpResponse is a VO. It is response of the Geocoder. If you modify it, you'd have different response.

raw, by the way, is similar to headers in HttpResponse.

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 like instanceof.

@unkind I wrote the definition of a value object. Yes, interfaces could 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 model Foo. Extracting FooInterface is never a bad idea", right? So we have DateTime. Extracting DateTimeInterface 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-value raw attribute though.

Now let's continue the journey towards Geocoder 4.0 😉

Thank you for merging