Skip to content
This repository
Browse code

Revert "fix the Flash middleware loading the session on every request…

… (very dangerous especially with Rack::Cache), it should only be loaded when the flash method is called"

This reverts commits e3069c6 and 2b2983d.

Reason: This add a non-backward compatible change in the way that flash
works now (swept in every request).
  • Loading branch information...
commit 3cba6eee66a4c25b93839ea6fd1da08d7780f2de 1 parent f7cde3e
Rafael Mendonça França authored June 05, 2012
1  actionpack/lib/action_controller/test_case.rb
@@ -460,6 +460,7 @@ def process(action, parameters = nil, session = nil, flash = nil, http_method =
460 460
 
461 461
         @request.session = ActionController::TestSession.new(session) if session
462 462
         @request.session["flash"] = @request.flash.update(flash || {})
  463
+        @request.session["flash"].sweep
463 464
 
464 465
         @controller.request = @request
465 466
         build_request_uri(action, parameters)
9  actionpack/lib/action_dispatch/middleware/flash.rb
@@ -4,7 +4,7 @@ class Request
4 4
     # read a notice you put there or <tt>flash["notice"] = "hello"</tt>
5 5
     # to put a new one.
6 6
     def flash
7  
-      @env[Flash::KEY] ||= (session["flash"] || Flash::FlashHash.new).tap(&:sweep)
  7
+      @env[Flash::KEY] ||= (session["flash"] || Flash::FlashHash.new)
8 8
     end
9 9
   end
10 10
 
@@ -235,6 +235,10 @@ def initialize(app)
235 235
     end
236 236
 
237 237
     def call(env)
  238
+      if (session = env['rack.session']) && (flash = session['flash'])
  239
+        flash.sweep
  240
+      end
  241
+
238 242
       @app.call(env)
239 243
     ensure
240 244
       session    = env['rack.session'] || {}
@@ -251,8 +255,7 @@ def call(env)
251 255
         env[KEY] = new_hash
252 256
       end
253 257
 
254  
-      if (!session.respond_to?(:loaded?) || session.loaded?) && # (reset_session uses {}, which doesn't implement #loaded?)
255  
-         session.key?('flash') && session['flash'].empty?
  258
+      if session.key?('flash') && session['flash'].empty?
256 259
         session.delete('flash')
257 260
       end
258 261
     end
20  railties/test/application/middleware/session_test.rb
@@ -26,25 +26,5 @@ def app
26 26
       require "#{app_path}/config/environment"
27 27
       assert app.config.session_options[:secure], "Expected session to be marked as secure"
28 28
     end
29  
-
30  
-    test "session is not loaded if it's not used" do
31  
-      make_basic_app
32  
-
33  
-      class ::OmgController < ActionController::Base
34  
-        def index
35  
-          if params[:flash]
36  
-            flash[:notice] = "notice"
37  
-          end
38  
-
39  
-          render :nothing => true
40  
-        end
41  
-      end
42  
-
43  
-      get "/?flash=true"
44  
-      get "/"
45  
-
46  
-      assert last_request.env["HTTP_COOKIE"]
47  
-      assert !last_response.headers["Set-Cookie"]
48  
-    end
49 29
   end
50 30
 end

6 notes on commit 3cba6ee

Will Bryant

But that behavior was itself a backwards-incompatible behavior regression in 3.1!

Rafael Mendonça França

So it should be reverted in 3.1 too

Will Bryant

What I mean is that this revert is putting it back to behavior that was itself a regression. It should never have been that way in the first place, it happened because of what was supposed to be refactoring (moving ActionController code to Rack middleware) accidentally changed the behavior.

Rafael Mendonça França

Wich behaviour? The flash behaviour was always this, swept in every request

Will Bryant

Not true, go test on older versions of rails. It was only swept if the flash object was loaded.

There was even test coverage that was supposed to prove that it didn't do that. Unfortunately the actioncontroller test harness code did not behave the same way as the Rack middleware, so the test was passing even though it didn't have the same behavior any more.

Rafael Mendonça França

This is how it was working in 3.1 and 3.2.

f7cde3e

We can't change a behavior in a stable release so it was reverted in 3.2 and should be reverted in 3.1. I didn't reverted on master exactly because this behaviour you proposed can be the right.

Please sign in to comment.
Something went wrong with that request. Please try again.