Http client adapters

[jubianchi]

This is a draft implementation of HTTP adapters solving #49.

The library now supports:

• Guzzle 3
• Guzzle 4
• Buzz

The user has to choose an adapter when creating the API client :

<?php

$buzz = new \Gitlab\HttpClient\Adapter\BuzzAdapter();
$guzzle3 = new \Gitlab\HttpClient\Adapter\Guzzle3Adapter();
$guzzle4 = new \Gitlab\HttpClient\Adapter\Guzzle4Adapter();

$bgitlab = new \Gitlab\Client('http://git.foobar.com/api/v3/', $buzz);
$bgitlab->authenticate('foobar', \Gitlab\HttpClient\HttpClientInterface::AUTH_HTTP_TOKEN);

$g3gitlab = new \Gitlab\Client('http://git.foobar.com/api/v3/', $guzzle3);
$g3gitlab->authenticate('foobar', \Gitlab\HttpClient\HttpClientInterface::AUTH_HTTP_TOKEN);

$g4gitlab = new \Gitlab\Client('http://git.foobar.com/api/v3/', $guzzle4);
$g4gitlab->authenticate('foobar', \Gitlab\HttpClient\HttpClientInterface::AUTH_HTTP_TOKEN);

⚠️ This will break user's code when updating the library ⚠️ but I think it's a minor change.

[jubianchi]

/ping @cordoval, @m4tthumphrey

[cordoval]

wow very good work @jubianchi, let's default it to Guzzle4 of course! so that Gush dependencies start not including these others anymore. Still the php-github-api other library depends on guzzle3. But this is a very good step into that direction. 👍

[jubianchi]

@cordoval If possible, I would avoid requiring a default library: all supported library are added as suggests letting the final user choose which one fits best in his project.

If I choose Guzzle4, people using Buzz as their default HTTP client will then have two downloaded libs.

Thoughts ?

[cordoval]

you know that 3.7->3.9.* is not in guzzlehttp/guzzle but guzzle/guzzle so this should be 2 suggests

[cordoval]

@matthiasnoback book on principles of package design has some good info about avoiding these suggests i believe. But i am still working through it.

[cordoval]

cc @sstok

[sstok]

As @matthiasnoback has already explained once There's no such thing as an optional dependency

This library cannot work work without the an actual HTTP adapter, so there are two options here.

1. Pick one and just use it, make it a requirement.
2. Don't ship this core-package with any adapter-implementation at all.
   Create additional packages where each package provides an adapter and composer requirements for actual library.

For example: php-gitlab-api-buzz, provides the buzz http-adapter for the php-gitlab-api core package, and its composer.json requires the php-gitlab-api and buzz-library.

So when someone requires the php-gitlab-api-buzz package, this will install the php-gitlab-api and buzz-library, and no dependencies will be missing.

This also what I have done for RollerworksSearch.
https://github.com/rollerworks/rollerworks-search-doctrine-orm/blob/master/composer.json#L19-L21

Edit. Wrong markup for the article link.

[jubianchi]

@sstok thanks for the explanations :)

[glensc]

@sstok

🚧 Never link to branches 🚧

• see https://news.ycombinator.com/item?id=8046710
  and https://help.github.com/articles/getting-permanent-links-to-files/
• hit y
• and perhaps https://github.com/josephfrazier/octopermalinker

[sstok]

Thanks for the tip, but this issue is really old 🤪 I'm no longer using Gush (or Gitlab for that matter).

[m4tthumphrey]

I do not have the time to go through all this yet, please can you not merge it until I have gone through it.

Many thanks

[cordoval]

I will give it another scan also, I noticed already some cs fixes

[jubianchi]

@m4tthumphrey I won't merge this until it's clean and accepted by everyone ;)

[rossedman]

@jubianchi really like the adapters, has anyone tested this or written tests for it?

[ls12styler]

Any idea of the ETA for this to be merged? It's a really nice idea, and I think the splitting out of adapters is also great!

[jubianchi]

I also like it (not because i made the PR) because it allows the library to be more opene. But before getting this merged we have to review the implementation and also discuss how it should be integrated:

• Adapters are shipped in the library itself and we stick to composer suggests
• Adapters are split in their own libraries

[ls12styler]

I would suggest having the adapters as separate packages.

As I mentioned in #55, I think setting up a GitHub org is probably the way to go. This would simplify where the repos would live.

[jubianchi]

@ls12styler to me, keeping everything in a single repo/library is not a problem. Using suggests is a fake problem.

In fact, the library has optionnal deps. What is mandatory is having at least one of them installed. Splitting everything in several repository will force us to have a strict release process between every repo/library.

So, I'm not really a big fan of splitting everything but I can understand that it could also be cool to do it but before, I think we have to define a good release process and versioning scheme.

[m4tthumphrey]

Whilst I (still) haven't looked too much at this (I'm sorry just really busy), I really don't want to create individual packages just for the HTTP wrapper. All we need is a simple interface which @jubianchi has already defined. Then we just include all of the implementations as single classes inside the namespace and include the suggest block in composer.json as originally mentioned, with the default adaptor explicitly set; ideally Guzzle4 as that seems to be the standard these days.

[ls12styler]

@m4tthumphrey +1 - Sounds sensible.

[rossedman]

@m4tthumphrey +1. I agree with this. I think having Guzzle4 as the default would be best.

[digitalkaoz]

why not use https://github.com/egeloen/ivory-http-adapter here? it already handles everything related and is able to guess the best matching adapter

[jubianchi]

@digitalkaoz seems to be a really good alternative! 👍

[m4tthumphrey]

As posted in another issue:

There will not be a major new tag until the HTTP adaptor (#5, #50, #49), PHP version (#54) and PSR-4 (#55) discussions are concluded. I will also be doing a lot of house keeping over the coming days and weeks, which may include several breaking changes, mainly todo with models (#8, #10). I am not sure on the roadmap of Gitlab but envisage this to coincide with the release of 8.0 depending on @randx and co's plans.

However, I will tag a smaller release to coincide with the release of 7.8 next week.

[m4tthumphrey]

I like the look of Ivory as it handles everything that we don't need to care about (ie Gitlab functionality only). If anyone wants to crack on with an implementation feel free. It looks like it could be done whilst preventing any breaking changes too.

[rossedman]

@digitalkaoz @m4tthumphrey Wow! Didn't even know about Ivory. Really nice.

[Baachi]

@m4tthumphrey Any progress here? If you want i can integrate HTTPlug which is a psr-7 compatible http layer which supports:

• cURL
• Socket
• Guzzle5
• Guzzle6
• React

If you say yes, i can send you a PR tomorrow. (I need that for a project anyways)

[m4tthumphrey]

Crack on @Baachi. Probably best to create a new PR though!

[sagikazarmark]

@Baachi any progress with HTTPlug?

[Baachi]

@sagikazarmark Hey, i started to work on it but have some problems with the listeners.

[m4tthumphrey]

@Baachi I dont mind if some of the API has to change to support this. The client hasn't had a major (breaking) release for a long while.

[fbourigault]

@Baachi do you plan to finish this PR?

[m1guelpf]

@jubianchi I think you should close this in favour of #191

[fbourigault]

Superseeded by #191.