- Notifications
You must be signed in to change notification settings - Fork524
Implement Httplug#512
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Baachi commentedJun 17, 2016
@Nyholm awesome 👍 Thank you very much 🎉 |
Nyholm commentedJun 29, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I've updated the PR. I've also updated the docs. This PR is ready for the final review. Im using |
willdurand commentedJun 29, 2016
👍 |
Nyholm commentedJul 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 commentedJul 18, 2016
I am fine with the PR, thanks@Nyholm! I let @geocoder-php/geocoder review & merge it. |
Nyholm commentedJul 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. |
shadowhand commentedJul 18, 2016
What benefit does this have over using PSR-7 type hints? |
Nyholm commentedJul 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 commentedJul 18, 2016
And would depending directly on Guzzle be a detriment? Has anyone actually requested the ability to use a different HTTP client? |
Baachi commentedJul 24, 2016
Can be merged! |
| "php-http/discovery":"^1.0" | ||
| }, | ||
| "require-dev": { | ||
| "phpunit/phpunit":"^4.8", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not define a bin directory as/bin and avoid executing/vendor/bin/phpunit?
michaelcullum commentedJul 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 commentedJul 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 commentedAug 8, 2016
Would be cool if someone could merge this PR. I really need this in my application. |
willdurand commentedAug 16, 2016
@Nyholm can you merge this PR? |
| /** | ||
| * @paramHttpAdapterInterface $adapter An HTTP adapter | ||
| * @paramHttpClient $client An HTTP adapter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Parameters not aligned
Nyholm commentedAug 16, 2016
Thank you for the feedback. I will merge this when Travis had a change to run again. |
willdurand commentedAug 16, 2016
👍 |
ABM-Dan commentedAug 16, 2016
@willdurand any chance this might be released soon as 3.3.1 or 3.4.0? |
Nyholm commentedAug 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 commentedAug 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 commentedAug 16, 2016
You can use branches as aliases in your project: "willdurand/geocoder":"dev-master as 3.1", This will satisfy |
ABM-Dan commentedAug 16, 2016
Hm, too risky, I guess I will wait for BazingaGeocoderBundle 5. |
willdurand commentedAug 16, 2016
👍 (note that@Nyholm is an official maintainer of Geocoder)
You might want to use a Git commit hash to avoid issues. |
Flaxis commentedAug 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 commentedNov 7, 2016
Hi guys, |
egeloen commentedNov 7, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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:
|
ABM-Dan commentedNov 7, 2016
I guess this simply pushes the issue of "when will we see the next release?" |
Nyholm commentedNov 7, 2016
Have a look here:https://github.com/geocoder-php/Geocoder/milestone/3 It is basicallythis 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. =) |
This PR will replace#487. It continues the work and includes only some minor fixes.