Review

[sagikazarmark]

Hey @shulard

I finally got some time to review your work, and I am quite satisfied with it. Well done! 👍

I have a few minor thoughts:

I can see a ReactMessageFactory in the repo. I think things like this can be part of the adapter. Since it is not injected anywhere, I see no reason to have it separated. The request builder method is also in the adapter class. Are there any places where it is reused?

ReactPromiseAdapter and ReactHttpAdapter can be easily mixed. I think ReactPromise is fine enough (see the Guzzle 6 adapter)

I can see some header normalization in the request builder part, for example adding Content-Length header. Is this something which is necessary for react or just part of a regular normalization? If it is the latter, it is preferred to keep adapters/clients as pure as possible, and any further Request/Response processing should be placed in plugins. We already have a ContentLength plugin.

You used two terminology for messages: psr7Something (Request/Response) and Something (Request/Response). The latter two are the react messages I guess. In my opinion it would be better to apply special names for the react message objects and use the plain version for PSR-7 messages. The reason behind is when someone say "request" or "response" the first thing that everyone think about is the PSR-7 version, not the react one. This could cause a lot of confusion (actually did in the past).

There is something that I don't understand in the adapter's constructor. Here is what happens from my point of view:

1. The loop is being checked. If none is passed, we build one.
2. The client is being checked. If none is passed, we create one.
3. If a client is passed, the loop is being checked again, no matter if we created it before or not. If it is not passed, we throw an exception.

My question is: if we create a loop, why do we need to check it AFTER we already did (and possibly created one)? If a client is passed, then we don't need to create a fallback loop, as it will never be used.

I would probably do the following:

if (null !== $client && null === $loop) {
    throw new \RuntimeException('The Loop must be passed together with the Client');
}

$this->loop = $loop ?: ReactFactory::buildEventLoop();
$this->client = $client ?: ReactFactory::buildHttpClient($this->loop);

I think this is easier to read.

There are some CS things to be fixed, but StyleCI will do that.

Great work @shulard, thank you very much.

[shulard]

Hello !

I've commited all the required updates. You're right on all of them 👍
I've also updated all the others issues, I think this package is now up to date.