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 #538

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@Nyholm
Copy link

Nyholm commented Jul 19, 2016

This is a backwards compatible implementation of HTTPlug. It provides an abstraction over Guzzle and other libraries sending HTTP messages. Using HTTPlug will comply with the dependency inversion principle.

With this PR we can now allow people using Guzzle5 using this library. This will also future proof us whenever Guzzle7 is released.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 19, 2016

Coverage Status

Coverage decreased (-1.09%) to 98.907% when pulling dbff7b9 on Nyholm:httplug into 1711b5e on thephpleague:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 19, 2016

Coverage Status

Coverage decreased (-1.09%) to 98.907% when pulling dbff7b9 on Nyholm:httplug into 1711b5e on thephpleague:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 19, 2016

Coverage Status

Coverage decreased (-4.0%) to 96.049% when pulling 090a977 on Nyholm:httplug into 1711b5e on thephpleague:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 19, 2016

Coverage Status

Coverage decreased (-4.2%) to 95.823% when pulling f9e4017 on Nyholm:httplug into 1711b5e on thephpleague:master.

@Nyholm Nyholm force-pushed the Nyholm:httplug branch from f9e4017 to 32ecb92 Jul 19, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 19, 2016

Coverage Status

Coverage decreased (-4.2%) to 95.823% when pulling 32ecb92 on Nyholm:httplug into 1711b5e on thephpleague:master.

@shadowhand

This comment has been minimized.

Copy link
Member

shadowhand commented Jul 20, 2016

Nobody has ever complained about us using Guzzle directly. What is the impetus for making this PR?

@Nyholm

This comment has been minimized.

Copy link

Nyholm commented Jul 21, 2016

Thank you for the review. The main reason I made this PR is because it is a step towards better software design and better interoperability which means that more users can use this library.

6 month ago we created an issue with some excellent arguments from @sagikazarmark.

Nobody has ever complained about us using Guzzle directly.

That is not quite right... Here are issues and PRs related to you depending on Guzzle. You could expect the same wave of issues when Guzzle 7 is released.

@Nyholm Nyholm referenced this pull request Jul 21, 2016

Closed

Introducing HTTPlug #475

@shadowhand

This comment has been minimized.

Copy link
Member

shadowhand commented Jul 25, 2016

@Nyholm all of those tickets are about upgrading to the latest version of Guzzle, not replacing Guzzle with something else or requesting that we support older versions.

Guzzle 7 will most likely use PSR-7 and so very little would change for us. As far as I know, Guzzle 7 doesn't even exist right now in any form.

@Nyholm

This comment has been minimized.

Copy link

Nyholm commented Jul 25, 2016

Yes @shadowhand, that is exactly the point. Since we are dependent on Guzzle we have to deal with issues related to Guzzle. We have to be aware of changes in that dependency. If we, however, were dependent on an abstraction we would not have to deal with such issues.

Guzzle 7 will most likely use PSR-7 and so very little would change for us.

Yes, they would use PSR-7 and they would probably use PSR-17 which I know you are working hard on. (Thank you for that btw). But Im not concerned with how much of our code we have to change, I'm concerned with us forcing users to a specific version of Guzzle. That is bad software design (Dependency inversion principle) and our users will not be happy about it. (See the issues)

And like I said earlier:

With this PR we can now allow people using Guzzle5 using this library.

Whenever Guzzle7 is released, our users can start using that at day 1 without us doing any change to our library.

@shadowhand

This comment has been minimized.

Copy link
Member

shadowhand commented Jul 26, 2016

We have to be aware of changes in that dependency.

Just as we would have to be aware of changes to HTTPlug and make a version change if when it changes.

Whenever Guzzle7 is released, our users can start using that at day 1 without us doing any change to our library.

That implies that there will be a Guzzle 7 and that it will be so significantly different that it will warrant implementation change. If there is no change in our usage, the upgrade will be as simple as "guzzlehttp/guzzle": "^6.0 || ^7.0".

Further, as your PR shows, depending on HTTPlug actually increases user friction, as the end user not only has to install league/oauth2-client but also a php-http/guzzle6-adapter, which will bring in guzzlehttp/guzzle. If Guzzle 7 came out tomorrow, then the user would have to update their composer configuration to require php-http/guzzle7-adapter and personally validate that it works correctly. Which brings us full circle...

In my opinion, HTTPlug provides no benefits for us at this time.

@Nyholm

This comment has been minimized.

Copy link

Nyholm commented Jul 27, 2016

Just as we would have to be aware of changes to HTTPlug and make a version change if when it changes.

The dependencies I introduce are on 4 interfaces with one method, a virtual package and the discovery lib which is a school book example of SOLID. Guzzle7 is way more complex and more likely to break BC.

depending on HTTPlug actually increases user friction, as the end user not only has to install league/oauth2-client but also a php-http/guzzle6-adapter

Yes, that is the point. It complies with the Single Responsibility Principle. It is not "friction" it is flexibility.

If Guzzle 7 came out tomorrow, then the user would have to update their composer configuration to require php-http/guzzle7-adapter and personally validate that it works correctly.

All adapters follow the Liskov substitution principle. Which means you can easily swap them out and you will have no side effects. Also, you can create your own adapter if you like.

If an implementation misbehaves, it's a bug in the implementation, not in the abstraction.

In my opinion, HTTPlug provides no benefits for us at this time.

Guzzle5 has 8000 daily downloads. None of those people can use version 1 of this library nor any of the great features you added after version 1. Isn't that fact alone reson enough?

It seams like you intentionally ignoring my point that an abstraction is better than an implementation and you are focusing on details. Im trying to introduce good software design patterns and principals. "Guzzle 7 will most likely use PSR-7 and so very little would change for us." is simply not an argument for not using an abstraction.

@ramsey

This comment has been minimized.

Copy link
Contributor

ramsey commented Nov 1, 2016

Thank you for your contributions. We've decided not to move away from the use of Guzzle at this time; closing this PR.

@Aerendir

This comment has been minimized.

Copy link

Aerendir commented Jun 30, 2017

Premise 1: I know the discussion is now closed, but searching for the differences between Guzzle and HTTPlug I read this and came here;

Premise 2: I use Guzzle and I'm good with it and have no intention to switch

but...

This discussion is unfair in my own humble and irrelevant opinion.

Force someone to do or to use something is unfair.

You, The PHP League, are one of the best groups on the web that develops really very good software.

You have a responsibility to deliver good and quality software.
Sometimes this requires to make brave choices.

My arguments for the switching are those:

  1. An abstraction is even better than a concrete implementation: an abstraction doesn't force anyone to use something.
    If I'm already using Guzzle, I'll go with it; if I'm already using Buzz, I'm go with it. I'm free to use what I want (and this is one of the best freedoms in the World);
    This is not a debatable opinion; this is a fact and a de facto standard and a recognized principle: The Dependency Inversion principle is one of the 5 SOLID principles "intended to make software designs more understandable, flexible and maintainable".
    Only for this, the decision of not moving to HTTPlug is wrong, again, only in my own humble opinion.
  2. Force the use of a library over another is an unfair advantage for the chosen one (whichever it is), and this is against the "free market" (of packages in this scenario), is against the progress, is against the improvement of software; Is, in some ways, similar to what is happening in EU with Google and what happened with Microsoft some years ago, too.

Again, I'm an happy Guzzle's user and I have no intention to switch to something else.

But I'm also an happy and passionate developer who reads a lot of code to improve himself and see which solutions other better developers adopted.

If I can learn something I'm happy. Reading your code I learned a lot of things.

And again, you, as one the biggest groups of PHP developers IN THE PHP WORLD - this is important: we speak about the WORLD! -, have a responsibility in developing good software and respect best practices and good and recognized design principles, embrace them and improve them when possible.

Being stick with a concrete implementation is not a good practice and not a fair decision, especially if an alternative exists and even more so if it was already implemented in this package.

Excuse me for my long comment: I'm only trying to make the open source world a better place.

Thank you for your efforts in developing this amazing piece of code: I'm grateful to you.

@ramsey

This comment has been minimized.

Copy link
Contributor

ramsey commented Jun 30, 2017

Any time you use a package that has dependencies on other packages, they are forcing you to use those other packages. Switching from Guzzle to something else (even if it is an abstraction) means we're forcing our users to use that abstraction. The only way we could avoid forcing users to use any other packages is by implementing everything on our own, making this library wholly self-contained.

I do not feel our use of Guzzle in this package restricts anyone's use of this package or freedom to provide an alternative implementation. In fact, Guzzle is provided merely as a convenience. We use only the GuzzleHttp\Psr7\Request object in our RequestFactory class; others may override our RequestFactory to provide their own implementation. Freedom from our defaults is the primary benefit of the factory.

@Soullivaneuh

This comment has been minimized.

Copy link

Soullivaneuh commented Feb 10, 2018

I do not feel our use of Guzzle in this package restricts anyone's use of this package

But you are, because of the composer dependency:

"guzzlehttp/guzzle": "^6.0",

Currently, if I don't want to use guzzle, I'll have it installed anyway. I wan't to upgrade to Guzzle 7 or use Guzzle 5 for some reasons? Well I can't, because of your requirement.

Httplug was created because projects using multiple SDK are regularly whiling onto dependencies hells.

It allows to avoid that and the implementation is quite easy even for maintainer than end users. Currently, a lot of well used library switched to php-http.

Bonus: You don't have to manage HTTP clients anymore, just use the interface and you are good to go! 👍

Could you please re-consider it? In any case, the guzzle requirement should be optional.

@Soullivaneuh

This comment has been minimized.

Copy link

Soullivaneuh commented Feb 10, 2018

Also, using php-http allow us to use bench of plugins, such as the logger which is very very useful for developers.

@kelunik

This comment has been minimized.

Copy link

kelunik commented Feb 10, 2018

@ramsey If that's true, shouldn't it depend on Guzzle's PSR-7 implementation only instead of Guzzle itself? Depending on Httplug is then indeed not helpful and just a BC break.

@Aerendir

This comment has been minimized.

Copy link

Aerendir commented Feb 10, 2018

The important point is that oauth2-client library MUST depend on an abstract implementation.

I don't know HTTPPlug, but I know Guzzle and I prefer its implementation only for this - Guzzle is also used by major libraries like the AWS SDK and this is a point in its favor).

@kelunik

This comment has been minimized.

Copy link

kelunik commented Feb 10, 2018

Indeed, the RequestFactory should declare the PSR-7 RequestInterface as return type, but there's nothing wrong the using a concrete implementation in RequestFactory.

@Soullivaneuh

This comment has been minimized.

Copy link

Soullivaneuh commented Feb 10, 2018

@Aerendir I think you mistaken, HTTPPlug is an abstraction to allow using the HTTP client you want. And Guzzle is part of the current usable client.

@Aerendir

This comment has been minimized.

Copy link

Aerendir commented Feb 10, 2018

@Soullivaneuh , no I don't think... You spoke about Guzzle PSR-7: this is the abstraction, doesn't it? Exactly the same thing as HTTPlug: both are abstractions.

Guzzle Cliente is the concrete implementation that uses the Guzzle PSR-7 Messages (the abstraction)...

If the things are not this way, then I misunderstood and I'm in favor of HTTPlug :P

@ramsey

This comment has been minimized.

Copy link
Contributor

ramsey commented Feb 10, 2018

This conversation comes up every year and sometimes more often than that. We've discussed it at length.

I am not personally against the use of HTTPlug from a technology standpoint. However, I feel that proponents of HTTPlug have been pushy and are browbeating the topic to the point where I do not wish to support the project by including it. The league/oauth2-client project is not the only place where I've observed this behavior.

I typically do not enjoy being so forward with my opinions, but in this case, I feel it is necessary to explain why we have not implemented HTTPlug into this project.

@sagikazarmark

This comment has been minimized.

Copy link
Member

sagikazarmark commented Feb 11, 2018

I feel that proponents of HTTPlug have been pushy and are browbeating the topic

I'm sorry for that 😞

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