Support Symfony PSR-18 HTTP client
#1576
Conversation
|
Can you push the branch to this repo? This will make it easier for folks giving it a try. |
|
Sure, here it is. I will keep it in sync with the one in my fork repo so that I don't have to open a new PR. |
| @@ -111,7 +112,7 @@ public function create(Options $options): HttpAsyncClientInterface | |||
| */ | |||
| private function resolveClient(Options $options) | |||
| { | |||
| if (class_exists(SymfonyHttplugClient::class)) { | |||
| if (class_exists(SymfonyPsr18Client::class) || class_exists(SymfonyHttplugClient::class)) { | |||
There was a problem hiding this comment.
While this might be more of a hypothetical concern if, for any reason, we do end up with the SymfonyHttplugClient or if we fall through and php-http/discovery defaults to SymfonyHttplugClient, we run into the same issue we're trying to solve.
Again, our stance is that we can accept living with an abandoned package for a bit longer. Causing exceptions, even if they do not stem from the SDK, is not ok.
There was a problem hiding this comment.
I can easily remove everything related to the Httplug client, the PSR-18 client is more than enough as explained above. Do you feel safer this way?
There was a problem hiding this comment.
Not for the php-http/discovery case. Let's keep it for the moment. Maybe we get some feedback in #1541
There was a problem hiding this comment.
Not for the php-http/discovery case.
We can technically replace that case with the PSR-18 discovery. The only thing that would change is that we have to require psr/http-client-implementation instead of php-http/async-client-implementation. I may think about an unlikely case where you will not be able to update quietly to the new version because you are missing installed a PSR-18 compatible client, but as far as I see every single HTTP client supported by the autodiscovery that would be resolved now is also PSR-18 compatible, so there should be nothing to do: if it worked before, it should work after.
|
@cleptric do you want to move forward with this? One week should be enough to gather feedback considering the activity of the original issue |
|
@ste93cry please update the PR and move the branch to this repo. I can run some tests this week. |
I've already pushed the branch to this repo too. However I would prefer to keep working on my fork so that I don't need to close this PR, resulting in a loss of the context and conversations we had. In the end, the source branch should not matter much... |
|
Closing this because it's evident that there is a lack of interest in merging this PR, especially now that the new SDK is out. |
To work around issue #1541, we could support the built-in Symfony
PSR-18HTTP client and make it the preferred one to use.Http\Client\Common\PluginClientensures that a non-async client gets wrapped byEmulatedHttpAsyncClientand this allows us to not have to change anything in the rest of the codebase. There is no version of Symfony that doesn't have both thePSR-18and Httplug client, so the possibilities of instantiating theSymfonyHttplugClientclass should be none.Since the
HttpTransportis already syncronous, I don't expect any performance penalty with this change. However, the dependency on thephp-http/message-factorypackage can now be removed safely.