Socket issues

[Rid]

We recently made some changes to https://github.com/beluga-php/docker-php/blob/master/src/DockerClientFactory.php based on beluga-php/docker-php-api#14

There's a few issues around the return types, and discovery classes need to be updated to psr17/18.

However the main issue is around sockets, specifically Guzzle only supports sockets via curl opts, for example guzzle/guzzle#1962. However because we're using PluginClient there's no way to pass the options to guzzle.

In symfony/http-client it uses the bindto option to bind to a unix socket using the same curl_opt (https://github.com/symfony/http-client-contracts/blob/main/HttpClientInterface.php).

Unfortunately this makes Unix socket communication implementation specific and not something which could be supported using PluginClient.

Therefore we need to make a decision of whether to move back to php-http/socket-client or make our library slightly more opinionated and use a specific http client.

socket-client is going to be abandoned shortly, however forcing a http client is also not ideal as it may conflict with an existing client, or the user may have a different preference.

Taking everything into account, I think switching back to socket-client and either making a PR to fix the issues, or creating an issue with the maintainer there is probably the best way forward right now. However I'd definitely like to hear what others think?

@flavioheleno

[Rid]

It looks like Plesk have forked socket-client here: https://github.com/plesk/socket-client

They have renamed the package plesk/socket-client and made the necessary updates for PSR7.

As Plesk are well known in this space, I'm happy to continue with their fork for the time being unless php-http/socket-client becomes active again.

We also have the option of either taking over maintenance of php-http/socket-client or bringing it directly into our repo, I'd like to know your thoughts on this.

I'll make the change for this now.

[Rid]

Ok so I've:

• Reverted to plesk/socket-client
• Created and run a script to detect which packages we're directly using: https://gist.github.com/Rid/d599558f3b92e7bfcbd766e099bf7160
• Also used composer-require-checker check composer.js to check for ext dependencies
• Added directly used packages to composer.json
• Ran php-cs-fixer & the linter
• Updated the client endpoints to use their actual API versions (all versions were using the v1.41 version regardless of branch/release)
• Added better error reporting for client <-> API issues such as version mismatch

[Rid]

Looking at the code it appears that we don't support the http schema when using the socket client (it looks like an issue even in the old upstream repo). Trying to use http results in this issue: #56 (comment)

If we do want to support connection over http (as shown in https://docs.docker.com/config/daemon/remote-access/), we should also include a Psr18FactoryDiscovery and use that as the client based on whether it's unix:// or not.

@flavioheleno what do you think?

[flavioheleno]

first of all, great work @Rid!

check my comments below :)

We recently made some changes to https://github.com/beluga-php/docker-php/blob/master/src/DockerClientFactory.php based on beluga-php/docker-php-api#14

There's a few issues around the return types, and discovery classes need to be updated to psr17/18.

Probably I should have worked on this on a specific branch instead of doing it straight into master, a healthier development process than what we do now.

However the main issue is around sockets, specifically Guzzle only supports sockets via curl opts, for example guzzle/guzzle#1962. However because we're using PluginClient there's no way to pass the options to guzzle.

In symfony/http-client it uses the bindto option to bind to a unix socket using the same curl_opt (https://github.com/symfony/http-client-contracts/blob/main/HttpClientInterface.php).

Unfortunately this makes Unix socket communication implementation specific and not something which could be supported using PluginClient.

Therefore we need to make a decision of whether to move back to php-http/socket-client or make our library slightly more opinionated and use a specific http client.

I wonder why FIG does not have a PSR-15-like for HTTP Clients yet. Could this help us sort out this problem or would it just add to its complexity?

socket-client is going to be abandoned shortly, however forcing a http client is also not ideal as it may conflict with an existing client, or the user may have a different preference.

Taking everything into account, I think switching back to socket-client and either making a PR to fix the issues, or creating an issue with the maintainer there is probably the best way forward right now. However I'd definitely like to hear what others think?

One alternative is to replace client discovery (php-http/discovery or psr-discovery/discovery) with a custom discovery factory that sets up the client according to our needs. WDYT?

---

It looks like Plesk have forked socket-client here: https://github.com/plesk/socket-client

They have renamed the package plesk/socket-client and made the necessary updates for PSR7.

As Plesk are well known in this space, I'm happy to continue with their fork for the time being unless php-http/socket-client becomes active again.

We also have the option of either taking over maintenance of php-http/socket-client or bringing it directly into our repo, I'd like to know your thoughts on this.

I'll make the change for this now.

I'd rather replace php-http/socket-client with plesk/socket-client than taking over the maintenance of it. I think the best approach is if we could rely 0% on HTTP Client implementation - but I do understand that it may not be feasible right now.

---

Ok so I've:

* Reverted to [plesk/socket-client](https://github.com/plesk/socket-client)

* Created and run a script to detect which packages we're directly using: https://gist.github.com/Rid/d599558f3b92e7bfcbd766e099bf7160

Cool! I thought this was what composer-require-checker did! How do they differ from each other?

* Also used `composer-require-checker check composer.js` to check for ext dependencies

* Added directly used packages to composer.json

* Ran php-cs-fixer & the linter

* Updated the client endpoints to use their actual API versions (all versions were using the v1.41 version regardless of branch/release)

Great catch!

* Added better error reporting for client <-> API issues such as version mismatch

Awesome addition! The current DX is quite poor IMHO, this is a important addition to the right direction!

---

Looking at the code it appears that we don't support the http schema when using the socket client (it looks like an issue even in the old upstream repo). Trying to use http results in this issue: #56 (comment)

If we do want to support connection over http (as shown in https://docs.docker.com/config/daemon/remote-access/), we should also include a Psr18FactoryDiscovery and use that as the client based on whether it's unix:// or not.

My objective for this package is that it would be 100% independent on any sort of implementation and just rely on PSR contracts, so the end users could throw in their own preferred implementation for each contract. I think we should open a new branch where we start working towards that.

[Rid]

One alternative is to replace client discovery (php-http/discovery or psr-discovery/discovery) with a custom discovery factory that sets up the client according to our needs. WDYT?

I think it's a good idea, I wonder if there's any standard way to discover the unix socket connection options? Or would this have to be hard-coded dependent on the implementation?

Cool! I thought this was what composer-require-checker did! How do they differ from each other?

I find that composer-require-checker misses a lot of dependencies, so I use both to cover all bases.

My objective for this package is that it would be 100% independent on any sort of implementation and just rely on PSR contracts, so the end users could throw in their own preferred implementation for each contract. I think we should open a new branch where we start working towards that.

I agree that it should be the aim, it a shame that TCP/Unix socket transport is not part of the PSR-18 spec, it would have made it a lot easier!