Skip to content

Remove yiisoft/cookies dependency + Fix NullSession::getCookieParameters() - #86

Merged
vjik merged 2 commits into
masterfrom
rm-cookie
Sep 10, 2026
Merged

Remove yiisoft/cookies dependency + Fix NullSession::getCookieParameters()#86
vjik merged 2 commits into
masterfrom
rm-cookie

Conversation

@vjik

@vjik vjik commented Sep 6, 2026

Copy link
Copy Markdown
Member
Q A
Is bugfix? ✔️
New feature?
Breaks BC?

@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (817d8aa) to head (80f144b).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##              master       #86   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity        73        77    +4     
===========================================
  Files              3         3           
  Lines            182       187    +5     
===========================================
+ Hits             182       187    +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The replacement serializer omits cookie-field validation, allowing malformed or attribute-injected headers.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Removes yiisoft/cookies by implementing session-cookie serialization internally and fixes NullSession cookie defaults.

Changes:

  • Builds Set-Cookie headers directly in session middleware.
  • Defines cookie parameter types and valid NullSession defaults.
  • Expands middleware tests and simplifies test helpers.
File summaries
File Description
src/SessionMiddleware.php Implements cookie header generation.
src/SessionInterface.php Defines the cookie parameter shape.
src/Session.php Applies the shared Psalm type.
src/NullSession.php Returns complete cookie defaults.
tests/SessionMiddlewareTest.php Tests cookie serialization behavior.
tests/SessionTest.php Simplifies session initialization.
tests/ConfigTest.php Simplifies default parameter loading.
composer.json Removes yiisoft/cookies.
CHANGELOG.md Records the enhancement and fix.
Review details
  • Files reviewed: 9/9 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/SessionMiddleware.php
Comment on lines +93 to +95
$sameSite = $cookieParameters['samesite'] ?? 'Lax';

$cookieParts = [$this->session->getName() . '=' . urlencode($sessionId)];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We assume a SessionInterface implementation returns valid cookie parameters, so validating them here is out of scope.

In the common case (built-in SessionInterface implementation) cookie parameters come from PHP's own session configuration (session_get_cookie_params(), session_name()), and the cookie value is urlencode().

@vjik
vjik requested a review from a team September 6, 2026 04:31
@vjik vjik added the status:code review The pull request needs review. label Sep 6, 2026
@vjik
vjik merged commit 4ccc68f into master Sep 10, 2026
33 checks passed
@vjik
vjik deleted the rm-cookie branch September 10, 2026 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:code review The pull request needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants