Comments and changes to this ticket

• You flagged this item as spam.
  

  Bertg September 17th, 2010 @ 01:57 PM

  I'm seeing and expecting the same behaviour. I'm noticing that the compiled callback does not put any conditions on an after filter.

  _conditional_callback_around_21(halted) do
    unless halted
      result = do_before
      halted = (response_body)
    end
  
    value = yield if block_given? && !halted
    do_after
  end
  halted ? false : (block_given? ? value : true)
  

  I'm expecting that the last compiled line should be used as a condition on the "do_after" call.

  When adding a :if statement to the after_filter the output slightly changes on the last lines:

    if _callback_after_21(self)
      do_after
    end
  end
  halted ? false : (block_given? ? value : true)
  

  It seems to me that the last line should be included in the condition.

    if (halted ? false : (block_given? ? value : true)) && _callback_after_21(self)
      do_after
    end
  end
  

    if halted ? false : (block_given? ? value : true)
      do_after
    end
  end
  

• You flagged this item as spam.
  

  Bertg September 30th, 2010 @ 05:29 PM

  Surprised to see no response. Thinking this would be quite a serious issue...

• You flagged this item as spam.
  

  Marcelo Giorgi October 3rd, 2010 @ 04:51 AM

  • Assigned user set to “Santiago Pastorino”
  • Tag changed from “rails 3.0.0, actionpack, after_filter, before_filter, callbacks, chain” to “rails 3.0.0, actionpack, after_filter, before_filter, callbacks, chain, patch”

  Hi Bertg,

  I agree with you, I would expect Rails 3 to inherit the behavior of Rails 2.3.x filters.

  For that, I've wrote tests and patch to make it work as you suggested.

  BTW: This patch applies cleanly on master now.

  • after_filter-halts-when-before_filter-renders-redirects-or-returns-false.patch 6.1 KB
• 

  Neeraj Singh October 3rd, 2010 @ 10:40 AM

  • Importance changed from “” to “Low”

  Interestingly I am not able to reproduce this error. I tested with rails edge. In both render :text and redirect_to cases the after_filter is not invoked.

  Here is my test code

  class UsersController < ApplicationController
  
    before_filter :do_before
    after_filter  :do_after
  
    def do_before
      puts 'before >>>' * 10
      #render :text => 'filter should be halted'
      redirect_to surveys_path
    end
  
    def do_after
      puts 'after <<<' * 10
    end
  
    def index
      @users = User.all
    end
  
  end
  

  I'm sure I'm missing something. Not sure what? :-)

• 

  José Valim October 3rd, 2010 @ 01:00 PM

  • State changed from “new” to “open”
  • Milestone cleared.
  • Assigned user changed from “Santiago Pastorino” to “José Valim”

  Hey guys,

  If this bug still exists, I would recommend to use a solution similar to the one in ActiveModel::Callbacks:

  http://github.com/rails/rails/blob/master/activemodel/lib/active_mo...

  In other words, simple adding "!halted" as condition to :if in after_filter will fix the issue. I prefer this solution because AS::Callbacks is already too complex and adding another option will just make it worse.

• You flagged this item as spam.
  

  Marcelo Giorgi October 3rd, 2010 @ 03:34 PM

  Hi Neeraj and Jose,

  Neeraj, I run your code with rails edge and got the following output for this url http://localhost:3000/users:

  before >>>before >>>before >>>before >>>before >>>before >>>before >>>before >>>before >>>before >>>
  after <<

  So, for me it is reaching the after filter either when I redirect or render from the before_filter called do_before.

  Jose, Yeah I agree, AS::Callbacks is complex enough. So, I wrote a new patch here.

  Let me know what you think.

  • after_filter-halts-when-before_filter-renders-or-redirects.patch 6.2 KB
• 

  Jeremy Kemper October 15th, 2010 @ 11:02 PM

  • Milestone set to “3.0.2”

  [bulk edit]

• 

  Ryan Bigg October 16th, 2010 @ 02:28 AM

  • Tag changed from “rails 3.0.0, sheepskin boots, actionpack, after_filter, before_filter, callbacks, chain, patch” to “actionpack after_filter before_filter callbacks chain patch rails 3.0.0”

  Automatic cleanup of spam.

• 

  Ryan Bigg October 19th, 2010 @ 08:35 AM

  • Tag cleared.

  Automatic cleanup of spam.

• You flagged this item as spam.
  

  Evgeniy Dolzhenko October 22nd, 2010 @ 11:58 AM

  +1 having this issue with 3.0.1 application

• 

  José Valim October 22nd, 2010 @ 12:22 PM

  Great, I will apply this patch in the following days.

• 

  Neeraj Singh November 11th, 2010 @ 03:48 PM

  I still can't reproduce this problem with rails edge.

  I don't see boom in the following case.

  class UsersController < ApplicationController
    before_filter :f1
    before_filter :f2
  
    def f2
      raise 'boom'
    end
  
    def f1
      render :text => 'finished'
    end
  
    def index
      @users = User.all
    end
  end
  

• You flagged this item as spam.
  

  Evgeniy Dolzhenko November 11th, 2010 @ 03:55 PM

  It should be after_filter :f2 not before_filter :f2

• 

  Repository November 11th, 2010 @ 04:07 PM

  • State changed from “open” to “resolved”

  (from [2bb1c202b48c4f1c8d81927fb3e5fc00806231f9]) Make after_filter halt when before_filter renders or redirects [#5648 state:resolved]

  Signed-off-by: José Valim jose.valim@gmail.com
  https://github.com/rails/rails/commit/2bb1c202b48c4f1c8d81927fb3e5f...

• 

  Repository November 11th, 2010 @ 04:07 PM

  (from [0de95dfb2c99f188c5e4c98aa59f6e8f2736f70a]) Make after_filter halt when before_filter renders or redirects [#5648 state:resolved]

  Signed-off-by: José Valim jose.valim@gmail.com
  https://github.com/rails/rails/commit/0de95dfb2c99f188c5e4c98aa59f6...

• 

  Neeraj Singh November 11th, 2010 @ 04:23 PM

  My bad. Anyway it is committed now.