SFEley / sinatra-flash

TODO: one-line summary of your gem
Other
154 stars 24 forks source link

Enabling session #1

Closed rkh closed 14 years ago

rkh commented 14 years ago

Enabling session in an after hook will use it starting from the next request, but not for the current.

SFEley commented 14 years ago

D'oh! Good catch. I'll write a test for that and figure something out. My intention was just to delay the test for it as long as possible, to give the application a chance to set up its own session preferences.

rkh commented 14 years ago

Yes. Only problem here is, Rack::Session gets set up in Sinatra::Base.new.

rkh commented 14 years ago

I think you could do something like this:

unless session?
  settings.enable :session
  Rack::Session::Cookie.new(proc { |e| e["rack.session"].merge! session }).call({})
end

Not tested. You'd have to pass some headers (set-cookie) to the current headers.

SFEley commented 14 years ago

Yeah, I see your point. So setting the option does nothing after that. Looking at it closer, I see a bigger problem: just testing on session will never prove the existence of a real session because it defaults to {} if there isn't an env['rack.session'].

I could test on that, of course, and set the same Rack::Session::Cookie that Sinatra defaults to if it isn't there. But then it's starting to get complicated. The other option would be just to back it out and say "Create a session before you use this" like Rack::Flash does.

I'm not sure at the moment which one I like better. Do you have an opinion?

(Update: This is a reply to your next-to-last comment, not the comment just before this, which I hadn't read yet. I don't think that code will work because session will never return nil.)

rkh commented 14 years ago

Oh, but you could use settings.session? instead.

There is an even bigger problem: If you just did a settings.enable :session in your after filter, sessions might stay disabled all the time, as new is not necessarily called again (sinatra uses prototype.dup instead).

Maybe you should just do a enable :session in your register and mention that, in case someone wants to use another session store, they'll have to disable :session again.

SFEley commented 14 years ago

Checking settings.session? isn't reliable, because it's possible to have a session without setting that option. (Sinatra's documentation advises it when you want to pass options or use a different session store.)

The more I think about it, between all of these issues and Sinatra's attempts to hide the session details (you can't even test env['rack.session'] after calling session once because it sets it to {}) it just doesn't make sense to try to guess.

I'm just going to pull out the "Create a session for you" code and tell the user it's up to them. Anyone using Sinatra instead of Rails is probably clueful enough to know how to do this anyway. >8->

Thank you very much for raising the issue. If you hadn't, it would likely have been a long time before I realized this wasn't working. I appreciate your time and help.

rkh commented 14 years ago

No problem, your extension seems promising. Thinking about replacing Rack::Flash with it for BigBand.

SFEley commented 14 years ago

Cool! Let me know how it goes. >8->