[Http Client] Deal With Scopped client to separate repository and Http Client

[ismail1432]

This PR should fix #14

Description

• Separate Http Client and Repository

• Configure Http Client with Symfony ScopingHttp Client to not pass base uri, ID, token and accept header

• Test bundle configuration

• Test HttpClient

• Refacto tests

• [x] Add scopped client

• [x] Move methods to specific repository

• [x] Fixe tests

• [x] Add tests of new configuration

[yoanbernabeu]

i am so excited to read your code! I am on a "disconnected" weekend in the mountains, I watch all this Monday! Thanks bro!

[ismail1432]

Disconnected weekend is a good idea 🔥🔥 No problem I'll highlight good or interesting part of the PR 👀

[ismail1432]

It's OK on my side, I added some comments to highlight interesting thing, tests are green BUT you should try locally before merging.

If you have any question don't hesitate :wave:

[ismail1432]

:warning: I noticed that the path V0 and the id is removed when we call the API, I will investigate when I have the time to do :detective:

[ismail1432]

Well done here, the only thing I "do not approve" is the renaming to AirtableRepositoryInterface, even tho we see that findAll, findBy etc refers to the Repository pattern, it does not change the face that this is an AirtableClient for Symfony.

Could'nt we create a AirtableRepositoryInterface that inherits from AirtableClientInterface, just to please everyone ?

interface AirtableRepositoryInterface extends AirtableClientInterface {}

Thanks @liorchamla for the feedback and nice catch, I would prefer to not make AirtableRepositoryInterface extends AirtableClientInterface to respect the Interface segregation principle, from my POV we should not force the AirtableHttpRepository implements some methods of the AirtableClientInterface My 2 cents

[liorchamla]

Thanks @liorchamla for the feedback and nice catch, I would prefer to not make AirtableRepositoryInterface extends AirtableClientInterface to respect the Interface segregation principle, from my POV we should not force the AirtableHttpRepository implements some methods of the AirtableClientInterface My 2 cents

Wait, maybe I did not understand : did you replace the AirtableClientInterface by the AirtableRepositoryInterface ?

If yes, then I disagree with that :p

If not, then I just did not understand (because the bundle evolves so much in so little time) what is the difference between the two interfaces and we can forget my first comment :)

[liorchamla]

Ok @ismail1432 we did not understand each other :

We had once a AirtableClientInterface, which is now named AirtableRepositoryInterface. This is what I disagree with. The thing that brings this bundle is indeed a Symfony Client for Airtable. Even if the methods names let us see a repository pattern, it is still a client for a third party app in the cloud.

BUT ! I can understand why some people want to call it a repository. So my proposition still ok : ▶ We create two interfaces that are in fact the same : AirtableClientInterface AND AirtableRepositoryInterface ▶ We configure the container to give us the SAME service with the two aliases (the two interfaces)

So people who wants to see in it a repository ask the container for AirtableRepositoryInterface, and people like me, who still want to see in it a client ask for an AirtableClientInterface :D

In the end : it does not really matters but I find it weird to call this thing a Repository but we will do whatever suits you all :)

[ismail1432]

⚠️ I noticed that the path V0 and the id is removed when we call the API, I will investigate when I have the time to do

I push the correction => https://github.com/yoanbernabeu/Airtable-Client-Bundle/commit/f44886d85bf5b1f189dbf48a90bdd082fb1cf6da but it's strange that HttpClient remove these info from base_uri. Maybe I misunderstand something and I will open an issue for asking

[ismail1432]

My bad maybe the Naming leads to the confusion. The main goal of the PR is to separate responsibility from the current AirtableClient in 2 classes

A HTTP Client and a service/repository (naming is difficult) with basic method (findX) at the end there is just 2 big changes :

• remove request method from AirtableClient
• create a configured client that is injected in AirtableClient

:bulb: To avoid confusion I will Update the PR to keep the name AirtableClient with all "finders" and call the configured HttpClient Transport so we would have

final class AirtableTransport extends ScopingHttpClient
{
    public const BASE_URI = 'https://api.airtable.com';
    private const VERSION = 'v0';
    private string $id;

    public function __construct(HttpClientInterface $client, string $id, array $defaultOptionsByRegexp, string $defaultRegexp = null)
    {
        parent::__construct($client, $defaultOptionsByRegexp, $defaultRegexp);

        $this->id = $id;
    }

    public function request(string $method, string $url, array $options = []): ResponseInterface
    {
        $url = sprintf('%s/%s/%s', self::VERSION, $this->id, $url);

        return parent::request($method, $url, $options);
    }

and the current AirtableClient

class AirtableClient

    public function __construct(/* configured AirtableTransport */ HttpClientInterface $httpClient,ObjectNormalizer $objectNormalizer);
    public findX

[liorchamla]

Haha, one thing keeps bugging me :p

When u ask for the HttpClientInterface in a constructor, you think u will actually receive it. Here this is not the case, what you receive in the constructor is AirtableTransport, so why not just create and AirtableTransportInterface and ask for it in the AirtableClient constructor ?

Haha :p

[ismail1432]

so why not just create and AirtableTransportInterface and ask for it in the AirtableClient constructor ?

done https://github.com/yoanbernabeu/Airtable-Client-Bundle/pull/16/commits/94ba27a8cb8897de5e3e7a06b0e442952226633c#diff-4fca284f12d70bb5aeae63b1d3090a19d7d2a047684d5f441cb0d07e41062c24R15

[liorchamla]

so why not just create and AirtableTransportInterface and ask for it in the AirtableClient constructor ?

done 94ba27a#diff-4fca284f12d70bb5aeae63b1d3090a19d7d2a047684d5f441cb0d07e41062c24R15

Wow bro ! U work hard and late :p

See everybody here ?! This is what leads to "good practices", from one simple class, we have now 10 classes and 3 interfaces :D :D

Haha wp everybody :p

[ismail1432]

Thanks @yoanbernabeu !

Before merging I would like to review myself and give you the go :heavy_check_mark: and can you confirm that it's still working? I can't create an account on Airtable :cry: (they said that there are too many requests) I only tried by following the end URL...

[yoanbernabeu]

Thanks @yoanbernabeu !

Before merging I would like to review myself and give you the go and can you confirm that it's still working? I can't create an account on Airtable (they said that there are too many requests) I only tried by following the end URL...

@ismail1432, if you need an Airtable account, I have a "test" key that I can provide you privately if you want ;)

[Spomky]

That's really clever!

if you need an Airtable account, I have a "test" key that I can provide you privately if you want ;)

Few months ago, I heard someone saying he mocks almost nothing (by the way the talk is very interesting). I don't use Airtable, but having real Airtable requests and responses will help a lot in the future. Is it possible to get some request/responses tuples? If yes, I will likely use them to write additional tests.

[ismail1432]

It's OK from my side, I debug each method to see if the URL is well parsed and It's good :sunglasses:

[ismail1432]

Even if I did an ironic comment about the number of classes / interfaces ^^ I also approve this PR ^^

:sweat_smile: Yes I add new classes/interface but at the end we still removing more than adding :ok_hand:

+280 
- 313

[yoanbernabeu]

If it's OK for you, I suggest to merge tomorrow

(today I am blocked by a bug in production with Docker and CORS :( )

[ismail1432]

Have fun for your bug in production, :fire: