Rack master integration (We depend after this commit on Rack ~> 1.5.0) #8891

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
5 participants
Owner

spastorino commented Jan 11, 2013

Review please ...

/cc @josevalim @NZKoz @jeremy @carlosantoniodasilva @raggi

The thing I'm not sure is about is request.session_options[:id] I've changed that to request.session.id to adhere to Rack API

@@ -3,17 +3,17 @@
module ActionDispatch
class Request < Rack::Request
# Session is responsible for lazily loading the session from store.
- class Session # :nodoc:
+ class Session < Rack::Session::Abstract::SessionHash # :nodoc:
@jeremy

jeremy Jan 11, 2013

Owner

Changing the ancestry of a class burned us with FlashHash. What a pain.

Deleting all that implementation below looks great though.

@spastorino

spastorino Jan 11, 2013

Owner

Don't remember exactly what was the issue with FlashHash but I've checked method by method and inheriting from SessionHash just adds some more useful methods.
Anyway as everything could be a cause of issues.
Given that is a major version change I'd do it since the risk is low.

@jeremy

jeremy Jan 11, 2013

Owner

Is it possible to do in a separate commit? Then we can revert it, if needed, without reverting the entire Rack 1.5 update.

@spastorino

spastorino Jan 11, 2013

Owner

@jeremy I'm going tomorrow for vacations won't be possible until after that :( sorry. @carlosantoniodasilva was checking to see if he can split commits. Do whatever you guys want with this branch, take it over :). Completely destroy it whatever :P.
I wanted to leave what I promised to do implemented to allow releasing Rails.

session.merge! session_was if session_was
set(env, session)
- Options.set(env, Request::Session::Options.new(store, env, default_options))
+ set_options(env, default_options.dup)
-
- def loaded_session?(session)
- !session.is_a?(Request::Session) || session.loaded?
- end
@jeremy

jeremy Jan 11, 2013

Owner

Why remove this API?

@spastorino

spastorino Jan 11, 2013

Owner

Because you get the right version through inheritance.
See https://github.com/rack/rack/blob/master/lib/rack/session/abstract/id.rb#L284-286

Also !session.is_a?(Request::Session) is wrong because we also have NullSessionHash which inherits from Rack's SessionHash and it's not a Request::Session see

class NullSessionHash < Rack::Session::Abstract::SessionHash #:nodoc:

I'm kind of fixing and reusing code at the same time.

@@ -237,17 +237,26 @@ def recycle!
end
class TestSession < Rack::Session::Abstract::SessionHash #:nodoc:
- DEFAULT_OPTIONS = Rack::Session::Abstract::ID::DEFAULT_OPTIONS
@jeremy

jeremy Jan 11, 2013

Owner

Breaks people who TestSession::DEFAULT_OPTIONS.update ...

@spastorino

spastorino Jan 11, 2013

Owner

You're right fixed a71448d

request.env['action_dispatch.request.flash_hash'] = nil
request.env['rack.session.options'] = { skip: true }
request.env['action_dispatch.cookies'] = NullCookieJar.build(request)
end
class NullSessionHash < Rack::Session::Abstract::SessionHash #:nodoc:
- def initialize
- super(nil, nil)
@jeremy

jeremy Jan 11, 2013

Owner
def initialize(env)
  super nil, env
end
@spastorino

spastorino Jan 11, 2013

Owner

Yes I was reusing Rack initializer but that way I'm passing NullSessionHash creators the responsibility to define a null store which should be in NullSessionHash. So agree.

Fixed e3b9b25

Owner

jeremy commented Jan 11, 2013

Does session lazy loading still work?

Owner

spastorino commented Jan 11, 2013

@jeremy thanks for reviewing it ❤️ ❤️ ❤️

@@ -162,7 +162,7 @@ def initialize(env = {})
super
self.session = TestSession.new
- self.session_options = TestSession::DEFAULT_OPTIONS.merge(:id => SecureRandom.hex(16))
+ self.session_options = Rack::Session::Abstract::ID::DEFAULT_OPTIONS
@carlosantoniodasilva

carlosantoniodasilva Jan 11, 2013

Owner

@spastorino shouldn't this be changed back to use the TestSession::DEFAULT_OPTIONS as well?

Owner

spastorino commented Jan 11, 2013

/cc @tenderlove too :)

+
+ private
+
+ def load!
@robin850

robin850 Jan 12, 2013

Member

It's a detail but should you not indent after private? :)

+
+ private
+
+ def load!
@robin850

robin850 Jan 12, 2013

Member

Same here, no ?

@agis

agis Jan 12, 2013

Contributor

@robin850 Not always, it's a matter of personal preference. I myself for ex. prefer not to indent :)

@robin850

robin850 Jan 12, 2013

Member

Yes but, guides tell us to indent after private or protected

@agis

agis Jan 12, 2013

Contributor

@robin850 Oh missed that, thanks! 😊

Owner

spastorino commented Jan 27, 2013

@carlosantoniodasilva the link doesn't work

@spastorino sorry, since it was merged, it's gone from my fork now.

@spastorino I've pushed the changes again rebased from current master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment