New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Httplug #512

Merged
merged 16 commits into from Aug 16, 2016

Conversation

Projects
None yet
10 participants
@Nyholm
Member

Nyholm commented Jun 12, 2016

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

@Nyholm Nyholm force-pushed the Nyholm:httplug branch from 7a88bc2 to ec9de73 Jun 12, 2016

@willdurand willdurand referenced this pull request Jun 12, 2016

Closed

Implement httplug #487

@Baachi

This comment has been minimized.

Member

Baachi commented Jun 17, 2016

@Nyholm awesome 👍 Thank you very much 🎉

Nyholm added some commits Jun 29, 2016

@Nyholm

This comment has been minimized.

Member

Nyholm commented Jun 29, 2016

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

This comment has been minimized.

Member

willdurand commented Jun 29, 2016

👍

@Nyholm

This comment has been minimized.

Member

Nyholm commented Jul 10, 2016

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

This comment has been minimized.

Member

willdurand commented Jul 18, 2016

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

@Nyholm

This comment has been minimized.

Member

Nyholm commented Jul 18, 2016

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.

Nyholm added some commits Jul 18, 2016

@shadowhand

This comment has been minimized.

shadowhand commented Jul 18, 2016

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

@Nyholm

This comment has been minimized.

Member

Nyholm commented Jul 18, 2016

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

This comment has been minimized.

shadowhand commented Jul 18, 2016

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

},
"require-dev": {
"phpunit/phpunit": "^4.8",

This comment has been minimized.

@michaelcullum

michaelcullum Jul 24, 2016

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

@michaelcullum

This comment has been minimized.

michaelcullum commented Jul 24, 2016

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

This comment has been minimized.

sagikazarmark commented Jul 27, 2016

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

This comment has been minimized.

Flaxis commented Aug 8, 2016

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

@willdurand

This comment has been minimized.

Member

willdurand commented Aug 16, 2016

@Nyholm can you merge this PR?

@@ -40,13 +40,13 @@ class ArcGISOnline extends AbstractHttpProvider implements Provider
private $protocol;
/**
* @param HttpAdapterInterface $adapter An HTTP adapter
* @param HttpClient $client An HTTP adapter

This comment has been minimized.

@sagikazarmark

sagikazarmark Aug 16, 2016

Parameters not aligned

@@ -27,19 +28,16 @@ protected function getMockAdapter($expects = null)
->method('__toString')
->will($this->returnValue(''));
$response = $this->getMock('Psr\Http\Message\MessageInterface');
$response = $this->getMock('Psr\Http\Message\ResponseInterface');

This comment has been minimized.

@sagikazarmark

sagikazarmark Aug 16, 2016

I would recommend using actual implementations for testing instead. This is what we are going to do in HTTPlug libraries as well. The reasoning behind is that HTTP Messages can be interpreted as ValueObjects/DTOs as well and as such, they shouldn't be mocked. The actual behaviour is not important, rather the value they carry.

*/
protected function getAdapter($apiKey = null)
{
return new CachedResponseAdapter(new CurlHttpAdapter(), $this->useCache(), $apiKey);
return new CachedResponseClient(

This comment has been minimized.

@sagikazarmark

sagikazarmark Aug 16, 2016

Is this something that could be replaced by VCR in the future?

This comment has been minimized.

@Nyholm

Nyholm Aug 16, 2016

Member

Maybe, I'll have to look in to that.

Nyholm added some commits Aug 16, 2016

@Nyholm

This comment has been minimized.

Member

Nyholm commented Aug 16, 2016

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

@willdurand

This comment has been minimized.

Member

willdurand commented Aug 16, 2016

👍

@Nyholm Nyholm merged commit 9ea2bcb into geocoder-php:master Aug 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Nyholm Nyholm deleted the Nyholm:httplug branch Aug 16, 2016

@ABM-Dan

This comment has been minimized.

ABM-Dan commented Aug 16, 2016

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

@Nyholm

This comment has been minimized.

Member

Nyholm commented Aug 16, 2016

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

This comment has been minimized.

ABM-Dan commented Aug 16, 2016

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

This comment has been minimized.

sagikazarmark commented Aug 16, 2016

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

This comment has been minimized.

ABM-Dan commented Aug 16, 2016

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

@willdurand

This comment has been minimized.

Member

willdurand commented Aug 16, 2016

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

This comment has been minimized.

Flaxis commented Aug 17, 2016

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

This comment has been minimized.

Romain commented Nov 7, 2016

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

This comment has been minimized.

Contributor

egeloen commented Nov 7, 2016

@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

This comment has been minimized.

ABM-Dan commented Nov 7, 2016

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

@Nyholm

This comment has been minimized.

Member

Nyholm commented Nov 7, 2016

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. =)

@geocoder-php geocoder-php locked and limited conversation to collaborators Nov 7, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.