Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Declare the reconnect parameter deprecated in the disconnect #11

Merged
merged 2 commits into from
Mar 6, 2024
Merged

Declare the reconnect parameter deprecated in the disconnect #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e231d05
Declare the reconnect parameter deprecated in the disconnect
msmakouz Mar 6, 2024
2b4a7dd
Fix Psalm issues
msmakouz Mar 6, 2024
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
13 changes: 12 additions & 1 deletion src/Payload/Disconnect.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,21 @@

final class Disconnect
{
/**
* @deprecated
*/
public bool $reconnect = false;

/**
* @param bool $reconnect This parameter is no longer used since v2.0.1 due to the removal of this option in
* centrifugal/centrifugo v5.0.0 API. It will be removed in v3.0.0.
*/
public function __construct(
public readonly int $code,
public readonly string $reason,
public readonly bool $reconnect = false
bool $reconnect = false
) {
/** @psalm-suppress DeprecatedProperty */
$this->reconnect = $reconnect;
}
}
1 change: 0 additions & 1 deletion src/RPCCentrifugoApi.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@ public function disconnect(
new DTO\Disconnect([
'code' => $disconnect->code,
'reason' => $disconnect->reason,
'reconnect' => $disconnect->reconnect,
])
);
}
Expand Down
6 changes: 5 additions & 1 deletion src/Request/AbstractRequest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,15 @@ final public function error(int $code, string $message, bool $temporary = false)
$this->sendResponse($response);
}

/**
* @param bool $reconnect This parameter is no longer used since v2.0.1 due to the removal of this option in
* centrifugal/centrifugo v5.0.0 API. It will be removed in v3.0.0.
*/
final public function disconnect(int $code, string $reason, bool $reconnect = false): void
{
$response = $this->getResponseObject();
$response->setDisconnect(
new Disconnect(\compact('code', 'reason', 'reconnect')),
new Disconnect(\compact('code', 'reason')),
Comment on lines +63 to +71
Copy link

Choose a reason for hiding this comment

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

The disconnect method documentation correctly indicates the deprecation of the $reconnect parameter. However, the method still accepts and processes this parameter. As with the Disconnect class, consider adding a deprecation warning when this parameter is used, to better inform developers of its impending removal.

final public function disconnect(int $code, string $reason, bool $reconnect = false): void
{
    if ($reconnect !== false) {
        trigger_error('The $reconnect parameter is deprecated and will be removed in v3.0.0.', E_USER_DEPRECATED);
    }
    // Existing implementation...
}

);

$this->sendResponse($response);
Expand Down
2 changes: 1 addition & 1 deletion src/Request/Connect.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ private function mapSubscriptions(array $subscriptions): array
foreach ($subscriptions as $name => $subscription) {
$sub = new DTO\SubscribeOptions();

if ($subscription->expireAt) {
if ($subscription->expireAt !== null) {
$sub->setExpireAt($this->parseExpiresAt($subscription->expireAt));
}

Expand Down
3 changes: 3 additions & 0 deletions src/Request/RequestInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ public function error(int $code, string $message, bool $temporary = false): void

/**
* Send disconnect response to Centrifugo server.
*
* @param bool $reconnect This parameter is no longer used since v2.0.1 due to the removal of this option in
* centrifugal/centrifugo v5.0.0 API. It will be removed in v3.0.0.
*/
public function disconnect(int $code, string $reason, bool $reconnect = false): void;
}
53 changes: 53 additions & 0 deletions tests/Unit/RPCCentrifugoApiTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use PHPUnit\Framework\TestCase;
use RoadRunner\Centrifugo\CentrifugoApiInterface;
use RoadRunner\Centrifugo\Exception\CentrifugoApiResponseException;
use RoadRunner\Centrifugo\Payload\Disconnect;
use RoadRunner\Centrifugo\RPCCentrifugoApi;
use RoadRunner\Centrifugal\API\DTO\V1 as DTO;
use Spiral\Goridge\RPC\Codec\ProtobufCodec;
Expand Down Expand Up @@ -72,4 +73,56 @@ public function testPublishErrorHandling(): void

$this->api->publish(channel: 'foo-channel', message: \json_encode(['foo' => 'bar']), skipHistory: true, tags: ['baz', 'baf']);
}

public function testDisconnectWithDisconnectObject(): void
{
$this->rpc->shouldReceive('call')
->once()
->withArgs(fn(
string $method,
DTO\DisconnectRequest $request,
string $responseClass
): bool => $method === 'centrifuge.Unsubscribe'
&& $request->getUser() === 'foo-user'
&& $request->getClient() === 'foo-client'
&& $request->getSession() === 'foo-session'
&& $request->getDisconnect()->getCode() === 400
&& $request->getDisconnect()->getReason() === 'foo-reason'
&& $responseClass === DTO\DisconnectResponse::class
)
->andReturn(new DTO\DisconnectResponse);

$this->api->disconnect(
user: 'foo-user',
client: 'foo-client',
session: 'foo-session',
disconnect: new Disconnect(code: 400, reason: 'foo-reason'),
);
}

public function testDisconnectWithDisconnectObjectAndDeprecatedReconnect(): void
{
$this->rpc->shouldReceive('call')
->once()
->withArgs(fn(
string $method,
DTO\DisconnectRequest $request,
string $responseClass
): bool => $method === 'centrifuge.Unsubscribe'
&& $request->getUser() === 'foo-user'
&& $request->getClient() === 'foo-client'
&& $request->getSession() === 'foo-session'
&& $request->getDisconnect()->getCode() === 400
&& $request->getDisconnect()->getReason() === 'foo-reason'
&& $responseClass === DTO\DisconnectResponse::class
)
->andReturn(new DTO\DisconnectResponse);

$this->api->disconnect(
user: 'foo-user',
client: 'foo-client',
session: 'foo-session',
disconnect: new Disconnect(code: 400, reason: 'foo-reason', reconnect: true),
);
Comment on lines +103 to +126
Copy link

Choose a reason for hiding this comment

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

The testDisconnectWithDisconnectObjectAndDeprecatedReconnect method tests the disconnect functionality with the deprecated reconnect parameter. As with the previous test file, ensure that this test is clearly marked or documented as testing deprecated behavior.

Consider adding a comment or documentation to clarify that this test is specifically for ensuring backward compatibility with the deprecated reconnect parameter.

}
}
6 changes: 3 additions & 3 deletions tests/Unit/Request/AbstractRequestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public function testDisconnect(): void
{
$worker = $this->createWorker(function (Payload $arg) {
$expects = new Payload((new ConnectResponse())
->setDisconnect(new Disconnect(['code' => 111, 'reason' => 'some', 'reconnect' => false]))
->setDisconnect(new Disconnect(['code' => 111, 'reason' => 'some']))
->serializeToString()
);

Expand All @@ -115,11 +115,11 @@ public function testDisconnect(): void
$req->disconnect(111, 'some');
}

public function testDisconnectWithReconnect(): void
public function testDisconnectWithDeprecatedReconnect(): void
{
$worker = $this->createWorker(function (Payload $arg) {
$expects = new Payload((new ConnectResponse())
->setDisconnect(new Disconnect(['code' => 111, 'reason' => 'some', 'reconnect' => true]))
->setDisconnect(new Disconnect(['code' => 111, 'reason' => 'some']))
->serializeToString()
);

Comment on lines 115 to 125
Copy link

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [118-126]

The testDisconnectWithDeprecatedReconnect method tests the disconnect functionality with the deprecated reconnect parameter. While it's important to test deprecated features for backward compatibility, ensure that this test is clearly marked or documented as testing deprecated behavior, to avoid confusion.

Consider adding a comment or documentation to clarify that this test is specifically for ensuring backward compatibility with the deprecated reconnect parameter.

Expand Down
Loading