Implement Httplug

[Nyholm]

This PR will replace #487. It continues the work and includes only some minor fixes.

[Baachi]

@Nyholm awesome 👍 Thank you very much 🎉

[Nyholm]

I've updated the PR. I've also updated the docs. This PR is ready for the final review.

Im using php-http/discovery:0.9 which makes discovery more lightweight and easier to use.

[willdurand]

👍

[Nyholm]

Thank you for the reviews. Is there any questions or suggestions on this PR? Any thing I can do to help getting this merged?

[willdurand]

I am fine with the PR, thanks @Nyholm! I let @geocoder-php/geocoder review & merge it.

[Nyholm]

Thank you.

FYI. Within an hour or two we will tag 1.0 of php-http/discovery. There are no BC breaks. I will update this PR to make sure we have stable dependencies.

[shadowhand]

What benefit does this have over using PSR-7 type hints?

[Nyholm]

HTTPlug is using PSR-7 but PSR-7 does not include a way of creating or sending HTTP Requests. HTTPlug is an abstraction above libraries sending HTTP messages so we do not need a dependency on Guzzle, Buzz or any other library.

This will make sure we follow the dependency inversion principle. ( The D in SOLID )

[shadowhand]

And would depending directly on Guzzle be a detriment? Has anyone actually requested the ability to use a different HTTP client?

[Baachi]

Can be merged!

[michaelcullum]

Why not define a bin directory as /bin and avoid executing /vendor/bin/phpunit?

[michaelcullum]

It's a curious debate as ultimately you are still depending on an implementation, just one of a higher level than the http client itself. But that level of abstraction does remove a dependency on one specific http client which might help improve usage in larger applications using other http clients already. It changes the dependency from 'Guzzle (An http client)' to 'HttpPlug and an http client (but we don't care which one)' and the http client is what is more likely to be swapped in so does help make the lib more interoperable on balance. 👍

[sagikazarmark]

Thanks @michaelcullum, you summarised perfectly what goal we are trying to achieve. Also many thanks to @Nyholm who does the most to actually achieve that goal.

@shadowhand I accept that you don't want to see an HTTP abstraction layer in your projects, but please stop trolling. Do you think your question is valid in a project which provided such layer from the very beginning?

[Flaxis]

Would be cool if someone could merge this PR. I really need this in my application.

[willdurand]

@Nyholm can you merge this PR?

[sagikazarmark]

Parameters not aligned

[Nyholm]

Thank you for the feedback. I will merge this when Travis had a change to run again.

[willdurand]

👍

[ABM-Dan]

@willdurand any chance this might be released soon as 3.3.1 or 3.4.0?

[Nyholm]

This is a major BC break, so we need to release 4.0.

Before we tag that release I suggest we should look over the code if there is any more BC breaking changes we should do before making a release.

@ABM-Dan feel free to test dev-master in your application to see if you can find some bugs that were not discovered by any of us reviewing this.

[ABM-Dan]

Unfortunately I use this as part of BazingaGeocoderBundle, and there's no branches of it that use the dev-master version, even tho they are supposed to be synchronized.

[sagikazarmark]

You can use branches as aliases in your project:

"willdurand/geocoder": "dev-master as 3.1",

This will satisfy ~3.1, although you might need to override some services manually.

[ABM-Dan]

Hm, too risky, I guess I will wait for BazingaGeocoderBundle 5.

[willdurand]

Before we tag that release I suggest we should look over the code if there is any more BC breaking changes we should do before making a release.

👍 (note that @Nyholm is an official maintainer of Geocoder)

Hm, too risky, I guess I will wait for BazingaGeocoderBundle 5.

You might want to use a Git commit hash to avoid issues.

[Flaxis]

Thanks for merging! I was already working in this branch through a forked repository. I haven't found any bugs (yet) using the nominatim endpoint.

[Romain]

Hi guys,
I'm using your package, and I still see the warning about @egeloen package which is abandoned.
Do you think you could merge this fix anytime soon?
Thanks
Romain

[egeloen]

@Romain This is because your composer.json file should still rely on stable version of this library but this feature has only been added to the master branch (not yet released). Two possibilites for you:

• Keep the code as it is and upgrade when the new major version is released (deprecated does not mean buggy, the idea behind this deprecation is only to encourage library like this one to move forward).
• Upgrade to the master branch (as your risk) and upgrade your code accordingly as there are probably BC breaks (it will be a new major release).

[ABM-Dan]

I guess this simply pushes the issue of "when will we see the next release?"

[Nyholm]

Have a look here: https://github.com/geocoder-php/Geocoder/milestone/3

It is basically this PR (input are welcome) and some minors.

Im going to lock this thread so we do not ping all participants now when we go off topic. Feel free to open new issues. =)