Skip to content
This repository was archived by the owner on Jan 13, 2022. It is now read-only.

Use HTTPlug instead of our HTTP clients #605

Closed
yguedidi opened this issue Jun 4, 2016 · 18 comments
Closed

Use HTTPlug instead of our HTTP clients #605

yguedidi opened this issue Jun 4, 2016 · 18 comments
Milestone

Comments

@yguedidi
Copy link
Contributor

yguedidi commented Jun 4, 2016

Point 1 in #587

@yguedidi yguedidi added this to the v6.0 milestone Jun 4, 2016
@Nyholm
Copy link

Nyholm commented Jul 23, 2016

It will also make sure we support projects stuck with Guzzle5.
Im happy to provide with a PR if you considering it.

@SammyK
Copy link
Contributor

SammyK commented Oct 15, 2016

I wanted to raise a point that I've been hearing from a number of people in the community and have seen in other projects. What is the real benefit of using HTTPlug for our specific use case?

Right now we're going to be pulling in 4 new dependencies with the HTTPlug implementation:

        "psr/http-message": "^1.0",
        "php-http/client-implementation": "^1.0",
        "php-http/httplug": "^1.0",
        "php-http/discovery": "^1.0",

Whereas if we just use Guzzle (the de-facto HTTP client for PHP), we could just do "guzzlehttp/guzzle": "^5.0 || ^6.0" and handle the version differences in the SDK itself. We'd get the benefits of ripping out our custom HTTP client and have only one new dependency.

@Nyholm
Copy link

Nyholm commented Oct 15, 2016

The "real" benefit is that we do not depend on an implementation but an abstraction. Which means that we do not have to bother about Guzzle anymore. As you can see I removed almost 2000 lines of code. And you could even remove more.

The 4 dependencies you are talking about are:

"psr/http-message": "^1.0", // A PSR, only interfaces
"php-http/client-implementation": "^1.0", // A virtual package
"php-http/httplug": "^1.0", // Only interfaces (a soon to be PSR)
"php-http/discovery": "^1.0", // A by the book SOLID package that allows zero config.

The only dependency that is an implementation is the discovery package.

Many libraries has taken this step, including PHPGeocoder, FOSHttpCache, KnpGithub client. Im currently working to create a PSR of HTTPlug. But that will of course take some time.

I described other benefits in my PR: #641 (comment)

@SammyK
Copy link
Contributor

SammyK commented Oct 15, 2016

As you can see I removed almost 2000 lines of code. And you could even remove more.

Yes, and we'll have the same benefit switching to Guzzle. :)

I described other benefits in my PR...

However some others librairies can be chosen, like Zend/Http or CakePHP one which are already available in their respective framework.

I'm just concerned with the number of dependencies we're having to add. In order to use zend-http I have to pull in the 4 dependencies + the adapter just to use the native framework HTTP client. Whereas if the SDK assumed Guzzle by default - there's one dependency. The tradeoff is that the developer can't choose their own HTTP client.

The question I'm asking is if it's worth it to include 4 extra dependencies on every single project that ever installs the PHP SDK just so that a small subset of developers can use their favorite HTTP client. :)

Less maintenance on this library: HTTPlug offer a large feature set

I don't see how this would be any different than using Guzzle which has the same features.

Better integration with frameworks

A lot of people are already installing a framework-specific adapter to integrate the Facebook PHP SDK (like in Laravel) or are using a third-party package to integrate like The PHP League's Facebook Client or HWIOAuthBundle.

Shared work: Many libraries already implement HTTPlug or are on the way

And a whole heckofa lot more use Guzzle. Just sayin'. :)

@Nyholm
Copy link

Nyholm commented Oct 15, 2016

The question I'm asking is if it's worth it to include 4 extra dependencies on every single project that ever installs the PHP SDK just so that a small subset of developers can use their favorite HTTP client. :)

I will naturally say yes. =) I've been working over a year with HTTPlug because I strongly believe so. It is better software design, it increases flexibility and encourage the community to make HTTP clients better since we (library authors) shares the work. BUT of course, some (as you say) may have opinions that they do not like this. That is fine every library author decides for him/her self. But these opinions is however, not based on software design or best practices.

Dependencies are bad if they are unstable. Large packages that provide an implementation are naturally a greater subject of change. That's why we had Guzzle 4, 5 and 6 releases within two years.

Better integration with frameworks

A lot of people are already installing a framework-specific adapter to integrate the Facebook PHP SDK (like in Laravel) or are using a third-party package to integrate like The PHP League's Facebook Client or HWIOAuthBundle.

Using HTTPlug, Facebook will get a nice Symfony Web Toolbar integration for free. So there is no need of creating a FacebookGraphBundle to show what requests are made and how they were handled.

Shared work: Many libraries already implement HTTPlug or are on the way

And a whole heckofa lot more use Guzzle. Just sayin'. :)

True. I know. Im a maintainer of Guzzle and our plan is to implement HTTPlug as soon as our PSR gets accepted. That will of course be a non-BC breaking change.

@SammyK
Copy link
Contributor

SammyK commented Oct 15, 2016

It is better software design, it increases flexibility and encourage the community to make HTTP clients better since we (library authors) shares the work.

On an architectural-astronaut level I agree with you. But this isn't very pragmatic in our case. :) Guzzle has inadvertently become "the abstraction" and I'm OK with using it as such.

So there is no need of creating a FacebookGraphBundle to show what requests are made and how they were handled.

This is a moot point. :)

[...] our plan is to implement HTTPlug as soon as our PSR gets accepted.

I have some pretty big concerns with future-coding something that is so far out and so susceptible to change until it is released.

That will of course be a non-BC breaking change.

Assuming it makes its way through PHP FIG without changes to its current state? :)

Ultimately I'd love to get @yguedidi's input on this and we can definitively go one way or another. :)

@Nyholm
Copy link

Nyholm commented Oct 15, 2016

But this isn't very pragmatic in our case. :)

That is a fair point.

Assuming it makes its way through PHP FIG without changes to its current state? :)

That is a hard requirement from my side. HTTPlug and Guzzle are the two biggest players here. PHP-FIG cannot push though a change that forces a major version bumb for any of these libraries.

I have some pretty big concerns with future-coding something that is so far out and so susceptible to change until it is released.

My point is, this is the direction that the HTTPClient libraries are moving.

@yguedidi
Copy link
Contributor Author

Choosing Guzzle as a dependency is a business decision.
But the SDK is a library, and by definition, a library aim to be plugged to a business project, it can't be used by its own.
Developers of business projects make business decision, we must not make the decision for them.
So we must be as abstract as possible.

Yes, it's true that Guzzle is the de-facto HTTP client for PHP, and I think it's still the case, but it become the first choice before the rise of HTTPlug, which I hope will become the de-facto abstraction for libraries that need to make HTTP calls.

Like we did by choosing composer as the only install method, or by bumping the PHP version to a supported one, we try to enforce best practices, and I think using HTTPlug will enforce developers to use PSR-7 to make HTTP calls, which is a best practice IMO.
And developers will have the business decision about the implementation in their hands: a Zend project may use Diactoros, an any framework name here project may use Guzzle, a micro-framework project may use Buzz, and so on.

About having many dependencies, it's a problem only when thoses dependencies may conflict in their versions. But with very stable packages like PSR interfaces or HTTPlug interfaces, there is no problem as they will not have as many major versions as Guzzle have haha.
And now, we have composer to handle dependency resolution for us!

Just my 2 cents :)

@yguedidi
Copy link
Contributor Author

@SammyK @Nyholm any feedback? :)

@Nyholm
Copy link

Nyholm commented Oct 20, 2016

That is a good argument. I agree with you. And I will use your points in the future. =)

@yguedidi
Copy link
Contributor Author

@Nyholm will you bookmark my comment as "pro-HTTPlug argument"? 😄

@Nyholm
Copy link

Nyholm commented Oct 20, 2016

I've never considered the business perspective like you do. That is a different approach to the same issue.
I'll bookmark it as "pro-abstraction".

@SammyK
Copy link
Contributor

SammyK commented Oct 28, 2016

Sorry for the delay. :)

But with very stable packages like PSR interfaces or HTTPlug interfaces, there is no problem as they will not have as many major versions as Guzzle have

That's the point that I disagree with the most. Guzzle never set out to create very different major versions, it just happened organically. And HTTPlug is still quite new and there is no guarantee that it won't change in the future. In fact I'd be really surprised if there wasn't another major release of HTTPlug in the next year or two. But who knows, maybe HTTPlug is a silver bullet, but I'm very skeptical. :)

In the end, I'll go with @yguedidi as long as no one else on the intertubes has any huge objections. :)

@ollietb
Copy link

ollietb commented Aug 6, 2017

We've started using this fork in production rather than the current release for the reasons stated above. I hope it gets merged in soon 👍

@yguedidi
Copy link
Contributor Author

@ollietb thanks for your comment! I'll merge it soon ;)

@Nyholm
Copy link

Nyholm commented Dec 2, 2017

This could now be closed

@kada-zaouak
Copy link

merci

@yguedidi
Copy link
Contributor Author

yguedidi commented Dec 3, 2017

Closed by #641

@yguedidi yguedidi closed this as completed Dec 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants
@ollietb @SammyK @Nyholm @yguedidi @kada-zaouak and others