Remove dependency on Guzzle #94

Nyholm · 2015-10-03T20:21:34Z

In order to not be dependent on any version of Guzzle I've implemented PHP-HTTP. This PR will replace #88 and #86. Please try and review carefully.

_Don't merge this PR until ´php-http` is stable._ It is stable now (2016-02-28)

I use Guzzle's psr7 package to properly encode the multipart.

Ping @auro1, @RubenKruiswijk and @farmani any

Question: What is the story behind CURLOPT_FORBID_REUSE? I can't find anything about it.
Answer: it was used because we wanted to help the load balancers to distribute the load. The load balancers used today are more clever and this setting is not needed.

PHPUnit OK (49 tests, 75 assertions)

…. Manual sending message to mailgun API works.

auro1 · 2015-10-09T11:32:18Z

composer.json

@@ -2,11 +2,13 @@
    "name": "mailgun/mailgun-php",
    "description": "The Mailgun SDK provides methods for all API functions.",
    "require": {
-        "guzzlehttp/guzzle": "~5.0"
+        "guzzlehttp/psr7": "~1.2",


Why not php-fig/http-message ?

~~You are correct. We should not be dependent on Guzzle's implementation here. We should use a message factory.. I'll update in a minute.~~

Sorry, We need the implementation for because there are too difficult to handle file upload without it. See here: https://github.com/mailgun/mailgun-php/pull/94/files#diff-72d96caf4bb8f89a5d30eafdff75bbe2R63

Nyholm · 2015-10-09T12:22:30Z

First of all. Don't merge this until php-http has reached a stable version. (I'll add that in the PR's description).

@RubenKruiswijk php-http/discovery is used to find a transportation library and to find a factory to create requests.
Our requests we use here are quite complex (with the file upload). It is too difficult to make sure we got all the parameters right etc etc, so We should use a library that do that for us. This is not a part of PSR-7 btw.
Guzzle's implementation helps us with that (there are probably others as well).

@RubenKruiswijk Do you think I should remove happyr/http-auto-discovery? It was a good fit in the beginning but the complexity of file upload removed the beauty and simplicity of using happyr/http-auto-discovery.

Nyholm · 2015-10-12T17:26:37Z

I removed dependency on happyr/http-auto-discovery and using php-http directly.

RubenKruiswijk · 2015-10-12T19:38:37Z

src/Mailgun/Connection/RestClient.php

-            'defaults'=>[
-                'auth' => array(Api::API_USER, $this->apiKey),
-                'exceptions' => false,
-                'config' => ['curl' => [ CURLOPT_FORBID_REUSE => true ]],


We need a way to set these defaults (CURLOPT_FORBID_RESUSE ~~and User-Agent header~~)

I understand that we need to set CURLOPT_FORBID_RESUSE for backwards compatibility, but why is that needed? I cant find any documentation about why we use it. This is from the official documentation

Pass a long. Set close to 1 to make libcurl explicitly close the connection when done with the transfer. Normally, libcurl keeps all connections alive when done with one transfer in case a succeeding one follows that can re-use them. This option should be used with caution and only if you understand what it does as it can seriously impact performance.

Set to 0 to have libcurl keep the connection open for possible later re-use (default behavior).

Here is the commit that added the CURLOPT_FORBID_RESUSE.
b4b9f0e

Ping @travelton, I know it was over 2 years ago, but do you remember why you added that?

@Nyholm If I recall, it was an internal decision to help force connections between API nodes, thus, improving the overall performance from a customer's perspective.

For example, if you are processing 1,000 API calls, reusing connections, and you land on a node that isn't performing very well, you'd issue 1,000 API calls on that non-performant node. With this option enabled, you effectively force a new connection to a different API node every time. So now, maybe one in every x API calls hits a non-performant API node.

I'm betting this "trick" is obsolete, as the load balancing architecture has rapidly improved/changed over the years. Unfortunately, I don't have the insight to confirm, as I'm not working with the Mailgun team these days. As an outsider looking in, I would remove it from default configurations, and allow it as an "Advanced Option" for tinker-ers that love performance tuning. :)

Thank you for your input. Do you know who are on the Mailgun team that can confirm this?

@Nyholm Reached out to the Mailgun team, they confirmed this setting will have no performance benefit due to improvements in load balancing.

Thank you @travelton. You are awesome!

Nyholm · 2015-11-18T16:17:30Z

PHP-HTTP is probably stable now. They will tag a stable version before the end of the year. I've updated this PR to reflect the latest changes in PHP-HTTP.

virgofx · 2015-12-10T15:54:49Z

Looking forward to this +1

bshaffer · 2016-01-21T22:15:35Z

+1 on this! What else needs done to get this merged and tagged?

captn3m0 · 2016-01-21T22:23:21Z

👍 from me as well.

Nyholm · 2016-01-22T04:33:43Z

Thank you @bshaffer
We are waiting on Httplug to get stable. It is currently in beta and discussing release candidates. As soon as it get stable I'll update this PR and let you know.

Nyholm · 2016-02-16T11:25:33Z

Yeah, they did. 😄 The discovery repo is not stable yet though. I am following the discussions in Httplug and in Puli.

I can do some updates here to make sure we are closer to a merge.

tomschlick · 2016-02-25T23:06:58Z

Any news on this?

The fiasco over at Mandrill has caused me to look to Mailgun for our email platform and the dependency on guzzle v5 is conflicting with my existing codebase that has v6.

virgofx · 2016-02-26T00:55:20Z

@tomschlick You can temporarily use the following fork in your composer until everything is merged.

    "repositories": [{
        "type": "vcs",
        "url": "https://github.com/MunkSoftware/mailgun-php"
    }],
    "require": {
        "php": ">=5.6.0",
        "guzzlehttp/guzzle": "~6.0",
        "mailgun/mailgun-php": "dev-master#04f1baa2fdbb4d3fba035e28ce66ec676e382523",
        ...
    }

tomschlick · 2016-02-26T01:40:31Z

Don't know how I didn't think of that. I've done it many times before...
Thanks!

Tom Schlick

On Thu, Feb 25, 2016 at 4:55 PM -0800, "Mark Johnson" notifications@github.com wrote:

@tomschlick You can temporarily use the following fork until everything is merged.

"repositories": [{
"type": "vcs",
"url": "https://github.com/MunkSoftware/mailgun-php"
}],
"require": {
"php": ">=5.6.0",
"guzzlehttp/guzzle": "~6.0",
"mailgun/mailgun-php": "dev-master#04f1baa2fdbb4d3fba035e28ce66ec676e382523",
...
"
}]

—
Reply to this email directly or view it on GitHub.

Nyholm · 2016-02-28T11:12:16Z

I has been almost 5 months since I've opened this PR and it is now ready for a final review. Please be extra observant to the readme and the new installation instructions. Do they make sense? Do you understand that you need to do one of the following:

create a instance of a HttpClient and inject to Mailgun
install Puli.

After reviewing and fixed the issues you have found I will squash my commits and make sure it is ready for merge.

Nyholm · 2016-03-04T15:17:15Z

Ping @virgofx @captn3m0 @tomschlick @bshaffer @jessespears Do you have any input?

tomschlick · 2016-03-04T17:50:27Z

$mailgun = new \Mailgun\Mailgun('api_key', null, null, true, $client);

This is a little crazy with 5 params, 3 of which most people wouldn't touch.

Since this is going to be a breaking change, can we get rid of some of the options in construct and throw some of them to their own methods?

For instance if we got rid of the SSL = true and the api version it would look much cleaner as

$mailgun = new \Mailgun\Mailgun('api_key', 'mydomain.com', $client);

// If you needed those options you could do...

$mailgun->setApiVersion('v2'); // default current version
$mailgun->setSSLEnabled(false); // default true

virgofx · 2016-03-05T01:20:34Z

Agree with @tomschlick 's comment.

@Nyholm I'll review the changes this weekend and provide some feedback.

Nyholm · 2016-03-05T10:16:10Z

Thank you for the review. I agree, the constructor was very bloated with parameters. I removed API version and SSL as you suggested.

tomschlick · 2016-03-06T13:20:29Z

👍 looks good!

virgofx · 2016-03-06T18:40:48Z

src/Mailgun/Mailgun.php

        $this->apiKey = $apiKey;
-        $this->restClient = new RestClient($apiKey, $apiEndpoint, $apiVersion, $ssl);
+        $this->restClient = new RestClient($apiKey, $apiEndpoint, $httpClient);
    }


Can we make $apiKey required? I don't think you can use Mailgun without it?

I agree, API key should be required. I did not want to make too many BC breaking changes. That's why I left is as it was.

Thank you for your review.

virgofx · 2016-03-06T18:47:11Z

Looks good to me. Only comment was updating the Readme to not have the null options in construct as the first example ... if you wanted to do that .. perhaps make two examples. The first one should always be the simple default for how most people can use the API. The second one could be with a different rest client and different endpoint. And ... wasn't sure was it possible to have a null API key? If it's not, make this parameter not optional.

mollierobbert · 2016-03-21T15:11:14Z

When will this be merged?

tristanbes · 2016-03-30T10:31:18Z

Interested in this getting merged ASAP too ;-)

Nyholm · 2016-03-30T10:34:50Z

I have contacted @jessespears and he said he will do his best to have a look at it before the weekend.

jessespears · 2016-04-01T23:59:57Z

Thanks for this contribution, @Nyholm! This is great!

sagikazarmark · 2016-04-02T06:11:32Z

Nice Job @Nyholm

Anders Munk and others added 9 commits July 23, 2015 13:27

Bumped to guzzlehttp/guzzle:~6.0

ffab6c9

PHPUnit OK (49 tests, 75 assertions)

Updated rest client to use PSR7. No phpunit tests yet; will come soon…

04f1baa

…. Manual sending message to mailgun API works.

working on http adapter

b2dd642

make sure we can attach files

5ec7aab

Make sure we do not depend on any http transport library

b080b18

Added information about PHP-adapter

e0c3558

Use guzzle5 in development

5fcbb3d

Merge branch 'http' of https://github.com/Nyholm/mailgun-php into http

108f619

code style

ad015ce

auro1 reviewed Oct 9, 2015
View reviewed changes

Removed dependency on Happyr/AutoDiscovery

61d0326

RubenKruiswijk reviewed Oct 12, 2015
View reviewed changes

Nyholm added 5 commits October 12, 2015 22:46

Changed according to Rubens suggestions

4131802

Added docs

b842045

Updated the php-http lib

78f2527

Updates to the php-http library

a0edd0e

Updated to reflect changes in PHP-HTTP

7f3d637

Nyholm added 2 commits December 21, 2015 00:24

Updated to httplug beta

ac0b85b

updated link

6ea7e93

bshaffer mentioned this pull request Jan 21, 2016

Bumped to guzzlehttp/guzzle:~6.0 #86

Merged

Updated to latest version of Httplug and made auto discovery optional

b961cfb

Nyholm added 2 commits March 5, 2016 11:13

Removed unused tests and some parameters from the constructor

b85f146

removed unsupported PHP versions

8986940

virgofx reviewed Mar 6, 2016
View reviewed changes

bshaffer mentioned this pull request Mar 14, 2016

add mailgun docs GoogleCloudPlatform/php-docs-samples#22

Closed

jessespears merged commit 9ac73e0 into mailgun:master Apr 1, 2016

Nyholm mentioned this pull request Apr 5, 2016

Updated PHP docs for version 2.0 of the mailgun client mailgun/documentation#187

Merged

Nyholm mentioned this pull request Jul 20, 2016

upgrade guzzle version #88

Closed

Remove dependency on Guzzle #94

Remove dependency on Guzzle #94

Conversation

Nyholm commented Oct 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Oct 9, 2015

Nyholm commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Nov 18, 2015

virgofx commented Dec 10, 2015

bshaffer commented Jan 21, 2016

captn3m0 commented Jan 21, 2016

Nyholm commented Jan 22, 2016

Nyholm commented Feb 16, 2016

tomschlick commented Feb 25, 2016

virgofx commented Feb 26, 2016

tomschlick commented Feb 26, 2016

Nyholm commented Feb 28, 2016

Nyholm commented Mar 4, 2016

tomschlick commented Mar 4, 2016

virgofx commented Mar 5, 2016

Nyholm commented Mar 5, 2016

tomschlick commented Mar 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virgofx commented Mar 6, 2016

mollierobbert commented Mar 21, 2016

tristanbes commented Mar 30, 2016

Nyholm commented Mar 30, 2016

jessespears commented Apr 1, 2016

sagikazarmark commented Apr 2, 2016