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

Make it rails 3.2.11 compatible. #4

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

raskhadafi
Copy link

Rails 3.2 uses to store the secret_key the config secret_token.
So I extended the initializer. Can you merge it?

Thanks in advance.

@pixeltrix
Copy link
Owner

Hmm, I don't think we should set both - we should try to do some feature detection. I think Rails 4 had a backwards compatible mode for secret_token - maybe you can look at that as a starting point?

@raskhadafi
Copy link
Author

Ok, I'll give it another shot.
I thought I'll push it cause of the working tests.
Maybe your test should also receive an update.
Cause my rails app where I integrated the gem crashed after the first request had to be made.

@pixeltrix
Copy link
Owner

@raskhadafi yes, please do update the tests - if you want to you could adapt them to use the appraisal gem to make sure it works across both 3.2 and 4.0.

@raskhadafi
Copy link
Author

I did a little research and in the rails guides under the point 4.7 Action Pack they recommend to set both.

# config/initializers/secret_token.rb
Myapp::Application.config.secret_token = 'existing secret token'
Myapp::Application.config.secret_key_base = 'new secret key base'

So my solution should work. What do you think @pixeltrix ?

@cmar
Copy link

cmar commented Jul 23, 2014

An alternate solution would be to change the railtie to use before_initialize, then we could modify our 3.2 secret_token.rb to

RailsFrontend::Application.config.secret_token = RailsFrontend::Application.config.secret_key_base

@pixeltrix I could do a pull request for the change if your interested.

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.

3 participants