[WIP] Transport decoupling and PSR7

[joelwurtz]

This is a attempt to #350 and #291, and also a first step for this : ruflin/Elastica#845

The goal here is to decouple endpoint from transport by using PSR7. Each endpoint can be transformed into a RequestInterface which then can be used by any client using this interface.

It use https://github.com/php-http/message-factory to allow people to choose the implementation for PSR7.

I made this in a none BC break but also added a deprecated flag to inform user that this will change in 3.0

Not sure if this is complete, anyway it's just a small step to bring discussion about this.

[ruflin]

I didn't check in detail but I assume performRequest is mainly used internally. Should we switch this to createRequest already?

[joelwurtz]

Yes that was something i was thinking about, but don't know if all of this must be done in one PR or should we separate ?

[ruflin]

It would be a good proof that it works as expected :-) But lets see what @polyfractal thinks as he is the one that knows side effects / risk etc best.

[polyfractal]

performRequest() is 100% an internal method call, so it should be safe to convert everything to createRequest() internally.

However, I don't think we can remove performRequest() before 3.0 because a user can inject their own endpoints, and so removing performRequest() would break backwards compat for those custom endpoints. The best we can do is deprecate for now.

Regarding the PRs, we can lump the refactoring into this one, or split it into smaller chunks as you/me find time to work on it.

It might be nice to refactor one of the smaller namespaces, like NodesNamespace just to ensure the tests pass and everything works as expected? Then the rest we can do as time allows?

Note: I'm going to be pretty busy for the next week or two, need to prepare my talks for Elasticon :)

[joelwurtz]

Will continue this and begin to replace the performRequest by createRequest everywhere it is done.

Also the performRequest use an options parameters to set specific options to the underlying http client, this is not possible anymore.

AFAIK it was only used for verbose in some parts, but don't know the usage it was done by users ?

It might be nice to refactor one of the smaller namespaces, like NodesNamespace just to ensure the tests pass and everything works as expected? Then the rest we can do as time allows?

Ok for me

Want also to change the client to have something which respect more PSR7 interface, but this will be another PR, in the meantime i will transform back the PSR7 request into parameters for guzzlering.

[polyfractal]

Also the performRequest use an options parameters to set specific options to the underlying http client, this is not possible anymore.
AFAIK it was only used for verbose in some parts, but don't know the usage it was done by users ?

Oh, there's no way to send custom parameters to the underlying HTTP implementation? That's sorta a deal breaker?

I'm looking at the change closer now, and seem some issues. I'm not familiar with PSR7 enough to say if this is a problem or not, but here are some potential snags:

• We need signal to the client that a request should be run in async mode, this is done through setting ['client']['future']. This isn't technically passed to RingPHP, but is part of the endpoint's $options map. Same for ['client']['ignore'], which tells the client to ignore certain classes of HTTP errors so an exception is not thrown
• Currently you can set custom curl options/headers through ['client']['curl']. I believe PSR7 allows setting this kind of information, but the translation will need to be done
• Verbosity can be toggled via ['client']['verbose'] as you mentioned. This is used in a few places internally, and is a very useful debugging tool for the end-user. It instructs RingPHP to emit all the info from CURLOPT_VERBOSE flag. There's likely an equivalent in PSR-7 land, but it'd need translation for sure.
• Variety of other things, ['client']['never_retry'], ['client']['timeout'], etc

Full disclosure, I'm dubious of the usefulness of converting the internals to PSR7. I'm not sure what it really gains us? In my eyes I think it makes more sense to have:

• All endpoints can return a PSR-7 compliant request object instead of executing it. This allows the user to do whatever they want to the request, execute it with a different client entirely, etc.
• An endpoint that blindly executes PSR-7 compliant request objects. This allows ES-PHP to execute any arbitrary HTTP request the user builds (including ones that were generated by the client but subsequently modified), and still get to leverage the ES-specific features like sniffing.
• Responses should probably be PSR-7 compliant, so that they can fit into anyone's PSR-7 pipeline. Relatively inoculous.

So in light of that, perhaps it makes more sense to just write shims that can convert to/from PSR-7, instead of rewiring all the guts? I'm just not sure I see much added benefit. I'm fairly happy with RingPHP and don't really see the need to support umpteen different (nearly identical) HTTP libs. They all either fallback to streams or libcurl and we don't need many of the features that full Guzzle provides, etc.

Happy to be convinced otherwise, but that's my current feelings :) Perhaps I just need to read into PSR-7 some more, it's been a while since I last looked at it.

[joelwurtz]

PSR7 is only about the request, response, url, stream and file used in an Http context, it tell nothings about a client and how it must be send.

So in light of that, perhaps it makes more sense to just write shims that can convert to/from PSR-7, instead of rewiring all the guts?

That is my intention for this PR, do not go beyond the PSR7, so it will be easy to accept this withtout breaking too much stuff.

About the client options thing, i may have jump too far by saying this is not possible anymore.
In fact, it may need extra works to decouple parameters from the $params and pass them to the transport, but should not be a big problem in this PR. I was just pointing the fact that the endpoint does not have the information about the client anymore, so it will induce more work for end user when switching (as he will need to separate the parameters of the endpoint from the client, but IMO this is a good thing as they are unrelated).

About guzzlering my intention is to propose, after this one, another pull request to use httplug.

Like you said :

don't really see the need to support umpteen different (nearly identical) HTTP libs.

Your are right, That's is cleary not a job for an api client such as elasticsearch-php, and that's what this library is about, providing a simple contract for http clients and providing adapters for all existing implementations (we don't have the guzzlering implementation, but AFAIK it shouldn't be so difficult to add).

The benefit of supporting multiple client through the same interface is to let the final user choose the implementating but still having the same contract.
Then it allows using the same contract for differents api (there is others libraries who are in the process to switch to httplug) and this is clearly a win as the final user can have all the wanted behavior (such as logging, profiling, caching, etc ...) on this contract and they can be used everywhere.

There is other things in httplug that will help to reduce your maintenance cost, but don't want to talk too much of this here as it's not the subject for this PR (will do a long explanation of pros / cons in the following PR :) )

[polyfractal]

Ok. :) I probably just don't understand PSR7 and the ramifications enough (I've only briefly skimmed the spec, and quite a while ago).

As I said before, I'm going to be pretty unavailable for the next two-ish weeks. I'll try to make myself available if/when you have questions, and when I get back I'll dive into the PSR7 spec more deeply and start helping out with the integration.

Thanks for the PR and working on this! :)

[polyfractal]

Ok, I'm back from the Elasticon trip. I want to fix up these failing PHP7 tests and triage some of the smaller issues/PRs, then I'll start helping out with this PR

Thanks for being patient :)

[polyfractal]

Hmm, I just ran the tests and this fails because the NodeNamespace is now using createRequest, but the ClientBuilder hasn't cutover to the method yet and doesn't set a factory:

No factory found for creating the request

I don't think this will be able to done incrementally or without a bwc break? E.g. if we cut everything over to the new method, that means any user-injected bulk endpoints would be broken (because the injected factory would conflict with the existing bulk definition, which injects a serializer:

return new $fullPath($transport, $serializer);

Should we target this for the next major? Happy to entertain alternatives solutions :)

[joelwurtz]

Hey sorry for the delay, i was busy in a project and didn't have the time to look at this.

Yes i think i agree with you by tring to go further it was really hard to do changes without BC break, it's still possible but need a big amount of time.

So it may be better to do this in the next major version, i have then a bigger proposal for this :

I'm one of the maintainer of all the https://github.com/php-http libraries and when looking at elasticsearch-php i really like the ConnectionPool part to handle a cluster and IMO this is something that can be useful for others PHP API clients. I have already begin to look at this to have an implementation in our library (look at php-http/client-common@364030d)

So the goal would be to use the https://github.com/php-http/httplug interface for this library ant the ConnectionPool in the client-common library.

One of the big advantages is it will allow people to reuse their client with others api (we already have some library using httplug and more to comes). For GuzzleRing it would be possible to keep it and just add a wrapper around it to respect the interface of httplug.

This would remove a lot of codes here and then you can focus only on the elasticsearch-php part of the API and not the connection (i think you will just need to provide some sort of factory so users of this library can easily have a ready to use elasticsearch client)

If we want to go further, to also remove a lot of maintenance, we can use a OpenAPI (swagger) schema and generate the client with it, that's something that i have done in this library https://github.com/docker-php/docker-php by using https://github.com/jolicode/jane-openapi

Using this really save me a lot of times (i update my library from docker 1.9 to docker 1.10 with all new api calls in about 1 hour)

However i don't know if all the elasticsearch calls can be under a OpenAPI schema (but the spec is really flexible)

WDYT ?

[joelwurtz]

Closing this, as some work has been done in others commits, and rebasing will be too much work, will do an issue to track my ideas and smaller PR to achieve this.