Decouple from Buzz #191

fbourigault · 2017-06-02T15:35:59Z

As discussed in #171 this replace the buzz client by httplug.

I tried to implement this as it was done by @Nyholm for https://github.com/KnpLabs/php-github-api. This remove all the HttpClient stuff which was built on the top of Buzz. Instead it use Httplug standard plugins with a few custom ones for error handling and authentication.

I set PHP required version to 5.6+ as discussed in #171.

This IS a breaking change.

To do

Nyholm

Good work. I really like it. I only had one small comment.

You may also want to consider creating a factory function like: https://github.com/FriendsOfApi/boilerplate/blob/master/src/ApiClient.php#L78-L83

Nyholm · 2017-06-02T17:59:06Z

lib/Gitlab/Api/AbstractApi.php

@@ -1,6 +1,11 @@
 <?php namespace Gitlab\Api;

 use Gitlab\Client;
+use Gitlab\HttpClient\Message\ResponseMediator;
+use function GuzzleHttp\Psr7\stream_for;


You need to remove this line. It will couple us to Guzzle. You should use the StreamFactoryDiscovery instead.

fbourigault · 2017-06-06T12:27:10Z

@Nyholm I fixed the StreamFactory related issue. About the factory function, as there is many authentication methods, I don't know which one I should promote over the others.

I also added tests on some new classes, improved the Gitlab\Client code to limit the amount of breaking changes. I re-introduced the user-agent with a getter and a setter instead of the options array.

ResultPager is now fixed, but still waiting for unit tests.

Nyholm

Good. I like it 👍

Nyholm · 2017-06-06T12:45:02Z

lib/Gitlab/Client.php

-        'user_agent'  => 'php-gitlab-api (http://github.com/m4tthumphrey/php-gitlab-api)',
-        'timeout'     => 60
-    );
+    private $userAgent = 'php-gitlab-api (http://github.com/m4tthumphrey/php-gitlab-api)';


Do we really need this in a property? We never read from it (as I can see)

Users where able to read it through getOption('user_agent'). I added it to limit the breaking changes surface.

fbourigault · 2017-06-06T13:33:06Z

I removed useless methods kept to limit breaking changes.
I documented the breaking changes in UPGRADE.md and also updated README.md.

fbourigault · 2017-06-06T19:50:48Z

ResultPager is currently broken. Waiting for php-http/client-common#74 and a release.

fbourigault · 2017-06-12T16:16:04Z

I no longer require php-http/client-common#74. I ended with a ApiVersion plugin which add the /api/v3/ part when required.

This PR is definitively ready for review.

m1guelpf · 2017-06-12T16:17:10Z

ping @m4tthumphrey

Nyholm

Good. I approve this. I just added some small comments and suggestions.

Nyholm · 2017-06-12T19:16:35Z

README.md

-
-3. Include Composer's autoloader:
+```bash
+composer require m4tthumphrey/php-gitlab-api php-http/guzzle6-adapter


We should add php-http/message here. We could also add a link to http://docs.php-http.org/en/latest/httplug/users.html

When using php-http/guzzle6-adapter, there is no need for php-http/message, right?

EDIT: php-http/message is a php-http/client-common dependency.

Nyholm · 2017-06-12T19:28:01Z

lib/Gitlab/Client.php

+     *
+     * @return Client
+     */
+    public static function createWithHttpClient(HttpClient $httpClient)


Hm, I know I asked before. Maybe create a factory function for URL?

Client::create($url)?

Looks interesting as many companies/users run their own GitLab instance.

Yeah, exactly.

$client = new \Gitlab\Client(); $client->setUrl('http://git.yourdomain.com'); // Could be the same as $client = Gitlab\Client::create('http://git.yourdomain.com');

Nyholm · 2017-06-12T19:32:06Z

lib/Gitlab/Client.php

     */
-    public function getOption($name)
+    public function __get($api)


FYI, mailgun/mailgun-php#192 (comment)

We decided to remove it in Mailgun api

Humm. This should not be in the diff. I will give it a look. Moreover, I also prefer writing methods and remove the magic method and huge comment in the top of the file. It's the same amount of work!

And this will be part of an other PR.

Nyholm · 2017-06-12T19:34:13Z

lib/Gitlab/HttpClient/Plugin/ApiVersion.php

+    {
+        $uri = $request->getUri();
+
+        if (substr($uri->getPath(), 0, 8) !== '/api/v3/') {


Hm, But what if there get new endpoints with /api/v4/?

There are no plans to support multiple versions in a single release. V3 api will be discontinued in august, so the plan is to replace this by v4.

fbourigault · 2017-06-13T07:58:19Z

What about removing Client::setUserAgent and documenting how to customize it like https://github.com/KnpLabs/php-github-api/blob/master/doc/customize.md#configure-the-http-client?

Nyholm · 2017-06-13T12:14:43Z

👍

fbourigault · 2017-06-13T15:32:03Z

As we are breaking, I cleaned a bit more the Gitlab\Client public API. I will update UPGRADE.md and add a doc/customize.md to document how to set the client timeout, how to customize the user-agent and how to add custom headers.

fbourigault · 2017-06-14T11:57:13Z

Documentation is now up-to-date.

m1guelpf · 2017-06-14T12:20:06Z

@fbourigault Is this ready now?

fbourigault · 2017-06-14T12:21:53Z

Yes! I will change things only if a reviewer can convince me to do so!

Nyholm

Good.
I've made some comments. I am really picky now. This PR is good to merge without them.

Nyholm · 2017-06-14T21:00:54Z

UPGRADE.md

+
+## `Gitlab\Client` changes
+
+* The constructor no longer allow to specify base url. Use `setUrl` instead.


Use setUrl or Client::create instead.

Nyholm · 2017-06-14T21:03:52Z

lib/Gitlab/Client.php

+
+        $this->httpClientBuilder->addPlugin(new GitlabExceptionThrower());
+        $this->httpClientBuilder->addPlugin(new HistoryPlugin($this->responseHistory));
+        $this->httpClientBuilder->addPlugin(new RedirectPlugin());


Do we really need this?

Nyholm · 2017-06-14T21:05:40Z

lib/Gitlab/HttpClient/Message/ResponseMediator.php

+namespace Gitlab\HttpClient\Message;
+
+use Psr\Http\Message\ResponseInterface;
+


Maybe a class comment?

fbourigault · 2017-06-19T05:55:02Z

@m4tthumphrey, do you agree with these changes?

fbourigault · 2017-06-28T08:11:03Z

Is it ok if I move forward with next release and merge this PR by the end of this week? ping @m4tthumphrey @jubianchi @radutopala

fbourigault · 2017-07-07T08:52:05Z

https://twitter.com/m4tthumphrey/status/883245976198275072

m1guelpf · 2017-07-07T08:54:18Z

@fbourigault Did you tag a new release?

fbourigault · 2017-07-07T08:56:48Z

Not yet, we have to make the library v4 only.

m1guelpf · 2017-07-07T08:58:04Z

@fbourigault Do you have a task list/milestone?

fbourigault · 2017-07-07T09:01:49Z

I will post a task list today with some "instructions" to be efficient.

m1guelpf · 2017-07-07T09:05:36Z

@fbourigault Please ping me then 😄

m4tthumphrey · 2017-07-10T20:32:00Z

Amazing @fbourigault! Top work!

Nyholm suggested changes Jun 2, 2017

View reviewed changes

m1guelpf mentioned this pull request Jun 5, 2017

Http client adapters #50

Closed

fbourigault mentioned this pull request Jun 6, 2017

Request for collaborators #192

Closed

fbourigault added this to the v4 milestone Jun 6, 2017

fbourigault mentioned this pull request Jun 6, 2017

Gitlab API v4 #171

Closed

Nyholm approved these changes Jun 6, 2017

View reviewed changes

fbourigault added the breaking label Jun 6, 2017

m1guelpf approved these changes Jun 6, 2017

View reviewed changes

fbourigault changed the title ~~[WIP] Decouple for Buzz~~ [WIP] Decouple from Buzz Jun 10, 2017

fbourigault changed the title ~~[WIP] Decouple from Buzz~~ Decouple from Buzz Jun 12, 2017

Nyholm approved these changes Jun 12, 2017

View reviewed changes

fbourigault added work-in-progress and removed work-in-progress labels Jun 13, 2017

m1guelpf approved these changes Jun 14, 2017

View reviewed changes

Nyholm approved these changes Jun 14, 2017

View reviewed changes

replace buzz by httplug

e49bc73

moufmouf mentioned this pull request Jun 19, 2017

Adding the Jobs API #196

Merged

fbourigault merged commit 0a46ae1 into GitLabPHP:master Jul 7, 2017

fbourigault deleted the httplug branch July 7, 2017 08:52

fbourigault mentioned this pull request Jul 17, 2017

The server does not see the parameters passed in the request body #204

Closed


		## `Gitlab\Client` changes

		* The constructor no longer allow to specify base url. Use `setUrl` instead.

		namespace Gitlab\HttpClient\Message;

		use Psr\Http\Message\ResponseInterface;

Decouple from Buzz #191

Decouple from Buzz #191

Conversation

fbourigault commented Jun 2, 2017 • edited Loading

To do

Nyholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbourigault commented Jun 6, 2017

Nyholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbourigault commented Jun 6, 2017

fbourigault commented Jun 6, 2017

fbourigault commented Jun 12, 2017

m1guelpf commented Jun 12, 2017

Nyholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbourigault Jun 13, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbourigault commented Jun 13, 2017

Nyholm commented Jun 13, 2017

fbourigault commented Jun 13, 2017

fbourigault commented Jun 14, 2017

m1guelpf commented Jun 14, 2017

fbourigault commented Jun 14, 2017

Nyholm left a 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

fbourigault commented Jun 19, 2017

fbourigault commented Jun 28, 2017

fbourigault commented Jul 7, 2017

m1guelpf commented Jul 7, 2017

fbourigault commented Jul 7, 2017

m1guelpf commented Jul 7, 2017

fbourigault commented Jul 7, 2017

m1guelpf commented Jul 7, 2017

m4tthumphrey commented Jul 10, 2017

fbourigault commented Jun 2, 2017 •

edited

Loading

fbourigault Jun 13, 2017 •

edited

Loading