Please Add place_id (google) in result. #421

Activity

1. willdurand

   added

   feature request

   on Apr 20, 2015
2. vandalouze commented on Oct 7, 2016

   

   vandalouze

   on Oct 7, 2016

   More actions

   Any plans for this?
3. unkind commented on Oct 7, 2016

   

   unkind

   on Oct 7, 2016

   Last edited by unkind

   Contributor

   More actions

   I wonder why do you need a generic Geocoder if you need Google-specific thing. These requirements are mutually exclusive.

   Just don't have direct dependency on the library in your codebase (see https://speakerdeck.com/jakzal/decoupling-from-the-framework for instance). Sometimes the library fits your requirements and you can use it as an implementation detail; sometimes it does not and it means you should use something else.

   You can have your own set of contracts for that would fit your needs perfectly, e.g.

   namespace Acme\Geocoder;
   
   interface Geocoder
   {
       /**
        * @return Address
        * @throws AddressWasNotFound
        */
       public function geocode(...);
   }
   
   
   final class Address
   {
       public function __construct(int $placeId, ...)
       {
           // ...
       }
   }

   Then you can simply copy-paste code from GoogleMaps provider as a start point, tweak it and get place id from the Google Maps API.
4. vandalouze commented on Oct 7, 2016

   

   vandalouze

   on Oct 7, 2016

   Last edited by vandalouze

   More actions

   I wonder why do you need a generic Geocoder if you need Google-specific thing. These requirements are mutually

   Lot time you don't want to create EVERY time a new place entity when you run the geocoder, just reuse the existing place from the corresponding provider. I think this is a very legitime reason to provide the vendor ID.

   Just imagine when you have a more generic place like just a City. When now 1'000 entities creates an own Place it's just total bad practice and have nothing to do with a RDBMS.

   This makes the whole library just not so good as it could be.
5. unkind commented on Oct 7, 2016

   

   unkind

   on Oct 7, 2016

   Last edited by unkind

   Contributor

   More actions

   So you want to load same address for updates or something like this, if I understood you correctly?

   Well, my point is that every time you add provider-specific thing into the Geocoder, you make it more and more vague thing. Mixed types (int, string, array), bunch of nullables fields which you can't rely on, and so on. Just for example, OpenStreetMap doesn't have place_id AFAIK, but it has composite key like type (node, relation, way) + id. Some providers doesn't have it at all.

   All these provider-specific make an abstraction more leaky.

   I mean, if you want to stick to specific provider like Google Maps, use its place_id and other stuff, why you don't want to use something like GoogleGeocoder with very specific and straightforward interface? No annoying nullable, less if's, more details.
6. vandalouze commented on Oct 7, 2016

   

   vandalouze

   on Oct 7, 2016

   Last edited by vandalouze

   More actions

   Because I like abstraction but there is a justification for access specific information. In most other libraries you can simply access the underlaying driver, adapter OR response to get more specific information when you want. And there are lot of issues here which are related to this underlaying design problem of this library.

   When I want to access some provider specific response information in this library I have to rewrite many classes (Address, AddressFactory, Provider) so it ends in a totally new fork instead just overriding classes.

   Conclusion: The author of this library want to provide only the abstraction interfaces and don't want to give access to the underlaying informations. Makes no sense because how I said, it is not a part of the abstraction interface itself.
   The users of this library should not justify why they want to access the underlaying information, the library should just provide it somehow because the data is just there and important for many people.
7. unkind commented on Oct 7, 2016

   

   unkind

   on Oct 7, 2016

   Contributor

   More actions

   In most other libraries you can simply access the underlaying driver, adapter OR response to get more specific information when you want

   Majority can be wrong.

   When I want to access some provider specific response information in this library I have to rewrite many classes (Address, AddressFactory, Provider) so it ends in a totally new fork instead just overriding classes.

   From my experience, this is not that hard. Your contracts is a clarifying of your own needs. It should be simple to say what do I want from a library, so it's easy to make bunch of simple value objects and interfaces for my needs. And implementation of providers is not a rocket science, it takes literally few minutes to understand it.

   Yes, after all, I have a "totally new" library, but I don't see a problem at all, because for some reason original library just doesn't fit my needs, as simple as that.

   Conclusion: The author of this library want to provide only the abstraction interfaces and don't want to give access to the underlaying informations

   This is called "encapsulation".

   The users of this library should not justify why they want to access the underlaying information, the library should just provide it somehow because the data is just there and important for many people.

   There is a public contract between users of the library and its developers: the interface and value objects. They take the responsibility to maintain this contract, they keep BC, they add new providers, etc. When you ask them to add new methods, you ask them to take more responsibilities. They can do it, but they don't have to.
8. Nyholm commented on May 22, 2017

   

   Nyholm

   on May 22, 2017

   Member

   More actions

   Sorry for the very late response. This is now very easy to achieve. See #624

   I will close this issue, do not think that I do not want to see this feature. I will be happy to review a PR.
9. Nyholm

   closed this as completedon May 22, 2017