Skip to content

Added PSR18 adapter as discovery possibility #148

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

Conversation

ricardofiorani
Copy link

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets
License MIT

What's in this PR?

Adds support for this PSR-18 Guzzle Adapter

Why ?

It has more than 10k downloads so why not ?

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix) (what is this ?)

@dbu dbu requested a review from Nyholm December 18, 2019 16:50
@dbu dbu mentioned this pull request Dec 18, 2019
@XWB
Copy link

XWB commented Dec 18, 2019

@ricardofiorani Can you fix the merge conflict?

@ruudk
Copy link

ruudk commented Dec 26, 2019

@dbu @XWB The merge conflict seems to be resolved by @ricardofiorani. Can this be merged so that we can tag a new release for php-http/discovery? On Symfony 4.4 it's impossible to use HttplugBundle because of #150.

@dbu
Copy link
Contributor

dbu commented Dec 27, 2019

@ricardofiorani thanks for this pull request and sorry for the late reaction - i am not too much following the discovery component.

can you please explain the motivation for this client adapter? we already discover adapters for guzzle 5 and 6, and newest guzzle afaik implements psr-18 natively. what scenario does this adapter cover?

@GrahamCampbell
Copy link
Contributor

👎 here. People must only be downloading that package because they are not aware of the originals (and neither must the package author be!).

@GrahamCampbell
Copy link
Contributor

Having had a look at the code for the proposed addition, it is not as complete as the existing Guzzle 6 package that already has millions of downloads. I think this PR can be closed.

@ricardofiorani
Copy link
Author

I'm sorry, I didn't know you guys were this picky about free open source contributions.
I rather close the pull request and contribute to other repositories and initiatives then.

Sorry for any inconvenience.

@ricardofiorani ricardofiorani deleted the add-psr18-adapter-for-guzzle branch June 28, 2020 21:38
@GrahamCampbell
Copy link
Contributor

GrahamCampbell commented Jun 28, 2020

It just doesn't make sense to offer a Guzzle PSR18 bridge with fewer features than the well established, well used and well-tested existing version that is maintained by php-http: https://github.com/php-http/guzzle6-adapter.

NB That package had more than 10k downloads since I first commented on this issue, alone. Daily download stats:

image

@nawarian
Copy link

Really disappointed to see this discussion. I didn't know Open Source was treated like a popularity contest around here.

So a discovery component will only allow popular packages to be... discovered. Amazing!

@GrahamCampbell
Copy link
Contributor

It's not a popularity contest, but it is not practical to develop a 2nd package which has no advantages over the first. Open source is meant to be all about collaboration.

@dbu
Copy link
Contributor

dbu commented Jun 29, 2020

i asked what the use case for the adapter is in december. there was no reply by the author to that question. that question has still not been answered.

when doing a project that aims to solve an already solved problem (adapting guzzle to psr-18) it would be a good idea to explain the motivation. i can't find anything on that topic at https://github.com/ricardofiorani/guzzle-psr18-adapter

the constructive open source collaboration would be to first see if it is possible to adjust the existing psr-18 adapter to solve the issue one has. if that is not possible because it is e.g. a different incompatible approach to build the adapter, a second project makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants