Decouple from Guzzle #389

Nyholm · 2016-06-12T14:02:31Z

This PR replaces #352. We do not want to be coupled to Guzzle, instead we want to use an abstraction layer so the end user can choose what HTTP message implementation to use.

This PR removes all our HTTP layer and replaces it with Httplug and custom plugins. I struggled for a while to make this PR backwards compatible but the code got too complicated and would have left us with an abstraction for an abstraction.

Question to the maintainers:
Do you like this change? Should I continue with this PR and complete the TODOs?

TODO

Add some more docs about HTTP plug in the readme.
Make sure existing docs are up to date
Make sure the tests passes

stof · 2016-06-15T16:18:30Z

lib/Github/Client.php

     */
    public function getHttpClient()
    {
-        if (null === $this->httpClient) {
-            $this->httpClient = new HttpClient($this->options);
+        if ($this->pluginsModified) {


you need to reset this property here, otherwise a new client will be created for each call to the getter

cursedcoder · 2016-06-24T21:50:00Z

@Nyholm looks good!

What do you guys say @docteurklein @gquemener ?

P.S. that's a good candidate for 2.0

Nyholm · 2016-06-25T08:20:35Z

Thank you. I'll make the changes you suggested. Give me a couple of days.

gquemener · 2016-06-25T08:26:35Z

I totally agree with the idea of using an http client abstraction!

Nyholm · 2016-06-26T20:40:25Z

The tests are passing. I might need help making sure the docs are up to date. I will try to proof read the existing docs tomorrow.

More reviews are much welcome. Also, please try it out in your project to make sure I haven't missed anything.

cursedcoder · 2016-06-27T13:02:45Z

@Nyholm will be able to check it tomorrow!

cursedcoder · 2016-06-29T07:47:26Z

lib/Github/Api/AbstractApi.php

@@ -73,7 +73,12 @@ protected function get($path, array $parameters = array(), $requestHeaders = arr
        if (array_key_exists('ref', $parameters) && is_null($parameters['ref'])) {
            unset($parameters['ref']);
        }
-        $response = $this->client->getHttpClient()->get($path, $parameters, $requestHeaders);
+
+        if (count($parameters)>0) {


count($parameters) > 0 or 0 !== count($parameters)

cursedcoder · 2016-06-29T08:06:41Z

fantastic work you have done here @Nyholm
I will try it on our project in some days and also check the docs

Nyholm · 2016-06-29T08:25:11Z

Thank you @cursedcoder. I've updated the code according to your comments. They all makes perfect sense and some were just typos of mine.

Please do.

cursedcoder · 2016-07-07T10:17:15Z

README.md

-> `php-github-api` follows the PSR-0 convention names for its classes, which means you can easily integrate `php-github-api` classes loading in your own autoloader.
+> `php-github-api` follows the PSR-4 convention names for its classes, which means you can easily integrate `php-github-api` classes loading in your own autoloader.
+
+Why `php-http/guzzle6-adapter`? We are decoupled form any HTTP messaging client with help by [HTTPlug](http://httplug.io/). Read about clients in our [docs](doc/cusomize.md).


typo in cusomize

Nyholm · 2016-07-10T09:43:42Z

I've fixed the typo and rebased the PR.

cursedcoder · 2016-07-12T13:09:34Z

@Nyholm it seems Client::authenticate() doesn't work with arguments:

<?php
            $parameters['github_api_client_id'],
            $parameters['github_api_client_secret'],
            Github\Client::AUTH_URL_CLIENT_ID

Nyholm · 2016-07-14T07:56:20Z

Thank you @cursedcoder.

There was an issue where we appended ?foo=bar on the existing query instead of &foo=bar. It is now fixed

GrahamCampbell · 2016-07-15T04:23:32Z

.styleci.yml

@@ -1,5 +1,4 @@
 preset: psr2

 enabled:
-  - long_array_syntax


That will let us use the short array suntax. See this comment: #389 (comment)

Nyholm · 2016-07-18T20:26:11Z

I've rebase the PR and squashed my commits.

Nyholm · 2016-07-25T23:40:47Z

Is there anything else I can do to help getting this PR merged?

cursedcoder · 2016-07-27T08:13:27Z

@Nyholm I'm working on releasing it as RC or @dev version.
Stay tuned next few days.

Nyholm · 2016-07-27T08:25:09Z

Awesome. Thank you for merging!

sagikazarmark · 2016-07-27T08:41:11Z

composer.json

    },
    "require-dev": {
        "phpunit/phpunit": "~4.0",
+        "php-http/guzzle6-adapter": "~1.0",
+        "guzzlehttp/psr7": "^1.2",


This comes bundled with Guzzle 6, no need to require it separately IMO

Nyholm mentioned this pull request Jun 12, 2016

Use PSR-6 cache #390

Closed

stof reviewed Jun 15, 2016
View reviewed changes

cursedcoder reviewed Jun 29, 2016
View reviewed changes

cursedcoder reviewed Jul 7, 2016
View reviewed changes

Nyholm force-pushed the httplug branch from 83eb4e7 to 08aea7c Compare July 10, 2016 09:43

Nyholm mentioned this pull request Jul 14, 2016

Add method to get last request sent php-http/client-common#23

Closed

2 tasks

GrahamCampbell reviewed Jul 15, 2016
View reviewed changes

Nyholm force-pushed the httplug branch from d09ebbf to 726163a Compare July 18, 2016 20:23

Decouple from Guzzle by using HTTPlug

fcfc29c

Nyholm force-pushed the httplug branch from 726163a to fcfc29c Compare July 18, 2016 20:25

Nyholm mentioned this pull request Jul 23, 2016

[POC] [WIP] Abstract the HTTP Client by using HTTPlug #352

Closed

cursedcoder merged commit 729bb03 into KnpLabs:master Jul 27, 2016

Nyholm deleted the httplug branch July 27, 2016 08:25

sagikazarmark reviewed Jul 27, 2016
View reviewed changes

Uh oh!

Decouple from Guzzle #389

Decouple from Guzzle #389

Uh oh!

Conversation

Nyholm commented Jun 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

stof Jun 15, 2016

Choose a reason for hiding this comment

Uh oh!

cursedcoder commented Jun 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nyholm commented Jun 25, 2016

Uh oh!

gquemener commented Jun 25, 2016

Uh oh!

Nyholm commented Jun 26, 2016

Uh oh!

cursedcoder commented Jun 27, 2016

Uh oh!

cursedcoder Jun 29, 2016

Choose a reason for hiding this comment

Uh oh!

cursedcoder commented Jun 29, 2016

Uh oh!

Nyholm commented Jun 29, 2016

Uh oh!

cursedcoder Jul 7, 2016

Choose a reason for hiding this comment

Uh oh!

Nyholm Jul 10, 2016

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Jul 10, 2016

Uh oh!

cursedcoder commented Jul 12, 2016

Uh oh!

Nyholm commented Jul 14, 2016

Uh oh!

GrahamCampbell Jul 15, 2016

Choose a reason for hiding this comment

Uh oh!

Nyholm Jul 15, 2016

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Jul 18, 2016

Uh oh!

Nyholm commented Jul 25, 2016

Uh oh!

cursedcoder commented Jul 27, 2016

Uh oh!

Nyholm commented Jul 27, 2016

Uh oh!

sagikazarmark Jul 27, 2016

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Nyholm commented Jun 12, 2016 •

edited

Loading

cursedcoder commented Jun 24, 2016 •

edited

Loading