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

Rails 4.1 and default cookie store session store broken #85

Closed
zackchandler opened this issue Apr 10, 2014 · 9 comments · Fixed by #92
Closed

Rails 4.1 and default cookie store session store broken #85

zackchandler opened this issue Apr 10, 2014 · 9 comments · Fixed by #92

Comments

@zackchandler
Copy link

I posted a detailed writeup of this issue here:
http://stackoverflow.com/questions/22978704/object-stored-in-rails-session-becomes-a-string

The greater issue is that it is widely agreed that storing objects in the Rails session is a BAD idea. Yehuda Katz gives makes the case here: http://stackoverflow.com/a/1096221/1284634

Looking through the code base it would be very easy to modify the codebase to pass just the shop and token and create the session internally in the gem lib:

https://github.com/Shopify/shopify_app/blob/master/lib/generators/shopify_app/templates/app/controllers/sessions_controller.rb#L12-L13

I'm willing to put in a pull request if this is a desired change...

@zackchandler
Copy link
Author

Bump - anyone interested in this issue?

@maxrice
Copy link

maxrice commented Apr 19, 2014

We're about to upgrade to Rails 4.1 for a new app we're building so it'd be great to have a PR to fix this :)

@zackchandler
Copy link
Author

@maxrice I ended up moving away from shopify_app gem altogether. However you can also throw this file in initializers to make things work: https://gist.github.com/zackchandler/11104388 and then in sessions_controller.rb change to:

  def show
    if response = request.env['omniauth.auth']
      session[:shopify_shop] = params[:shop]
      session[:shopify_token] = response['credentials']['token']

      flash[:notice] = "Logged in"
      redirect_to return_address
    else
      flash[:error] = "Could not log in to Shopify store."
      redirect_to :action => 'new'
    end
  end

Hope this helps in the near term.

choonkeat added a commit to choonkeat/shopify_app that referenced this issue May 2, 2014
@choonkeat
Copy link

#90 is a pull request for this. existing apps, note the file changes and apply accordingly to your own app/controllers/sessions_controller.rb

@quietinvestor
Copy link

@choonkeat after applying your changes as per #85, the error in my test app just changes to:

undefined method `site' for false:FalseClass

Any ideas?

@choonkeat
Copy link

@IgnacioRodrigo could come from shop_session method returning false when session[:shopify] is a blank string, e.g. "" && YAML.load(""). will need your test code & stacktrace to assess?

@quietinvestor
Copy link

@choonkeat Uhh... ohh... I've woken up this morning, turned it on and it works error-free with your changes all of a sudden? :/ Apologies for my lack of professionalism but I'm relatively new to RoR and shopify_app development so I didn't write a test per se. I was just self-teaching myself the basics of setting up a very basic shopify_app to be deployed in Heroku through my Shopify Partners account. So to reproduce my app you literally just have to create a new rails project with PostgreSQL, do rails generate shopify_app with your API_key and secret and run rails server. The errors appear just after you accept giving read/write access or whatever through the shop you install it in. Only that now I'm getting no errors with your fix unlike yesterday? Wierd. Thanks a lot for your fix anyway, great job!

@danielristic
Copy link

Now that rails 4.1 is the default, this issue is more likely to happen to anybody creating a new app. Maybe it's the time for a definitive fix?

@csaunders
Copy link
Contributor

Take a look at an approach I took in #92

It wanted to make it completely independent from Rails. Unless something in the Ruby VM changes I believe using a class variable in the in memory store should behave consistently.

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 a pull request may close this issue.

6 participants