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

Enabling session #1

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

Enabling session #1

rkh opened this issue May 2, 2010 · 8 comments

Comments

@rkh
Copy link

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
Copy link
Owner

SFEley commented May 2, 2010

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
Copy link
Author

rkh commented May 2, 2010

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

@rkh
Copy link
Author

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
Copy link
Owner

SFEley commented May 2, 2010

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
Copy link
Author

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
Copy link
Owner

SFEley commented May 2, 2010

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
Copy link
Author

rkh commented May 2, 2010

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

@SFEley
Copy link
Owner

SFEley commented May 2, 2010

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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants