Declare the reconnect parameter deprecated in the disconnect #11
Conversation
WalkthroughThe updates focus on deprecating the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #11 +/- ##
============================================
+ Coverage 75.66% 78.93% +3.27%
Complexity 147 147
============================================
Files 23 23
Lines 489 489
============================================
+ Hits 370 386 +16
+ Misses 119 103 -16 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (6)
- src/Payload/Disconnect.php (1 hunks)
- src/RPCCentrifugoApi.php (1 hunks)
- src/Request/AbstractRequest.php (1 hunks)
- src/Request/RequestInterface.php (1 hunks)
- tests/Unit/RPCCentrifugoApiTest.php (2 hunks)
- tests/Unit/Request/AbstractRequestTest.php (2 hunks)
Additional comments: 5
src/Payload/Disconnect.php (1)
- 9-12: The
reconnectproperty is correctly marked as deprecated. This is a good practice to inform users of the library about the upcoming removal of this property in future versions.src/Request/RequestInterface.php (1)
- 46-48: The documentation for the
disconnectmethod correctly notes the deprecation of the$reconnectparameter. This is a good practice for informing users about deprecated features and aligning with the updated external API.tests/Unit/Request/AbstractRequestTest.php (1)
- 102-102: The
testDisconnectmethod correctly tests thedisconnectfunctionality without using the deprecatedreconnectparameter. This aligns with the updated API behavior.tests/Unit/RPCCentrifugoApiTest.php (1)
- 77-100: The
testDisconnectWithDisconnectObjectmethod correctly tests thedisconnectfunctionality using theDisconnectobject without the deprecatedreconnectparameter. This aligns with the updated API behavior.src/RPCCentrifugoApi.php (1)
- 158-163: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [97-126]
The
disconnectmethod has been updated to no longer pass thereconnectparameter in theDTO\Disconnectobject, aligning with the updated centrifugal/centrifugo v5.0.0 API. This change correctly reflects the deprecation of thereconnectparameter and ensures the library remains compatible with the latest API standards.
src/Payload/Disconnect.php
Outdated
| /** | ||
| * @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 | ||
| ) { | ||
| $this->reconnect = $reconnect; |
There was a problem hiding this comment.
The constructor still accepts the $reconnect parameter, which is marked for removal in future versions. While it's good to maintain backward compatibility, it's important to ensure that this deprecated feature does not cause confusion. Consider adding a trigger for a deprecation warning when this parameter is used, to inform developers of its impending removal.
public function __construct(
public readonly int $code,
public readonly string $reason,
bool $reconnect = false
) {
if ($reconnect !== false) {
trigger_error('The $reconnect parameter is deprecated and will be removed in v3.0.0.', E_USER_DEPRECATED);
}
$this->reconnect = $reconnect;
}| /** | ||
| * @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')), |
There was a problem hiding this comment.
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...
}| $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() | ||
| ); | ||
|
|
There was a problem hiding this comment.
📝 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.
| 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), | ||
| ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/Payload/Disconnect.php (1 hunks)
- src/Request/Connect.php (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/Payload/Disconnect.php
Additional comments: 1
src/Request/Connect.php (1)
- 106-106: The change to explicitly check if
expireAtis not null, instead of relying on its truthiness, is a good practice for clarity and avoiding potential bugs related to falsy values that are not null. This aligns well with the PR's objectives of ensuring compatibility with updated API specifications.
The reconnect parameter was removed in centrifugal/centrifugo v5.0.0:
https://centrifugal.dev/docs/getting-started/migration_v5