Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Modify cookie[]= to take a hash of extra options #91

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

kylewlacy commented Feb 27, 2013

Based on pull request #66, with the suggested syntax changes. Doesn't work using cookie[:key] = 'value', :option => 'asdf' for whatever reason, so you need to wrap the extra hash in {}'s.

Also, on implementation: when []=(key, value) gets called with a hash, value becomes an array containing both the bare value, as well as the hash. Just one of quirky Ruby things I guess.

modify cookie[]= to take a hash of extra options
Using the syntax `cookie[:key] = 'value', {:options => 'hash'}` will now
merge the provided hash with the existing `@options` hash (which allows
for chainging cookie options on a per-cookie basis).
Contributor

kylewlacy commented Feb 27, 2013

On an added note, does anyone have any ideas on syntax for getting the options back from a given cookie?

Owner

zzak commented Mar 26, 2013

@kylewlacy I imagine you still need to patch Cookie#[] to be able to access the extra options

Owner

rkh commented Aug 19, 2013

@kylewlacy cookie options will largely be lost, as the browser won't let you know. I don't like the api, why would you use this over response.set_cookie?

Contributor

TrevorBramble commented Dec 10, 2013

@kylewlacy Still interested in your suggested change? I'll close otherwise.

rykann commented Dec 20, 2013

I would also like the ability to set cookie options, but have a different implementation in mind. If you want to specify options, put the cookie value in a hash along with the options, like this:

cookie[:foo] = {:value => "bar", :domain => "example.com", ...}

The implementation could look like this:

def []=(key, value)
  value = {:value => value} unless value === Hash
  @response.set_cookie key.to_s, @options.merge(value)
end

I think this is cleaner, and would be consistent with the way Rack::Response#set_cookie accepts a hash with cookie options.

Owner

zzak commented Jan 13, 2014

@rykann Could you submit a pull request with patch and specs?

Thanks!

rykann commented Jan 13, 2014

When I suggested this, I didn't realize there was more history on the topic. It looks like @kylewlacy implemented the same thing in kylewlacy/sinatra-contrib@9c7240d, referenced in the original conversation in #66. @rkh commented that he found it counterintuitive, and the implementation moved in a different direction.

I still prefer the way I suggested, as it should be familiar to anyone accustomed to setting cookies with options using response.set_cookie or Rails. If the team is on board with this, I'd be happy to submit a PR.

Owner

zzak commented May 22, 2015

I'm with you, we probably shouldn't implement any more api that recommends using cookie outside of response.set_cookie.

Closing this until we can decide on a better API. Thanks!

@zzak zzak closed this May 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment