Skip to content
This repository was archived by the owner on Aug 25, 2022. It is now read-only.

skip handshake if using websocket as transport. #197

Open
adamzul wants to merge 5 commits into
base: master
Choose a base branch
from
Open

skip handshake if using websocket as transport. #197

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8521357
skip handshake if using websocket as transport. websocket transport i…
adamzul Feb 8, 2020
38916a9
Update composer.json
adamzul Feb 9, 2020
7c42f34
Update composer.json
adamzul Feb 9, 2020
9008bd1
Update composer.json
adamzul Feb 9, 2020
cc726ff
fixing if session id not found
adamzul Feb 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/Engine/SocketIO/Version1X.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ public function connect()
return;
}

$this->handshake();
if($this->options['transport'] != 'websocket'){
Copy link
Contributor

@peter279k peter279k Feb 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make coding style consistency? And this code snippet should be:

......
if ($this->options['transport'] != 'websocket') {
......

Copy link
Contributor

@Taluu Taluu Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be if (...) {

Copy link
Author

@adamzul adamzul Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

give space between ')' and '{' ?

Copy link
Contributor

@peter279k peter279k Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Taluu, maybe it should consider some coding style to check during Travis CI build.

We can consider using PHP_CodeSniffer or PHP-CS-Fixer.

Do you have any idea about this?

Copy link
Contributor

@Taluu Taluu Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, adding this (with maybe building through GA instead of Travis) would be awesome. Not sure I have the time for that currently though, especially as I'm not really maintaining it (because not using it...) currently...

$this->handshake();
}

$protocol = 'http';
$errors = [null, null];
Expand Down Expand Up @@ -239,7 +241,7 @@ protected function handshake()
*/
protected function upgradeTransport()
{
$query = ['sid' => $this->session->id,
$query = ['sid' => isset($this->session->id) ? $this->session->id : null,
Copy link
Contributor

@peter279k peter279k Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the package requires php-7.1 version at least.

And it can consider using the ?? syntax:

......
$this->session->id ?? null,
......

This comment was marked as outdated.

Sign in to view

Copy link
Contributor

@Taluu Taluu Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, didn't remember it was bumped 10 months ago. 😂

(We should still bump to maintained php versions though)

Copy link
Author

@adamzul adamzul Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i will update it

'EIO' => $this->options['version'],
'transport' => static::TRANSPORT_WEBSOCKET];

Expand Down Expand Up @@ -308,8 +310,10 @@ protected function upgradeTransport()
*/
public function keepAlive()
{
if ($this->session->needsHeartbeat()) {
$this->write(static::PING);
if($this->session){
Copy link
Contributor

@peter279k peter279k Feb 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be:

......
if ($this->session) {
......

Copy link
Contributor

@Taluu Taluu Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better :

if ($this->session === null) {
	return;
}

// ...

Copy link
Contributor

@Taluu Taluu Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then we can't keep alive the connection, it will be closed at some point.

Copy link
Author

@adamzul adamzul Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe we can by pass run write if session null?

if ($this->session->needsHeartbeat()) {
$this->write(static::PING);
}
}
}
}