8000 Feature detect `CGI::Cookie`. by ioquatix · Pull Request #2333 · rack/rack · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Feature detect CGI::Cookie. #2333

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.

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

Merged
merged 2 commits into from
May 18, 2025
Merged

Feature detect CGI::Cookie. #2333

merged 2 commits into from
May 18, 2025

Conversation

ioquatix
Copy link
Member
@ioquatix ioquatix commented May 17, 2025

I would prefer to feature detect CGI::Cookie rather than version check.

If we approve this, I want to backport to 2.2 and 3.0.

cc @Earlopain

This is a follow up to #2327.

@ioquatix ioquatix requested a review from jeremyevans May 17, 2025 07:20
Copy link
Contributor
@Earlopain Earlopain left a comment

Choose a reason for hiding this comment

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

I guess this is fine. But, users could end up with cgi in their gemfile one way or another in which case the version check is more predictable.

@@ -128,7 +129,7 @@ def parse_cookies_from_header
def identify_cookie_attributes(cookie_filling)
cookie_bits = cookie_filling.split(';')
cookie_attributes = Hash.new
cookie_attributes.store('value', Array(cookie_bits[0].strip))
cookie_attributes.store('value', cookie_bits[0].strip)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed. rack doesn't test against ruby-dev or previews so it doesn't show but here are the failures:

Finished in 3.234368s, 340.0974 runs/s, 1473.8583 assertions/s.

  1) Failure:
Rack::MockResponse#test_0007_parses cookies giving max-age precedence over expires [test/spec_mock_response.rb:120]:
Expected: "expires_and_max-age_test"
  Actual: "e"

  2) Failure:
Rack::MockResponse#test_0004_provides access to session cookies [test/spec_mock_response.rb:87]:
Expected: "session_test"
  Actual: "s"

  3) Failure:
Rack::MockResponse#test_0006_provides access to persistent cookies set with expires [test/spec_mock_response.rb:109]:
Expected: "persistent_with_expires_test"
  Actual: "p"

  4) Failure:
Rack::MockResponse#test_0009_parses cookie headers with equals sign at the end [test/spec_mock_response.rb:138]:
--- expected
+++ actual
@@ -1 +1 @@
-"_somebase64encodedstringwithequalsatthened="
+"_"


  5) Failure:
Rack::MockResponse#test_0005_provides access to persistent cookies set with max-age [test/spec_mock_response.rb:98]:
Expected: "persistent_test"
  Actual: "p"

  6) Failure:
Rack::MockResponse#test_0012_parses multiple set-cookie headers provided as hash with array value [test/spec_mock_response.rb:155]:
Expected: "awesome"
  Actual: "a"

  7) Failure:
Rack::MockResponse#test_0008_provides access to secure cookies [test/spec_mock_response.rb:128]:
Expected: "secure_test"
  Actual: "s"

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, did not mean to commit this change.

Copy link
Contributor
@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

This allows a Ruby 3.5 user to opt-in to the cgi/cookie support by installing the gem. It seems a little at-odds with #2332, in that this increases the cases where cgi/cookie is used, while #2332 eliminates the cgi/cookie usage completely. However, I guess that makes sense, as this allows full backwards compatibility if the cgi gem is manually installed, which is probably what we want in a stable branch.

@ioquatix ioquatix force-pushed the feature-detect-cgi-cookie branch from eb2fe0d to 42c1386 Compare May 18, 2025 00:25
@ioquatix
Copy link
Member Author
ioquatix commented May 18, 2025

@Earlopain What is the name you prefer to use on the changelog?

[@earlopain]: https://github.com/earlopain "Earlopain"

Or is it "earlopain" or "Earl O'Pain"?

@ioquatix ioquatix force-pushed the feature-detect-cgi-cookie branch from 31eecaf to 38b69c4 Compare May 18, 2025 01:36
@ioquatix ioquatix merged commit bd60f6e into 3-1-stable May 18, 2025
31 checks passed
@ioquatix ioquatix deleted the feature-detect-cgi-cookie branch May 18, 2025 01:41
ioquatix added a commit that referenced this pull request May 18, 2025
ioquatix added a commit that referenced this pull request May 18, 2025
@Earlopain
Copy link
Contributor

It's good like this, I don't care about capitalization, both is good. Thanks for handling the releases!

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
0