[2.0] Add types

[Jean85]

Q	A
Bug fix?	no
New feature?	no
BC breaks?	yes
Deprecations?	no
Related tickets	partially #115
License	MIT

What's in this PR?

Another PR toward 2.0

Why?

Because PHP 7.0 is now the required minimum.

Checklist

• Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
• Documentation pull request created (not needed?)

To Do

• Should I add : Promise where possible or it may interfere on how the promises works?

[sagikazarmark]

I think we can also drop docblocks where the type is obvious from the hint.

[Jean85]

Maybe we can do it with PHPCSFixer and the no_superfluous_phpdoc_tags + no_empty_phpdoc rules?

[sagikazarmark]

Sounds like a good idea.

[Jean85]

@sagikazarmark PHPCSFixer is not installed, and we have StyleCI... How should I proceed? Do we want to maintain StileCI? Or should we use PHPCSFixer all the way and add a separate job/stage on Travis?

I should open a separate PR for sure...

For now I've fixed the CS of this PR alone, for StyleCI.

[sagikazarmark]

We are working towards pretty ci which relies on phpcsfixer config, so you are free to ignore styleci and use PHPCS fixer only.

[dbu]

lets for now accept what styleci proposes and fix things when we switch to php-cs-fixer with prettyci. there should be no behaviour change whatsoever for style things, so its not critical.

there is an error with a stub in the tests. can you fix that?

[Jean85]

I've fixed the stub and a few other things in 2a80705. I also took the liberty of adding the specs to the dev autoloader (236a8ea) and migrated to ::class (e87785c).

[Jean85]

After #120 this has a lot of conflicts. Fixing now...

[Jean85]

I've fixed all the conflicts. I wanted to add : Promise to the Plugin interface but a lot of tests will fail 😢

[dbu]

looks good to me.

i was wondering if we need a changelog entry, but i think we should make all those clients final classes and then the type declarations are no relevant change anymore. is there any classes in this PR that we explicitly should not declare final?

[Jean85]

I don't know, you or @sagikazarmark should decide on that. I'll try lo list here all the impacted classes (checks will be used to mark classes made final):

• BatchClient: got sendRequest(...): ResponseInterface from PSR-18 and sendRequests(...): BatchResult
• BatchResult
• Deferred
• HttpClientPool, but it's abstract
• LeastUsedClientPool, extends HttpClientPool
• RoundRobinClientPool, extends HttpClientPool
• HttpMethodsClient
• PluginClient
• PluginClientFactory

Also, nearly all plugins are impacted, but I want to give another go to adding : Promise to the Plugin interface.

[Jean85]

I fixed nearly all specs, but I'm not able to fix it for the CookiePlugin. It seems to me that the callbacks used as mock are not working properly:

public function it_does_not_load_cookie_if_expired(RequestInterface $request, UriInterface $uri, Promise $promise)
{
    $cookie = new Cookie('name', 'value', null, 'test.com', false, false, null, (new \DateTime())->modify('-1 day'));
    $this->cookieJar->addCookie($cookie);

    $request->withAddedHeader('Cookie', 'name=value')->shouldNotBeCalled();

    $this->handleRequest($request, function (RequestInterface $requestReceived) use ($request, $promise) {
        if (Argument::is($requestReceived)->scoreArgument($request->getWrappedObject())) {
            return $promise->getWrappedObject();
        }
    }, function () {});
}

The error is:

---- broken examples

    Http/Client/Common/Plugin/CookiePlugin

79 ! does not load cookie if expired
exception [err:TypeError("Return value of Http\Client\Common\Plugin\CookiePlugin::handleRequest() must implement interface Http\Promise\Promise, null returned")] has been thrown.

Anyone can help me here?

[dbu]

lets do the final in a separate pull request. i only brought it up here because if we make the classes final we don't need a changelog.

[dbu]

see also #20 => can you please add a changelog for the abstract class?

@sagikazarmark is our expert in phpspec, maybe he can help with the error?

[Jean85]

@dbu changelog updated. I'll wait for help on the CookiePlugin specs 👍

[sagikazarmark]

I feel like being summoned. 😄

Lemme check...

[sagikazarmark]

It's one of my biggest regret that I didn't pay more attention to tests in this repo. We abused spec testing quite a bit....

Anyway, you could try using this line in the breaking examples:

$this->handleRequest($request, PluginStub::next(), function () {});

We don't really care about the returned promise in those cases.

[Jean85]

It seems to work, great! Thanks!

[dbu]

looks good to me!

lets merge?

[sagikazarmark]

Thanks @Jean85 , outstanding!