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

Extract ActiveRecord::SessionStore from Rails #7436

Merged

Conversation

sikachu
Copy link
Member

@sikachu sikachu commented Aug 23, 2012

This functionality will be available from gem active_record-session_store instead. The gem is currently hosted at https://github.com/sikachu/activerecord-session_store, and I'll be pushing to rails/ soon.

@rafaelfranca
Copy link
Member

👍 with the changelog entry

@sikachu
Copy link
Member Author

sikachu commented Aug 23, 2012

Pull request updated with changelog.

@fxn
Copy link
Member

fxn commented Aug 23, 2012

Prem, the "Action Controller Overview" and "Configuring Rails Applications" guides need to be updated as well.

@sikachu
Copy link
Member Author

sikachu commented Aug 23, 2012

@fxn sure thing. going to do that when I get home.

@carlosantoniodasilva
Copy link
Member

Yay 👍

@@ -1,5 +1,8 @@
## Rails 4.0.0 (unreleased) ##

* ActiveRecord::SessionStore has been extracted from Active Record as `activerecord-session_store`
gem. Plese read the `README.md` file on the gem for the usage. *Prem Sichanugrist*
Copy link
Contributor

Choose a reason for hiding this comment

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

Plese -> Please :)

@josevalim
Copy link
Contributor

Loving this! <3

@spastorino
Copy link
Contributor

👍

@arunagw
Copy link
Member

arunagw commented Aug 24, 2012

❤️

@sikachu
Copy link
Member Author

sikachu commented Aug 24, 2012

Pull request update with the doc fix from @josevalim and guides are updated from @fxn's request. The gem has also been pushed to rails/activerecord-session_store

@@ -1,5 +1,8 @@
## Rails 4.0.0 (unreleased) ##

* Setting `config.session_store` to `:active_record_store` will now break unless you have
`activerecord-session_store` gem installed. *Prem Sichanugrist*

Copy link
Member

Choose a reason for hiding this comment

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

We can say "ActiveRecord::SessionStore is extracted out of Rails into a gem activerecord-session_store. Setting config.session_store to :active_record_store will no longer work and will break if the activerecord-session_store gem isn't available". More informative imo.

EDIT: Now I see, you've done this in the AR changelog. Could do it here too.

@homakov
Copy link
Contributor

homakov commented Aug 24, 2012

Reasonable!
Offtopic question: would "encrypted" cookie store be useful? I mean it will be impossible to read on client side w/o encrypted_key. Because currently some people store "sensitive" info in session, while it easily can be decoded from base64.

@sikachu
Copy link
Member Author

sikachu commented Aug 24, 2012

@homakov might be, maybe open up another ticket requesting for comment on that? Then we can take it from there.

This functionality will be available from gem
`active_record-session_store` instead.
@sikachu
Copy link
Member Author

sikachu commented Aug 24, 2012

Pull request updated with updated documentation feedback from @vijaydev

@sikachu
Copy link
Member Author

sikachu commented Aug 24, 2012

Meta session migration generator has been removed, @josevalim

@@ -125,8 +125,6 @@ def session_store(*args)
case @session_store
when :disabled
nil
when :active_record_store
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should raise an error here. Otherwise people will get constant ActionDispatch::Session::ActiveRecordStore not found error. Tell them it was moved to a plugin and that they should set config.session_store = ActiveRecord::SessionStore.

Tell people to install `activerecord-session_store` gem when it's not
installed instead ofraising `NameError` on missing
`ActionDispatch::Session::ActiveRecordStore`.
@sikachu
Copy link
Member Author

sikachu commented Aug 24, 2012

Ok, error message has been added. I guess this is ready to roll.

@guilleiguaran
Copy link
Member

:shipit:

josevalim added a commit that referenced this pull request Aug 24, 2012
…sion_store

Extract ActiveRecord::SessionStore from Rails
@josevalim josevalim merged commit 73c0222 into rails:master Aug 24, 2012
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

Successfully merging this pull request may close these issues.

None yet

10 participants