hoaproject / Websocket

The Hoa\Websocket library.
https://hoa-project.net/
422 stars 75 forks source link

Need a socket factory #44

Closed Hywan closed 8 years ago

Hywan commented 9 years ago

Will fix #41.

  1. People are likely to use ws:// or wss:// directly. They don't understand that we must use a tcp:// URL. I can't blame them. HTTP Upgrade is just… confusing.
  2. Moreover, there is a need for automatic encryption, because with wss://, we need to instanciate a Hoa\Socket\Client object with tcp://…:443 and then call enableEncryption. Most of the time with ENCRYPTION_TLS (maybe with SSLv2 or SSLv3 sometimes, how to know?).
  3. In addition, we have to deal with URL endpoint (in the Websocket client).

So, yup, this is more like a factory. The same way we have Hoa\Socket\Socket to parse tcp:// and udp:// URL, maybe we can extend it to support ws:// and wss://. It could be great to have an enhanced Hoa\Socket\Socket object where we could be able to register new schemes (like ws:// or wss://) in order to automatically transform a string into a real Hoa\Socket\Socket valid object (with a real tcp:// or udp:// URL). So it requires a little bit of work on Hoa\Socket.

Here is the work I propose:

  1. On Hoa\Socket\Socket:
    1. Introduce a “socket wrapper” interface that declares a factory to transform a URL into a valid Hoa\Socket\Socket object (or a child of),
    2. Introduce the registerWrapper and unregisterWrapper static methods that take a “socket wrapper” interface as an argument,
    3. Declare tcp:// and udp:// as socket wrappers.
  2. On Hoa\Socket\Connection:
    1. In the setSocket method, we receive a URL; we then have to find and apply the appropriated stream wrapper to get a valid Hoa\Socket\Socket object (or a child of).
  3. On Hoa\Websocket\Connection:
    1. We must create Hoa\Websocket\Socket (the “socket wrapper”) that extends Hoa\Socket\Socket,
    2. We must declare the “socket wrapper” ws:// and wss://.

When executing:

new Hoa\Websocket\Client(
    new Hoa\Socket\Client('ws://…')
)

What is going to happen? The ws:// and wss:// URLs are going to be transform into a Hoa\Websocket\Socket object because Hoa\Socket\Client is extending Hoa\Socket\Connection\Connection. This latter will call the setSocket method: It will look for a valid “socket wrapper”, execute it and a valid Hoa\Websocket\Socket will be created. It will be used by Hoa\Websocket\Client very smootly thanks to inheritance, but we will have more information on it, such as: Should it be encrypted? for instance (in the Hoa\Websocket\Client, we would just need to call $connection->getSocket()->isSecure() for instance). Finally, we will have to act accordingly.

Well, that's easier that what I thought!

Finally: We will have to update the documentation.

Hywan commented 9 years ago

See hoaproject/Socket#26 for the work on Hoa\Socket.

Hywan commented 9 years ago

With this strategy, we ensure:

  1. No regression,
  2. No BC,
  3. Only enhancement.

Thoughts @hoaproject/hoackers?

flip111 commented 9 years ago

I find the code hard to reason about.

a Connection\Handler: takes a Connection

a Connection: is a child of Stream i couldn't find this class takes socket information

a Client is a child of Connection takes socket information

a WebSocket\Connection is a child of Handler takes a Connection

a WebSocket\Client: is a child of WebSocket\Connection takes a Socket\Client

So i'm very confused with the "normal" Socket\Connection\Handler wraps the Socket\Connection (is one if it's properties) and the Websocket\Connection suddenly EXTENDS Socket\Connection\Handler

My advice would be to clean up the code first and maybe make some kind of flowchart of what goes where before adding additional factories and other sort of classes. Readability & maintainability are very important when making a codebase that is suppose to be developed with a larger group.

Hywan commented 9 years ago

@flip111 Here is the class: Hoa\Stream\Stream.

The code is very clean actually. And very modular. Check the UML diagram: UML diagram of Hoa\Websocket\Server

More information about socket (connection) handler.

flip111 commented 9 years ago

documentation looks good :+1:

Hywan commented 9 years ago

@flip111 :-)

shulard commented 9 years ago

Hello,

We've introduced the handling of wss:// and ws:// transport layers in PR #47. We've also introduced a new Websocket\Client and Websocket\Server behaviour (construct from String) in the PR #50.

Pull Request #47

The PR #47 use the work done on the Socket library in the PR hoaproject/Socket#27 with the introduction of Socket\Transport wrapper handlers. The Socket\Connection::setSocket method was also updated to use the wrappers if one exists (else it fallback to the previous process): https://github.com/hoaproject/Socket/pull/27/files#diff-1dd508c79928f855d3387b0efa8b3e76R346

In the #47 wrappers for wss:// and ws:// are declared in the Websocket\Connection file and a specific Socket implementation is added :

The composer.json file was also updated to ensure that wrappers are registered all the time: https://github.com/hoaproject/Websocket/pull/47/files#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780R36

Pull Request #50

Then the PR #50 introduce an update in Websocket constructors to simplify the code. Instead of :

new Hoa\Websocket\Server(
    new Hoa\Socket\Server('tcp://127.0.0.1:8889')
);

we can now write :

new Hoa\Websocket\Server('tcp://127.0.0.1:8889');

Merging

To merge all that work, we need to respect the following order :

Hywan commented 9 years ago

Let's start by Hoa\Socket then :-). And thanks for the summary!

salaman commented 9 years ago

Any updates on this? In #55 you said the feature would be merged within a week, however there's been no updates on the PRs since October. Would love to start using it!

Hywan commented 9 years ago

@salaman Like I do :-). Be patient, it will be closed soon. I hope this week or next week, I promise ;-).

Hywan commented 8 years ago

Hoa\Socket part is merged. Now let's go for Hoa\Websocket patches.

Hywan commented 8 years ago

@salaman Almost here! Only #50 remains open. Will be closed very soon (in a couple of days).

Hywan commented 8 years ago

Done!