Skip to content

Merge Guzzle 8.3.1 cookie fixes - #3

Merged
binaryfire merged 6 commits into
8.3from
upstream-sync-guzzle-8.3.1
Oct 28, 2026
Merged

binaryfire merged 6 commits into
8.3from
upstream-sync-guzzle-8.3.1

Conversation

@binaryfire

@binaryfire binaryfire commented Oct 28, 2026 •

Copy link
Copy Markdown
Member

This merges upstream Guzzle 8.3.1. It fixes cookie updates that could leave an old cookie active or renew an expired cookie when loading a saved jar.

  • Expired cookies now remove the matching stored cookie instead of being stored themselves. This also drops expired records when loading saved jars. (guzzle/guzzle#3987)
  • A cookie with the same value now replaces an existing cookie when its expiry, Discard, Secure or HttpOnly flag changes. Exact duplicates remain unchanged. (guzzle/guzzle#3990)
  • Cookies reconstructed from arrays keep their stored expiry rather than receiving a fresh Max-Age lifetime. Parsing a Set-Cookie header still gives Max-Age precedence over Expires. The upgrade guide reflects that distinction. (guzzle/guzzle#3991)
  • The delayed-request test measures attachment separately from response completion, preventing a pending sibling request from making the timing assertion flaky. (guzzle/guzzle#3994)

The merge preserves the fork's coroutine ownership checks, pooled handlers, global middleware and idle-handle settings. No fork-specific source adjustments were needed.

Validation: affected cookie and cURL multi-handler tests, session-cookie loading tests, PHPStan, formatting, Composer normalization and platform checks pass. Existing platform-dependent cURL test skips remain.


Summary by cubic

Merges upstream Guzzle 8.3.1 cookie fixes. Cookie storage now removes expired cookies instead of storing them, and persisted cookies keep their original expiry instead of restarting their lifetime when loaded.

Bug Fixes

  • Expired cookies delete the matching stored cookie, including expired records dropped when loading saved jars.
  • Same-value cookies replace the existing entry when expiry or the Discard, Secure, or HttpOnly flags change; exact duplicates stay unchanged.
  • Rebuilt Max-Age cookies keep the stored expiry, while Set-Cookie header parsing still gives Max-Age precedence over Expires.
  • The delayed-request test times attachment separately from response completion so a pending sibling request can't make the assertion flaky.

The merge keeps the fork's coroutine ownership checks, pooled handlers, global middleware, and idle-handle settings. No fork-specific source changes were needed.

Written for commit de7f066. Summary will update on new commits.

View guided diff Turn on auto-fix

Summary by CodeRabbit

  • Bug Fixes

    • Expired cookies are no longer stored or sent, and can remove a matching cookie already in the jar.
    • Updates to a cookie’s value or attributes are now consistently reflected in the jar.
    • Cookies created from arrays retain their supplied expiration time when loaded, preventing their lifetime from being incorrectly restarted.
    • For cookies parsed from a header, Max-Age takes precedence over Expires.
  • Documentation

    • Clarified how Max-Age and Expires behave for parsed and array-created cookies.

GrahamCampbell and others added 6 commits October 25, 2026 01:30
Merge release 7d9b1f4, including guzzle/guzzle PRs guzzle#3987, guzzle#3990, guzzle#3991 and guzzle#3994.

Expired cookies now remove matching stored cookies. Same-value cookies replace old entries when their expiry or flags change. Restoring Max-Age cookies from persisted data preserves their original expiry, while Set-Cookie headers retain Max-Age precedence.

The delayed-request regression measures attachment separately from response completion so a pending sibling cannot make its timing assertion flaky. Preserve the fork-specific coroutine ownership, pooled handlers, global middleware and idle-handle settings without conflict resolutions or additional source changes.

Validated each affected cookie test file, session-cookie loading and the complete CurlMultiHandler test file. PHPStan, PHP-CS-Fixer, Composer normalization and platform checks pass.
@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Walkthrough

Cookie expiration and replacement handling changed for SetCookie and CookieJar. Tests and documentation cover the updated behavior. A CurlMultiHandler test now checks delayed-request attachment before checking the response.

Changes: Cookie Expiration and Replacement

Layer / File(s) Summary
SetCookie Max-Age and Expires handling
src/Cookie/SetCookie.php, tests/Cookie/SetCookieTest.php, tests/Cookie/FileCookieJarTest.php, UPGRADING.md
Parsed cookies use Max-Age to determine expiration. Cookies constructed from arrays retain a supplied Expires value. Tests cover parsing, construction, and file loading.
CookieJar expiration and replacement
src/Cookie/CookieJar.php, tests/Cookie/CookieJarTest.php, CHANGELOG.md
CookieJar rejects expired cookies and replaces matching cookies when their value, expiration, discard, secure, or HTTP-only setting differs. Tests cover removal and replacement. The changelog records these fixes.

CurlMultiHandler Test Update

Layer / File(s) Summary
Delayed request attachment check
tests/Handler/CurlMultiHandlerTest.php
The test waits for the delayed request to leave the handler’s delay map before checking the response.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: grahamcampbell

Merge Risk: Minimal · up to de7f0

The change is mergeable after normal checks; the proposed changelog reformatting is unnecessary, and a delayed-request test failure has not been established.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title is concise and clearly describes the main change: merging Guzzle 8.3.1 cookie fixes. It is directly related to the pull request objectives.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 6 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@binaryfire

Copy link
Copy Markdown
Member Author

@coderabbitai review

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Merge Guzzle 8.3.1 cookie fixes and timing-test correction

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

AI Description

• Remove expired cookies and replace matching cookies when their expiry or flags change.
• Preserve stored expiry when loading cookies while retaining Max-Age precedence for headers.
• Isolate delayed-request attachment timing from response completion to prevent a flaky test.
Diagram

graph TD
  H["Set-Cookie header"] --> S["SetCookie"] --> J["CookieJar"] --> E{"Expired?"}
  P["Saved jar data"] --> S
  E -->|Yes| R["Remove match"]
  E -->|No| C{"Attributes changed?"} --> U["Replace or retain"]
Loading
High-Level Assessment

Keep the distinction between header parsing and array reconstruction within SetCookie, then let CookieJar handle expiry and replacement. A separate persistence-specific expiry override would add another path for the same cookie semantics without a clear benefit.

Files changed (8) +161 / -41

Bug fix (2) +15 / -19
CookieJar.phpRemove expired matches and replace changed cookies +8/-17

Remove expired matches and replace changed cookies

• Treats any expired incoming cookie as a removal request. Replaces a matching cookie when its value, expiry, Discard, Secure, or HttpOnly attribute differs; exact duplicates remain unchanged.

src/Cookie/CookieJar.php

SetCookie.phpPreserve stored expiry without changing header precedence +7/-2

Preserve stored expiry without changing header precedence

• Derives Expires from Max-Age only when an array supplies no expiry. Header parsing clears a supplied Expires when a valid Max-Age is present, preserving header precedence.

src/Cookie/SetCookie.php

Tests (4) +131 / -18
CookieJarTest.phpCover cookie replacement and expiry removal +83/-2

Cover cookie replacement and expiry removal

• Adds cases for changed expiry and flags, expired response cookies, post-construction Max-Age deletion, and expired constructor records. Moves existing test expiry dates into the future so those tests continue to exercise stored cookies.

tests/Cookie/CookieJarTest.php

FileCookieJarTest.phpCheck that loading does not renew expired cookies +21/-0

Check that loading does not renew expired cookies

• Adds a persisted-cookie regression case with Max-Age and an expired stored Expires value, asserting that loading it produces no Cookie header.

tests/Cookie/FileCookieJarTest.php

SetCookieTest.phpTest distinct array and header expiry behavior +22/-13

Test distinct array and header expiry behavior

• Verifies that array construction preserves an explicit expiry while header parsing gives Max-Age precedence. Updates the existing attribute test to expect its supplied expiry unchanged.

tests/Cookie/SetCookieTest.php

CurlMultiHandlerTest.phpMeasure delayed-request attachment independently +5/-3

Measure delayed-request attachment independently

• Stops the timing loop when the delayed request attaches, then waits for its response separately. This prevents a sibling request's select timeout from inflating the attachment-time assertion.

tests/Handler/CurlMultiHandlerTest.php

Documentation (2) +15 / -4
CHANGELOG.mdRecord the 8.3.1 cookie fixes +9/-0

Record the 8.3.1 cookie fixes

• Adds a release entry covering expired-cookie removal, same-value cookie replacement, and preservation of persisted Max-Age cookie expiry.

CHANGELOG.md

UPGRADING.mdClarify Max-Age precedence by cookie source +6/-4

Clarify Max-Age precedence by cookie source

• Specifies that Max-Age takes precedence over Expires in parsed Set-Cookie headers, while cookies reconstructed from arrays retain a supplied Expires value.

UPGRADING.md

@binaryfire

Copy link
Copy Markdown
Member Author

@cubic-dev-ai review

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 28, 2026

Copy link
Copy Markdown

@cubic-dev-ai review

@binaryfire cubic can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 123,074 of the 120,000 allowed lines of code this month. Reviews resume on 10 November 2026 (in 14 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews.

To help optimise your usage, you can tune cubic to get the most out of your usage limits:

Learn more →

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@coderabbitai coderabbitai Bot 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.

🔇 Additional comments: 1
tests/Handler/CurlMultiHandlerTest.php-331-331: 1

331-331: Major · Functional Correctness · Quick win

⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Measure attachment before post-attachment select processing.

The loop checks delays only after tick() returns. tickFor() removes the
due entry before its later curl_multi_select() call. With the 5-second
sibling timeout, the same tick() can empty delays and then wait for the
sibling. $elapsed therefore includes time after attachment, so the
< 2.5 assertion can fail even when attachment was prompt.

Observe attachment before select processing, or remove this timing assertion
from the manual-tick test.


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 39701e10-1554-4eda-b7e6-a177d1279047
Commits

Reviewing files that changed from the base of the PR and between fd10dd5 and de7f066.

Files selected for processing: 8
  • CHANGELOG.md
  • UPGRADING.md
  • src/Cookie/CookieJar.php
  • src/Cookie/SetCookie.php
  • tests/Cookie/CookieJarTest.php
  • tests/Cookie/FileCookieJarTest.php
  • tests/Cookie/SetCookieTest.php
  • tests/Handler/CurlMultiHandlerTest.php

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@binaryfire

Copy link
Copy Markdown
Member Author

The attachment-timing concern in the review does not apply to this test's fresh-connection setup. The handler starts without shared or idle connections, and the only existing HTTP/1.1 connection is occupied by the stalled sibling. The delayed request must open a new TCP connection; the attachment tick's select observes that connection becoming ready.

The long sibling-only select occurs on a later tick that processes an already-buffered response. The revised loop stops before timing that tick, then waits for the response separately. This preserves the regression assertion without adding test instrumentation or changing production code. The affected suite and the PHP-version CI jobs pass.

The imported changelog retains upstream formatting. No documentation changes are needed solely to meet a docstring-coverage percentage.

@binaryfire
binaryfire merged commit b051f8b into 8.3 Oct 28, 2026
13 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.

2 participants