Introduce egeloen/http-adapter (road to PSR-7)

[egeloen]

This PR is a proposal for #344.

I have removed the previous adapters but maybe we should keep and deprecate them. Let me know your feeling about it.

[willdurand]

Great! Would you mind rebase your PR? So that we can review it once more before merging it :-)

[egeloen]

When I have implemented this PR, I would like to keep the BC (except for the classes removed but we can still keep them in the current state) but now, I'm not sure it it is the way to go. i'm thinking to rename getContent to just get and directly return the PSR response to the provider then, it can have an access to additional informations like the headers and it will only consume the body when it is really required. What do you think?

Btw, PR rebased.

[willdurand]

I'm fine with your proposal. It is ok to break BC now, but please, do it with parsimony ;-)

[willdurand]

ping @egeloen, I'd like to merge your PR. Let me know if you've time to make your minor tweaks :-)

[egeloen]

@willdurand My tweaks mainly depend on what would you accept as BC break. If you think just returning the response body is enough, I think my PR is ready as it is :)

[willdurand]

Relying on the interface would be nice IMHO.

[egeloen]

Which interfaces are you talking about? The one from the PSR-7? Can you clarify your POV plz? :)

[willdurand]

The one from the PSR-7?

yes

[egeloen]

I will finish it this weekend.

[willdurand]

ping @egeloen

[egeloen]

Sorry, I'm late but there is some BC breaks which have been introduced in the PSR standard. I'm currently fixing them and then, I will come back here. Hope I will be able to do it today.

Btw, do you want to typehint against the RequestInterface and the ResponseInterface or just one of them? For the response, it's pretty easy to see what I need to do but for the request, I'm not sure how you would like to see it because I need to instantiate my own implementation of the request which is available through a factory. Should I introduce something like Geocoder\Adapter\HttpAdapterInterface::createRequest or I let it like it is currently (just the url).

[willdurand]

We could remove this HttpAdapterInterface and directly rely on the Ivory one.

[egeloen]

IMO, removing the interface is not a good idea due to the GeoIP2Adapter which still need to implement it and typehinting the Ivory http adapter once would force me to implement a lot of methods which are not really needed for it. I can obviously typehint just this class and let the GeoIP2Adapter as it is currently or not implement any interface at all for it but it would not be consistent. What's your opinion?

[willdurand]

This GeoIP2Adapter is not a "real" HTTP adapter anyway. Moreover, the lib under the hood has changed a lot, and first stable version is 2.0, which is not what we require right now. I suggest you to not focus on this adapter.

Introduce your lib into Geocoder, and leave the geoip2 adapter alone. I will take care of it.

[egeloen]

I just push my changes.

[willdurand]

Awesome!

[willdurand]

@egeloen one more thing.

By default, the lib does not embed any HTTP adapters and does not install your lib, meaning it is not possible to only install Geocoder and start playing with it. I don't really know what to do, but requiring your lib sounds reasonable to me.

WDYT? ping @toin0u @Baachi et al.

[egeloen]

@willdurand I know, I have not included it by default as you can use the GeoIP2Adapter and then, we don't need my lib but anyway, for easing usage, maybe we should make it mandatory.

[Baachi]

Or we can build an geocoder.phar which include the required parts. I think it would be better for new users.

[willdurand]

Not sure about the PHAR.. It would be a bit tricky for those who only know about composer install.

[Baachi]

Thats true, but for users which doesn't know about composer it would be easier to use this library.

[willdurand]

Could be interesting for sure, but it is not "common". I mean, Silex does that because it is a framework, PHPUnit as well, but it is not the case of libraries. Not saying it is bad, I believe this could be a nice alternative.

[Baachi]

I agree, composer installation should be the prefered installation. But provide an alternative way to install geocoder would be a Nice to Have feature.