Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix HttpAdapter after egeloen/http-adapter changes #845

Closed
wants to merge 1 commit into from

Conversation

agallou
Copy link
Contributor

@agallou agallou commented May 14, 2015

Streams have changed on PSR7 and egeloen/http-adapter.
This change allows to work with this last version of egeloen/http-adapter.

On a Symfony project it works on the production environnment but there is an error on the debugger of the playbloom/guzzle-bundle bundle. Maybe some changes are required on the bundle or Guzzle but i'd like to validate that before this PR is merged.

Streams have changed on PSR7 and egeloen/http-adapter.
This change allows to work with this last version of egeloen/http-adapter.
@agallou
Copy link
Contributor Author

agallou commented May 14, 2015

cf #727

@ruflin
Copy link
Owner

ruflin commented May 14, 2015

@agallou Thx. Let me know when it is ready to be merged.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 10956ea on agallou:update_http_adapter into * on ruflin:master*.

@ruflin
Copy link
Owner

ruflin commented May 25, 2015

@agallou What you suggest as a next step?

@dbu
Copy link
Contributor

dbu commented Jun 1, 2015

i would recommend to look into php-http (though that might be a bit more heavy refactoring). php-http is the follow up of ivory from what i understood. this PR on FOSHttpCache could help how to do that FriendsOfSymfony/FOSHttpCache#173 - though php-http adapters seem not yet stable.

@im-denisenko
Copy link
Contributor

Related comment: #858 (comment)

@egeloen Could you please give us some insight when php-http will have stable versions of adapter interface and curl adapter?

@ddeboer
Copy link

ddeboer commented Jun 6, 2015

@im-denisenko You’d better contact @sagikazarmark, as he’s the most active developer on php-http right now.

@sagikazarmark
Copy link

There is no ETA for stable ATM, but 0.1 is already out. I think that preview release shows a stable state. Although I am the active developer, @egeloen is some kind of project lead, so I need his blessing to tag 1.0. (But I would like to do it soon)

The curl adapter wasn't top priority, we currently have two guzzle implementations (5, 6), but I am moving it up in the list since there is need for it (See the complete list: php-http/httplug#32)

Why is the curl adapter relevant anyway? You can just use any, can't you?

@ruflin
Copy link
Owner

ruflin commented Jun 8, 2015

@sagikazarmark The curl adapter is the "basic" adapter we use. @im-denisenko I assume that is why you mentioned it specifically?

I suggest we wait with this until everything around PSR-7 settles down.

@sagikazarmark
Copy link

@ruflin Since it is adapter, it can be exchanged, can't it be?

With the new separation it is possible to require only a virtual implementation (php-http/adapter-implementation) and a discover (php-http/discovery). This allows to require any implementation (not forced which) and you can easily discover the installed one using the HttpAdapterDiscovery if it is not configured.

While cURL is good, because you only need the extension installed (which is required by most clients anyway), I think it is even better not relying on any implementation by default. This will be documented, the linked http cache PR is a good example.

@ruflin
Copy link
Owner

ruflin commented Jun 8, 2015

@sagikazarmark Elastica has started because of Memcache and Thrift to have its own implementation of some adapters (including some Http Adapters): https://github.com/ruflin/Elastica/tree/master/lib/Elastica/Transport. As you correctly mentioned, in the future Elastica should not rely on an implementation, especially as Thrift and Memcache as transport are deprecated. Since we have an implementation for the adapter, just removing it means a BC break. I think we should probably remove all adapters and only depend on the discovery, but this needs some refactoring on the Elastica side. To make sure we make BC breaking changes only once, I suggest to wait still a little bit with this one.

@sagikazarmark
Copy link

sagikazarmark commented Jun 8, 2015 via email

@im-denisenko
Copy link
Contributor

I assume that is why you mentioned it specifically?

@ruflin Yep. Curl seems will be most lightweight.

until the adapter is stable

@sagikazarmark It's correct assumption that egeloen/http-adapter will be abandoned after that?

@sagikazarmark
Copy link

@im-denisenko Yup. AFAIK @egeloen plans an LTS release (closing a few issues if possible, etc), but after phly/http became abandoned I am not sure that will be possible.

@ruflin ruflin mentioned this pull request Aug 31, 2015
@joelwurtz
Copy link

Hey, just to inform you that php-http (ivory http adapter replacement) is now in a stable release and process.

Do you want me to give a shot about making a PR about that (like just adding a Handler for the moment, and have the big refactor around PSR7 for later ?)

@damienalexandre
Copy link
Contributor

This may conflict with another network change: the integration of the official php client, which is not PSR7 compatible neither.

@joelwurtz
Copy link

Didn't see this PR thanks,

I think i can make a PR that would allow the official client but also allowing to profit some of the php-http packages without doing BC to the actual client.

WDYT @ruflin should i give a try or have you something else in mind ?

@ruflin
Copy link
Owner

ruflin commented Jan 31, 2016

@joelwurtz I would be more then happy fi you could give it a try. I'm not sure on the schedule of #989 and it should not hold us back. About php-elasticsearch and support for PSR7 we should probably have a conversation with @polyfractal

@polyfractal
Copy link

I have an issue open at the ES-PHP repo to make the client PSR7 compliant (elastic/elasticsearch-php#291). If/when PSR7 compliance is required for the #989 integration, we can expedite the process of adding it to the official client. :)

@ruflin
Copy link
Owner

ruflin commented Jan 31, 2016

@polyfractal Awesome. Lets see if I can expedite #989 somehow

@joelwurtz
Copy link

Will try to do a PR /on elasticsearch-php in the first time then :), after we can update / replace #989

@ruflin
Copy link
Owner

ruflin commented Feb 1, 2016

@joelwurtz 👍 Can you link then the PR here for reference?

@joelwurtz
Copy link

Work begin here elastic/elasticsearch-php#383

@joelwurtz
Copy link

There is some updates about this, as with recents changes from @polyfractal the transport layer is better decoupled from endpoint in elasticsearch-php.

As this was done i was able to figure out a way to integrate PSR7 into elasticsearch-php, by using HTTPlug, without breaking the end user api on the endpoint : elastic/elasticsearch-php#455

If this change is accepted, then i think there is minor things to do in order to have a common factory class for creating a client compatible for both librairies. (so the setup would be the same for elasticsearch-php and Elastica, and the user would just have to choose to consume an elasticsearch service, by using the low level api from elasticsearch-php, or the OO API from Elastica). To be short, both library would depend on the HTTPlug contract for sending requests.

@ruflin
Copy link
Owner

ruflin commented Aug 23, 2016

@joelwurtz Thanks for your work. I will have a closer look at elastic/elasticsearch-php#383

@dinamic
Copy link

dinamic commented Aug 22, 2019

This PR seem obsolete...

Btw, elasticsearch-php PSR7 support seems to have been postponed for next major version - Elasticsearch 8. elastic/elasticsearch-php#291

@ruflin
Copy link
Owner

ruflin commented Aug 27, 2019

Thanks for the ping @dinamic .

Closing this PR as it is a few years old without activity. Please reopen if needed.

@ruflin ruflin closed this Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet