Skip to content

Conversation

@abnegate
Copy link
Member

@abnegate abnegate commented Aug 26, 2025

Summary by CodeRabbit

  • Chores

    • Updated runtime requirement to PHP ≥ 8.1 and Swoole 6.x; refreshed Swoole IDE helper to 6.x.
  • Bug Fixes

    • Ensures cookie handling behaves correctly on Swoole 6.
  • Refactor

    • Minor internal adjustments to align with Swoole 6 behavior without changing public APIs.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Aug 26, 2025

Walkthrough

composer.json dependency constraints were tightened: php changed from >=8.0 to >=8.1, ext-swoole changed from * to 6.*, and swoole/ide-helper was bumped from 5.0.2 to 6.0.2. In src/Swoole/Response.php, the internal call to Swoole's cookie() was changed to pass $name positionally instead of using the named name: parameter; no public API signatures were changed.

Changes

Cohort / File(s) Summary
PHP requirement
composer.json
Tightened PHP requirement from >=8.0 to >=8.1.
Dependency constraints (Swoole 6)
composer.json
Narrowed ext-swoole from * to 6.*; bumped swoole/ide-helper from 5.0.2 to 6.0.2.
Swoole response cookie call
src/Swoole/Response.php
Switched the internal call to Swoole cookie() to pass the cookie name as a positional argument instead of the named name: parameter; method signature unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App as App Code
  participant Resp as Response (this lib)
  participant Svr as Swoole\Http\Response (v6)

  App->>Resp: sendCookie(name, value, options)
  note right of Resp: Map options to Swoole cookie call
  Resp->>Svr: cookie(name, value, expires, path, domain, secure, httponly, samesite)
  Svr-->>Resp: Cookie set
  Resp-->>App: Done
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

Tighten versions, nudge to eight and six,
Swap a named param for a simple fix.
Cookies still set with care and grace—
No signatures moved from their place.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix parameter name for v6' directly reflects the main change: updating a Swoole function parameter from a named parameter to a positional argument in the Response class for v6 compatibility.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/Swoole/Response.php (1)

188-191: Fix PHP 8.4 deprecation: implicitly nullable parameter in end()

The signature declares string $content with a default null, which is deprecated in PHP 8.4 and is breaking CodeQL/PHPStan in CI. Switch to a nullable string.

Apply this diff:

-    protected function end(string $content = null): void
+    protected function end(?string $content = null): void
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1af73dd and b682c3e.

⛔ Files ignored due to path filters (1)
  • composer.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • composer.json (1 hunks)
  • src/Swoole/Response.php (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CodeQL
src/Swoole/Response.php

[error] 188-188: PHPStan: Deprecated in PHP 8.4: Parameter #1 $content (string) is implicitly nullable via default value null. Command './vendor/bin/phpstan analyse --level 6 src tests --memory-limit 512M' returned with exit code 1.


[error] 247-247: PHPStan: Missing parameter $name (mixed) in call to method Swoole\Http\Response::cookie(). Command './vendor/bin/phpstan analyse --level 6 src tests --memory-limit 512M' returned with exit code 1.


[error] 248-248: PHPStan: Unknown parameter $name_or_object in call to method Swoole\Http\Response::cookie(). Command './vendor/bin/phpstan analyse --level 6 src tests --memory-limit 512M' returned with exit code 1.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Unit & E2E
🔇 Additional comments (2)
src/Swoole/Response.php (1)

247-256: Tighten Swoole cookie call for PHPStan compatibility

PHPStan’s current stubs (ide-helper 6.x) still declare the first parameter as $name, so using a named argument name_or_object: breaks static analysis. In addition, the stub signature requires expires as an int and samesite as a string (passing false will be rejected). Casting each $options[...] value ensures you never pass the wrong type.

Apply this diff in src/Swoole/Response.php around line 247:

-        $this->swoole->cookie(
-            name_or_object: $name,
-            value: $value,
-            expires: $options['expire'] ?? 0,
-            path: $options['path'] ?? '',
-            domain: $options['domain'] ?? '',
-            secure: $options['secure'] ?? false,
-            httponly: $options['httponly'] ?? false,
-            samesite: $options['samesite'] ?? false,
-        );
+        $this->swoole->cookie(
+            $name,
+            value: $value,
+            expires: (int) ($options['expire'] ?? 0),
+            path: (string) ($options['path'] ?? ''),
+            domain: (string) ($options['domain'] ?? ''),
+            secure: (bool) ($options['secure'] ?? false),
+            httponly: (bool) ($options['httponly'] ?? false),
+            samesite: (string) ($options['samesite'] ?? ''),
+        );

Key points:

  • Switching the first argument to positional $name unblocks PHPStan without relying on the stub’s internal parameter name.
  • Casting expires to int and samesite to string matches the stub signature and prevents passing false to a parameter that expects a string.

After merging, please confirm locally that dependencies install (you may need --ignore-platform-req=ext-swoole --ignore-platform-req=ext-opentelemetry) and that

vendor/bin/phpstan analyse --level 6 src tests --memory-limit 512M

passes with zero errors.

composer.json (1)

17-17: LGTM on constraining to Swoole v6 + matching IDE helper

Pinning ext-swoole to 6.* and upgrading swoole/ide-helper to 6.0.2 are consistent with the code change in Response::sendCookie. No issues here.

Also applies to: 22-22

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/Swoole/Response.php (1)

247-256: Incorrect parameter name for Swoole 6.x Response::cookie() method.

The parameter should be name, not name_or_object. According to the official Swoole 6.0 documentation, the Response::cookie() method signature uses name as the parameter:

$response->cookie(
    name: 'sid',
    value: 'abc',
    expires: time() + 3600,
    path: '/',
    domain: 'example.com',
    secure: true,
    httponly: true,
);

Change line 248 back to name: $name, to match the actual Swoole 6.x API.

@abnegate abnegate merged commit c5ce710 into master Jan 28, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants