Using Httplug instead of Guzzle6 #505

Nyholm · 2016-03-21T15:07:08Z

This is PR will remove our hard coupled dependency on Guzzle6 and instead rely on an interface. This will make the Payum package more SOLID. With this PR we allow users that are stuck with using Guzzle5 to use Payum.

~~This PR introduce some small BC breaks. I've removed the function HttpClientFactory::createGuzzle and also the config parameter named guzzle.client.~~

This will fix #491

Usage

/** @var Payum $payum */
$payum = (new PayumBuilder())
    ->addDefaultStorages()
    ->addGateway('gatewayName', [
        'factory' => 'aGateway'
        'httplug.client' => new \Http\Adapter\Guzzle6\Client(),
    ])
    ->getPayum()
;

TODO

Make code changes
Deprecate existing classes tightly coupled to Guzzle
Add documentation

Implications

When you update to Payum 1.3.0 the installation will fail because you need to install a client implementation. If you choose php-http/guzzle6-adapter everything will work exactly as before. No BC breaks.

makasim · 2016-03-21T17:34:01Z

src/Payum/Core/Bridge/Guzzle/HttpClientFactory.php

     * @return HttpClientInterface
     */
    public static function create()
    {
-        return new HttpClient(static::createGuzzle());
+        return new HttpClient(HttpClientDiscovery::find());


it throws an exception if there is not any implementations (packages), am I right?

Yes. If you try to use discovery without installing puli or if you do not have a psr7 implementation, this will throw an exception.

So we can drop a guzzle as default client, since users are forced to take care of this on composer update. I am positive to it as long as the exception message is clear and a user understands how to proceed

Nyholm · 2016-03-21T20:06:29Z

Thank you for reviewing this so quickly.

makasim · 2016-03-22T14:48:56Z

src/Payum/Core/CoreGatewayFactory.php

@@ -111,7 +116,7 @@ public function createConfig(array $config = array())
    protected function buildClosures(ArrayObject $config)
    {
        // with higher priority
-        foreach (['guzzle.client', 'payum.http_client', 'payum.paths', 'twig.env'] as $name) {
+        foreach (['payum.http_client', 'payum.paths', 'twig.env'] as $name) {


@Nyholm I think you have to add a httplug_client options here too (before the http_client one).

Nyholm · 2016-03-22T15:46:32Z

I've updated the PR according to your suggestions. We are now completely independent from Guzzle. Does it look good? Should I start writing some docs?

makasim · 2016-03-22T17:35:10Z

src/Payum/Be2Bill/Api.php

@@ -138,6 +144,7 @@ public function __construct(array $options, HttpClientInterface $client = null)

        $this->options = $options;
        $this->client = $client ?: HttpClientFactory::create();
+        $this->messageFactory = $options['httplug.message_factory'] ?: MessageFactoryDiscovery::find();;


I'd prefer to have it injected as third argument (optional) and if not passed use a discovery logic. FYI I am going to make this argument not optional in 2.x version as they managed by gateway factory.

Okey, will do.

makasim · 2016-03-22T17:37:58Z

Does it look good? Should I start writing some docs?

Pretty good I must say. There is one comment about the injection of message factory only.

Nyholm · 2016-03-22T18:48:46Z

I made the changes you suggested. Also, I added a HttplugClient and let the old HttpClient be. That means we do not break BC at all.

makasim · 2016-03-22T19:00:52Z

composer.json

@@ -39,7 +39,10 @@
    "require": {
        "php": "^5.5.0|^7.0",
        "payum/iso4217": "~1.0",
-        "guzzlehttp/guzzle": "~6.0",
+        "php-http/httplug": "^1.0",


any reason to require it explicitly. I personally dont think we need it since we have a php-http/client-implementation here. Do I miss something?

You are correct. This package is required indirectly. This row will not have any effect in practice.

The reason I added it is because I believe it is good practice to specify all your dependencie and not rely on other packages to pull in the dependencies. We are using classes/interfaces from this package and that is why I've specified it as a dependency.

I've noticed that this is very opinionated topic and I'm fine with removing it if you like.

let's keep it as is.

Nyholm · 2016-03-23T09:33:59Z

Ive updated the docs, squashed my commits and also updated the PR description.

[httplug] guess client and message factory.

makasim · 2016-03-25T07:30:54Z

src/Payum/Core/Bridge/Guzzle/HttpClientFactory.php

@@ -28,6 +46,6 @@ public static function createGuzzle()
     */
    public static function create()
    {
-        return new HttpClient(static::createGuzzle());
+        return new HttplugClient(HttpClientDiscovery::find());


this change should be reverted. The whole factory is not used anywhere right now.

makasim · 2016-03-25T07:35:20Z

src/Payum/Core/CoreGatewayFactory.php

+
+                // TODO add "if else" for other message factories provided by httplug.
+
+                if (class_exists(MessageFactoryDiscovery::class)) {


It would be good to try discovery first and only after fallback to "my solution"

Nyholm · 2016-03-25T07:59:35Z

There you go, I've added more Httplug clients.

Nyholm · 2016-03-25T08:00:19Z

src/Payum/Be2Bill/Api.php

     *
     * @throws \Payum\Core\Exception\InvalidArgumentException if an option is invalid
     */
-    public function __construct(array $options, HttpClientInterface $client = null)
+    public function __construct(array $options, HttpClientInterface $client, MessageFactory $messageFactory )


Before I rewrite the failing tests. Is this the function signature that yo want?

Nyholm · 2016-03-25T08:34:25Z

There. Everything we discussed is updated

makasim · 2016-03-25T09:02:02Z

UPGRADE.md

+## 1.3.0
+
+* [http-client] guzzlehttp/guzzle was decoupled and replaced by [Httplug](http://docs.php-http.org/en/latest/httplug/introduction.html). This gives you option to use Guzzle6, Guzzle5, Buzz or any other library to send HTTP messages. Make sure to specify `httplug.client` and `httplug.message_factory` or use [Httplug discovery](http://docs.php-http.org/en/latest/discovery.html). 
+


The text is not accurate for current state. I'd like this one:

* [http-client] When you update to Payum 1.3.0 the installation will fail because you need to install a client implementation. If you choose php-http/guzzle6-adapter everything will work exactly as before.

makasim · 2016-03-25T09:08:12Z

@Nyholm Thanks for your work!

makasim · 2016-03-25T09:08:24Z

That's a great move forward.

Nyholm · 2016-03-25T09:10:09Z

Thank you for the feedback and for merging!

Nyholm force-pushed the httplug branch from cbd0f53 to 95aab9d Compare March 21, 2016 16:21

makasim reviewed Mar 21, 2016
View reviewed changes

makasim reviewed Mar 22, 2016
View reviewed changes

Nyholm force-pushed the httplug branch from 06a7513 to e8f008f Compare March 22, 2016 15:36

makasim reviewed Mar 22, 2016
View reviewed changes

Nyholm changed the title ~~[WIP] Using Httplug instead of Guzzle6~~ Using Httplug instead of Guzzle6 Mar 23, 2016

Using Httplug instead of Guzzle6

8f8254f

Nyholm force-pushed the httplug branch from ab7be0e to 8f8254f Compare March 23, 2016 09:31

makasim mentioned this pull request Mar 23, 2016

httplug makasim/PayumBundleSandbox#66

Merged

[httplug] guess client and message factory.

5478b39

makasim mentioned this pull request Mar 24, 2016

remove dependency on guzzle Payum/PayumBundle#353

Merged

Merge pull request #1 from formapro-forks/myhttplug

904fc96

[httplug] guess client and message factory.

makasim reviewed Mar 25, 2016
View reviewed changes

Nyholm added 3 commits March 25, 2016 08:47

Added php-http/guzzle6-adapter to the get-it-started.md

148efa3

Made discovery optional

73624ff

Added more Httplug clients

8497223

Nyholm reviewed Mar 25, 2016
View reviewed changes

Updated the tests to use Httplug

7b32329

makasim reviewed Mar 25, 2016
View reviewed changes

updated upgrade doc

6bba31c

makasim merged commit fa3a9b1 into Payum:master Mar 25, 2016

Nyholm deleted the httplug branch March 25, 2016 09:10

Lctrs mentioned this pull request Apr 18, 2016

[WIP] Replace Guzzle by HTTPlug mike182uk/paypal-ipn-listener#26

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using Httplug instead of Guzzle6 #505

Using Httplug instead of Guzzle6 #505

Nyholm commented Mar 21, 2016

makasim Mar 21, 2016

Nyholm Mar 21, 2016

makasim Mar 22, 2016

Nyholm commented Mar 21, 2016

makasim Mar 22, 2016

Nyholm Mar 22, 2016

Nyholm commented Mar 22, 2016

makasim Mar 22, 2016

Nyholm Mar 22, 2016

makasim commented Mar 22, 2016

Nyholm commented Mar 22, 2016

makasim Mar 22, 2016

Nyholm Mar 22, 2016

makasim Mar 23, 2016

Nyholm commented Mar 23, 2016

makasim Mar 25, 2016

makasim Mar 25, 2016

Nyholm commented Mar 25, 2016

Nyholm Mar 25, 2016

makasim Mar 25, 2016

Nyholm commented Mar 25, 2016

makasim Mar 25, 2016

Nyholm Mar 25, 2016

makasim commented Mar 25, 2016

makasim commented Mar 25, 2016

Nyholm commented Mar 25, 2016


		// TODO add "if else" for other message factories provided by httplug.

		if (class_exists(MessageFactoryDiscovery::class)) {

		## 1.3.0

		* [http-client] guzzlehttp/guzzle was decoupled and replaced by [Httplug](http://docs.php-http.org/en/latest/httplug/introduction.html). This gives you option to use Guzzle6, Guzzle5, Buzz or any other library to send HTTP messages. Make sure to specify `httplug.client` and `httplug.message_factory` or use [Httplug discovery](http://docs.php-http.org/en/latest/discovery.html).

Using Httplug instead of Guzzle6 #505

Using Httplug instead of Guzzle6 #505

Conversation

Nyholm commented Mar 21, 2016

Usage

TODO

Implications

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Mar 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Mar 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makasim commented Mar 22, 2016

Nyholm commented Mar 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Mar 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makasim commented Mar 25, 2016

makasim commented Mar 25, 2016

Nyholm commented Mar 25, 2016