Enabling session #1

Closed
rkh opened this Issue May 2, 2010 · 8 comments

Projects

None yet

2 participants

@rkh
rkh commented May 2, 2010

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

@SFEley
Owner

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
rkh commented May 2, 2010

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

@rkh
rkh commented May 2, 2010

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
Owner

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
rkh commented May 2, 2010

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
Owner

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
rkh commented May 2, 2010

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

@SFEley
Owner

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

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