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.

Sign up for GitHub

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

Jump to bottom

Add option to set the session middleware when enabling sessions #1161

Merged
merged 4 commits into from
Aug 9, 2016
Merged

Add option to set the session middleware when enabling sessions #1161

merged 4 commits into from
Aug 9, 2016

Conversation

jkowens
Copy link
Member

@jkowens jkowens commented Aug 4, 2016

By setting the session middleware as an option, rack protection for
sessions will be enabled by default and other session settings will be
applied to the middleware. Fixes #1038.

Example:

 set :sessions, :session_store => Rack::Session::Pool, :expire_after => 2592000

@jkowens
Add option to set the session middleware when enabling sessions
1340606 
By setting the session middleware as an option, rack protection for
sessions will be enabled by default and other session settings will be
applied to the middleware.
zzak
zzak reviewed Aug 4, 2016
View reviewed changes
README.md

```ruby
use Rack::Session::Pool, :expire_after => 2592000
Copy link
Member

Choose a reason for hiding this comment

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

If the use example still works I'd like to show both (since both should be supported).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call 👍

@jkowens
Copy link
Member Author

jkowens commented Aug 4, 2016 •
edited
Loading

@zzak I think I've just thought of a better way to handle setting a custom session middleware. What do you think about this:

enable :sessions
# override the default which is Rack::Session::Cookie
set :session_store, Rack::Session::Pool

If someone needed to set an option on the session middleware they could do this:

# enable sessions and set option hash to be passed to session middleware
set :sessions, :expire_after => 2592000
set :session_store, Rack::Session::Pool

@zzak
Copy link
Member

zzak commented Aug 4, 2016

@jkowens I think you're on the right track, let me know what you come up with after some polish :)

@jkowens jkowens force-pushed the custom_session_store branch from b59a765 to 4ee4b81 Compare August 4, 2016 15:07
@jkowens jkowens force-pushed the custom_session_store branch from 4ee4b81 to db0f8d5 Compare August 4, 2016 15:12
@jkowens
Copy link
Member Author

jkowens commented Aug 4, 2016

@zzak ok I think I'm ready for you to give it another look. Hopefully the documentation is up to snuff. Thanks!

@jkowens
Copy link
Member Author

jkowens commented Aug 4, 2016

I will squash my commits once everything is good.

zzak
zzak reviewed Aug 4, 2016
View reviewed changes
README.md
set :session_store, Rack::Session::Pool
```

Or to enable sessions with a hash of options:
Copy link
Member

Choose a reason for hiding this comment

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

I think using "enable" here is slightly confusing because enable is a method too, mentioned above

@jkowens
Copy link
Member Author

jkowens commented Aug 8, 2016 •
edited
Loading

Just for reference: Issue #601 was closed by 5aa1c7c, but in issue #757, it turns out it didn't really address the problem. #1038 is really just the same issue being brought up again.

@jkowens jkowens force-pushed the custom_session_store branch from d5eaa96 to 593cb37 Compare August 8, 2016 15:19
@jkowens
Copy link
Member Author

jkowens commented Aug 8, 2016

@zzak after finding #757, I understand the purpose of set :protection, :session => true and updated the readme.

@zzak zzak merged commit 4797c02 into sinatra:master Aug 9, 2016
@zzak
Copy link
Member

zzak commented Aug 9, 2016

@jkowens LGTM thanks for your patience and contribution!

@jkowens
Copy link
Member Author

jkowens commented Aug 9, 2016

Cool, thanks for your help 😄

@davydovanton davydovanton mentioned this pull request Aug 11, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jkowens @zzak