-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
There was a problem hiding this 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.
lib/rack/mock_response.rb
Outdated
@@ -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) |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
eb2fe0d
to
42c1386
Compare
@Earlopain What is the name you prefer to use on the changelog?
Or is it "earlopain" or "Earl O'Pain"? |
31eecaf
to
38b69c4
Compare
It's good like this, I don't care about capitalization, both is good. Thanks for handling the releases! |
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.