Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
Nyholm merged 16 commits intogeocoder-php:masterfromNyholm:httplug
Aug 16, 2016
Merged

Implement Httplug#512

Nyholm merged 16 commits intogeocoder-php:masterfromNyholm:httplug
Aug 16, 2016

Conversation

@Nyholm
Copy link
Member

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

@Baachi
Copy link
Member

@Nyholm awesome 👍 Thank you very much 🎉

@Nyholm
Copy link
MemberAuthor

Nyholm commentedJun 29, 2016
edited
Loading

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

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

@willdurand
Copy link
Member

👍

@Nyholm
Copy link
MemberAuthor

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
Copy link
Member

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

mablae reacted with thumbs up emojimablae reacted with hooray emoji

@Nyholm
Copy link
MemberAuthor

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
Copy link

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

@Nyholm
Copy link
MemberAuthor

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
Copy link

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

@Baachi
Copy link
Member

Can be merged!

"php-http/discovery":"^1.0"
},
"require-dev": {
"phpunit/phpunit":"^4.8",

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
Copy link

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 reacted with thumbs up emoji

@sagikazarmark
Copy link

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
Copy link

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

@willdurand
Copy link
Member

@Nyholm can you merge this PR?


/**
* @paramHttpAdapterInterface $adapter An HTTP adapter
* @paramHttpClient $client An HTTP adapter

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Parameters not aligned

@Nyholm
Copy link
MemberAuthor

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

sagikazarmark reacted with thumbs up emojisagikazarmark reacted with hooray emoji

@willdurand
Copy link
Member

👍

@NyholmNyholm merged commit9ea2bcb intogeocoder-php:masterAug 16, 2016
@NyholmNyholm deleted the httplug branchAugust 16, 2016 17:13
@ABM-Dan
Copy link

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

@Nyholm
Copy link
MemberAuthor

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
Copy link

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
Copy link

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
Copy link

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

@willdurand
Copy link
Member

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
Copy link

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
Copy link

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

ABM-Dan reacted with thumbs up emoji

@egeloen
Copy link
Contributor

egeloen commentedNov 7, 2016
edited
Loading

@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).
Nyholm and sagikazarmark reacted with thumbs up emoji

@ABM-Dan
Copy link

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

@Nyholm
Copy link
MemberAuthor

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

@geocoder-phpgeocoder-php locked and limited conversation to collaboratorsNov 7, 2016
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@Nyholm@Baachi@willdurand@shadowhand@michaelcullum@sagikazarmark@Flaxis@ABM-Dan@Romain@egeloen

[8]ページ先頭

©2009-2025 Movatter.jp