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

Ensure compatibility between ActionDispatch::Request::Session and Rack #24820

Merged
merged 1 commit into from May 4, 2016

Conversation

Projects
None yet
6 participants
@maclover7
Copy link
Member

maclover7 commented May 2, 2016

Adding the each method is required for ensuring compability between
Rails, and other Rack frameworks (like Sinatra, etc.), that are mounted
within Rails, and wish to use its session tooling. Prior to this, there
was an inconsistency between ActionDispatch::Request::Session and
Rack::Session::Cookie, due to the absence of the each method. This
should hopefully fix that error. :)

For a full integration test with Sinatra and a standalone Rack
application, you can check out the gist for that here: https://gist.github.com/maclover7/08cd95b0bfe259465314311941326470.

Solves #15843.

@maclover7 maclover7 added the actionpack label May 2, 2016

@maclover7

This comment has been minimized.

Copy link
Member

maclover7 commented May 2, 2016

cc @mperham since he had to monkeypatch this in Sidekiq

@mperham

This comment has been minimized.

Copy link
Contributor

mperham commented May 2, 2016

OMG please tell me this will get into Rails 5.

@schneems

This comment has been minimized.

Copy link
Member

schneems commented May 2, 2016

I'm in favor of the idea and the implementation, Travis is failing. I'm on mobile and can't easily see, is master failing on Travis now? If so we should revert the failing commit.

@maclover7 maclover7 force-pushed the maclover7:fix-15843 branch May 3, 2016

@maclover7

This comment has been minimized.

Copy link
Member

maclover7 commented May 3, 2016

Regarding Travis:

  • looks like the Action Pack suite is now failing based on my PR (rebased + pushed up a fix, I think)
  • looks like Railties and Active Support has been failing for a few days now on master (I think @vipulnsward is working on fixing this, from what I last remember..?)
@vipulnsward

This comment has been minimized.

Copy link
Member

vipulnsward commented May 3, 2016

This is bundler issue, we have to confirm manually.
Let me send in a change so that meanwhile we fall back to older version.

On Monday 2 May 2016, Jon Moss notifications@github.com wrote:

Regarding Travis:

  • looks like the Action Pack suite is now failing based on my PR
    (rebased + pushed up a fix, I think)
  • looks like Railties and Active Support has been failing for a few
    days now (I think @vipulnsward https://github.com/vipulnsward is
    working on fixing this..?)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#24820 (comment)

Vipul A.M.
+91-8149-204995

@maclover7 maclover7 force-pushed the maclover7:fix-15843 branch May 3, 2016

@mperham

This comment has been minimized.

Copy link
Contributor

mperham commented May 4, 2016

Paging @rafaelfranca, seems straightforward. Can this get into 5.0?

@kaspth

View changes

actionpack/lib/action_dispatch/request/session.rb Outdated
@@ -198,6 +198,11 @@ def merge!(other)
@delegate.merge!(other)
end

def each(&block)
hash = self.to_hash
hash.each(&block)

This comment has been minimized.

@kaspth

kaspth May 4, 2016

Member

Why not simply to_hash.each(&block)? 😁

@kaspth

View changes

actionpack/test/dispatch/request/session_test.rb Outdated
@@ -114,5 +114,31 @@ def delete_session(env, id, options); 123; end
}.new
end
end

class SessionIntegrationTest < ActionDispatch::IntegrationTest

This comment has been minimized.

@kaspth

kaspth May 4, 2016

Member

A tiny space too many before < 😁

Ensure compatibility between ActionDispatch::Request::Session and Rack
Adding the `each` method is required for ensuring compatibility between
Rails, and other Rack frameworks (like Sinatra, etc.), that are mounted
within Rails, and wish to use its session tooling. Prior to this, there
was an inconsistency between ActionDispatch::Request::Session and
Rack::Session::Cookie, due to the absence of the `each` method. This
should hopefully fix that error. :)

For a full integration test with Sinatra and a standalone Rack
application, you can check out the gist for that here: https://gist.github.com/maclover7/08cd95b0bfe259465314311941326470.

Solves #15843.

@maclover7 maclover7 force-pushed the maclover7:fix-15843 branch to 09159d8 May 4, 2016

@maclover7

This comment has been minimized.

Copy link
Member

maclover7 commented May 4, 2016

👍 updated @kaspth

@kaspth kaspth merged commit 3bed679 into rails:master May 4, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented May 4, 2016

🚀

@maclover7 maclover7 deleted the maclover7:fix-15843 branch May 4, 2016

@maclover7

This comment has been minimized.

Copy link
Member

maclover7 commented May 4, 2016

❤️ 💚 💙 💛 💜

@antonyr

This comment has been minimized.

Copy link

antonyr commented May 9, 2016

👍

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