ruflin / Elastica

Elastica is a PHP client for elasticsearch
http://elastica.io/
MIT License
2.26k stars 734 forks source link

Add support for Symfony HttpClient as Transport #1750

Open damienalexandre opened 4 years ago

damienalexandre commented 4 years ago

I created a new Abstract Transport to use the new Symfony HttpClient component as HTTP client with Elastica.

https://github.com/jolicode/elastically/pull/15

I'm willing to contribute this back to Elastica but I'm not sure it's appropriate to add in the core, as it's not standalone (needs symfony/http-client) and does not provide new features (except for Symfony Framework users).

What do you think, should I create a PR here? Another concern of mine is #944 - one day we might not have transport in Elastica :tada: but rely on the core elasticsearch-php client.

Thanks! :+1: :kissing:

thePanz commented 4 years ago

I like this idea, and more to remove the whole Transport implementations. I didn't know about official clients being implemented in the elasticsearch-php library, that could be nice to have here.

Another idea would be to rely on passing in a PSR-18 client. what do you think?

damienalexandre commented 4 years ago

Yes support for PSR-18 would be a better choice!

It would then add compatibility with all those clients: https://packagist.org/providers/psr/http-client-implementation (including Symfony/HttpClient).

thePanz commented 4 years ago

Do you think it would be a big effort to rely on a PSR-18 client? (or, on the httplug ecosystem?)

damienalexandre commented 4 years ago

You mean adding a PSR-18 enabled transport to Elastica?

I think it's simple, there is only the \Elastica\Transport\AbstractTransport to implement.

(I can't do it right now so if someone have time, go for it, otherwise I add this to my todo!)

thePanz commented 4 years ago

well, I mean "to remove the whole Transport implementations" :) Thus requiring a PSR-18 client to be passed in when building the client.

This would be a breaking-change @ruflin , but would remove all the burden of maintaining all the transports we have at the moment :)

ruflin commented 4 years ago

In general I like the idea. The part I'm concerned about is that it requires users to learn and know about PSR-18 before they can start using Elastica. Is there an option, that we would have a "default" fallback available that just works but still allow to "replace" it for expert users?

My long term thinking around Elastica is: Elastica is only a "method proxy" on top of php-elasticsearch, so all the transport layer is done by the low level client. This would mean we alos don't need PSR-18 as all the methods we implement depend on elasticsearch-php and it is elasticsearch-php that has to deal with it. I wonder if @ezimuel or @philkra have some thoughts on this.

The problem with the above is that it requires much more work, so I'm happy to rip out parts of the code now if we can. The fewer code to maintain, the better. But before we do, we should check what "hacks" we have in our transport layer to make things working.

ezimuel commented 4 years ago

@ruflin, @thePanz, @damienalexandre I'm planning to refactor elasticsearch-php using HTTPlug to support PSR-7 and PSR-18 standards. This is scheduled for version 8.0. If you want to discuss more I created this PR: https://github.com/elastic/elasticsearch-php/issues/990

deguif commented 3 years ago

For keeping track here, there's some progress on ongoing work in https://github.com/elastic/elastic-transport-php

thePanz commented 3 years ago

Thank you for the heads-up @deguif ! Unfortunately they still include Guzzle (also as a requirement in composer.json)

By default, TransportBuilder will use the default values: GuzzleClient as ClientInterface

ezimuel commented 3 years ago

@thePanz Guzzle is the default adapter. If you want to use a different PSR-18 library you can specify as described here. I'm just curios, what's the issue with Guzzle?

thePanz commented 3 years ago

Hi @ezimuel , having Guzzle as a "requirement" library means that it will be installed in the system (even if not used).

This could lead to some issues if, as an example, the project already requires an old version of Guzzle or the project is not compatible with Guzzle 7.

The approach used by HTTPlug is not relying on any actual implementation, see: https://docs.php-http.org/en/latest/httplug/library-developers.html leaving to the project's devs to choose the PSR-7 client of their choice. WDYT?

ezimuel commented 3 years ago

@thePanz Guzzle is the most used HTTP library in PHP projects, that's the reason why I used it as default. You right, the library is installed in the system even if you are not using it (e.g. using an alternative PSR-18 library), but I don't think we have space issue nowadays.

The benefit to have it as require for elastic-transport-php is double: a better UX perspective (everything is ready) and async support. I needed a library for async, unfortunately we don't have a PSR standard for it (not yet). I like the approach of HTTPlug but in our case this will not solve the async part adding also another step for the common PHP developer. In the past, I had issue and I received negative feedback using suggest libraries in composer.

Regarding the support of old version of Guzzle, since we decided to support PHP >= 7.3 we required Guzzle 7.0 that fully support also PSR-18.