Add Socket.IO v2 support #152
Conversation
There was a problem hiding this comment.
Basically, if I understand it correctly, it's just that the sent information has some more content after the closing } when doing the handshake ? IMO it would be better to either just extend the Socket 1.X class, and replace the upgrade method, or even better, do what you did with the closing brace on Socket 1.x.
I'm pretty sure that some other things may have changed, but as I stated in #135, we are not actively maintaining this lib, my knowledge is then really limited. So basically I think that if you are looking for a fix, just patching the SocketIO\Version1X class should be enough, until there is a real support for SocketIO 2.0.
Unless I missed something else ?
| * | ||
| * @author Baptiste Clavié <baptiste@wisembly.com> | ||
| * @link https://tools.ietf.org/html/rfc6455#section-5.2 Websocket's RFC | ||
| */ |
|
I'm currently checking whether the commit I pushed works, I tried to move what I thought I could into 1X and leaving only what must stay in 2X. |
src/Engine/SocketIO/Version1X.php
Outdated
| @@ -193,7 +195,7 @@ private function upgradeTransport() | |||
| 'transport' => static::TRANSPORT_WEBSOCKET]; | |||
|
|
|||
| $url = sprintf('/%s/?%s', trim($this->url['path'], '/'), http_build_query($query)); | |||
| $key = base64_encode(sha1(uniqid(mt_rand(), true), true)); | |||
| $key = base64_encode(random_bytes(20)); | |||
There was a problem hiding this comment.
that's not good, it is php7+ only (we don't have the polyfill too, and I don't want to introduce it if we can avoid it), and this lib supports 5.4+. Supporting 7+ only would be changing a lot of stuff (not that I'm against it, but if we are gonna make it to 7.0, everything should be adapted)
There was a problem hiding this comment.
(I meant the call to random_bytes)
|
@Taluu See the updated version. Apparently the break's main reason was the previously mentioned extra data after the JSON plus the |
src/Engine/SocketIO/Version1X.php
Outdated
| $url = sprintf('/%s/?%s', trim($this->url['path'], '/'), http_build_query($query)); | ||
| $key = base64_encode(random_bytes(20)); | ||
| $key = base64_encode(openssl_random_pseudo_bytes(16)); |
There was a problem hiding this comment.
Not good either, as it requires openssl. If you absolutely need 20 bytes encoded, you can pass the second argument of sha1 to true, it will generate you a binary digest with a length of 20 bytes,
src/Engine/SocketIO/Version1X.php
Outdated
| 'transport' => static::TRANSPORT_WEBSOCKET]; | ||
|
|
||
| if ($this->options['version'] === 2) | ||
| $query['use_b64'] = $this->options['use_b64']; |
|
Hum, I see for the header length. You could always crop it to the maximum number of chars, if on version 2 then I guess ? |
src/Engine/SocketIO/Version1X.php
Outdated
| @@ -167,7 +167,9 @@ protected function handshake() | |||
| throw new ServerConnectionFailureException; | |||
| } | |||
|
|
|||
| $decoded = json_decode(substr($result, strpos($result, '{')), true); | |||
| $open_curly_at = strpos($result, '{'); | |||
| $todecode = substr($result, $open_curly_at, strrpos($result, '}')-$open_curly_at+1); | |||
There was a problem hiding this comment.
tab to space please (code style)
|
Fixed the tab/space inconsistency & added a check to cut the hash length down for v2. |
|
Still missing a brace of two on "simple conditions" but I'll merge it anyway, Thanks @SeinopSys (I'll make a release too) |
|
Release done : https://github.com/Wisembly/elephant.io/releases/tag/v3.3.0 |
|
Sorry about that, leaving them off has become a habit of mine. I'm glad I could help keep the lib flowing for a little longer. |
|
I've just noticed that I left a few |
|
I'm pretty used to miss them, so some others it's not that big of a deal. Will fix if I ever have to fix something in Elephant. |
My site depends on this library for communicating with a separately running Node.js socket.io server, and I noticed that as of version 2.0.0 of socket.io Elephant.IO is failing to connect to the socket.io server and sending messages.
Therefore I dug into the code and messed around with it until the Node.js server was logging that it got the messages again. I can't guarantee with 100% certainty that this change set will permanently fix the issues as I don't have deep understanding of WebSockets or socket.io's API, but for the time being I managed to send messages with it and I consider that to be better than an error message saying
I'd highly appreciate if this fix could be merged for the sake of anyone else trying to upgrade.