Skip to content
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

allows symbol access to cookies #156

Merged
merged 1 commit into from
May 2, 2017

Conversation

Anorlondo448
Copy link
Contributor

@Anorlondo448 Anorlondo448 commented Jan 29, 2017

I think it's better if cookies method allows symbol access.

Symbol is available for cookies method in Ruby on Rails, but not available for testing.
Because rack-test is used in Ruby on Rails, but not allow symbol access to cookies.
This problem is also written in RUBY ON RAILS TUTORIAL (RAILS 5).
(@chapter 9.3)

There’s one more subtlety, which is that for some reason inside tests the cookies method doesn’t > work with symbols as keys, so that

cookies[:remember_token]
is always nil. Luckily, cookies does work with string keys, so that

cookies['remember_token']
has the value we need.

Ruby on Rails cookies method
https://github.com/rails/rails/blob/15a972e080d7712aa2eb04f21472398457d1439c/actionpack/lib/action_dispatch/middleware/cookies.rb#L315%23L317

# Returns the value of the cookie by +name+, or +nil+ if no such cookie exists.
def [](name)
  @cookies[name.to_s]
end

I hope that this PR will be merged.
Regards.

@Anorlondo448 Anorlondo448 changed the title allow symbol access to cookies allows symbol access to cookies Jan 31, 2017
jar = Rack::Test::CookieJar.new
jar["value"] = "foo;abc"
jar[:value].should == "foo;abc"
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move this closer to the other tests for Rack::Test::CookieJar, around line 46 would make sense. Could you fix that @Anorlondo448 and also rebase on latest master so we can see that Travis is still happy with this change in place?

Choose a reason for hiding this comment

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

@Anorlondo448 are you still available for the change that has been requested.
Otherwise I would just "re-implement" the change on the current master and open a PR that places the test at the right spot. What do you think @perlun ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@scepticulous let's give it a few days first. If he doesn't seem to be around I'm fine with you closing this PR and rebasing the change on master with the test at the right spot.

@Anorlondo448 Anorlondo448 force-pushed the allow_symbol_access_to_cookies branch 2 times, most recently from b8e2a64 to 1b7145b Compare May 1, 2017 14:29
@Anorlondo448 Anorlondo448 force-pushed the allow_symbol_access_to_cookies branch from 1b7145b to 2e750d9 Compare May 1, 2017 14:33
@Anorlondo448
Copy link
Contributor Author

Anorlondo448 commented May 1, 2017

@perlun , @scepticulous
I am sorry to keep you waiting.
I modified PR.
Thanks to your review!

@perlun perlun merged commit a396bd1 into rack:master May 2, 2017
@perlun
Copy link
Contributor

perlun commented May 2, 2017

Thanks for your contribution @Anorlondo448!

@Anorlondo448 Anorlondo448 deleted the allow_symbol_access_to_cookies branch May 2, 2017 12:40
alex-damian-negru pushed a commit to alex-damian-negru/rack-test that referenced this pull request Apr 5, 2021
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