This project is archived and is in readonly mode.

#6226 new
Alexey Ilyichev

before_filters overwriting each other

Reported by Alexey Ilyichev | December 27th, 2010 @ 12:28 PM

Hi!

Consider this code:

class UsersController < ApplicationController
  before_filter :log_filter_call, :only => :index
  before_filter :log_filter_call, :only => :show
  # ...
  # standard scaffold methods
private
  def log_filter_call
   logger.debug 'Before filter called during ' + params[:controller] + '#' + params[:action]
  end
end

In rails 2.3.10 log_filter_call not called for #index action, and called twice for #show action
In rails 3.0.3 log_filter_call not called for #index action, and called once for #show action

I expected it being called once for both #index and #show.

Comments and changes to this ticket

  • Alexey Ilyichev

    Alexey Ilyichev December 27th, 2010 @ 03:02 PM

    It seems to me that this code from active_support/callbacks.rb (set_callback method) is what causes that behaviour. I wonder why it was made this way.

    filters.each do |filter|
      chain.delete_if {|c| c.matches?(type, filter) }
    end
    
  • Oriol Gual

    Oriol Gual December 27th, 2010 @ 03:32 PM

    Why don't you just use this:

    class UsersController < ApplicationController
      before_filter :log_filter_call, :only => [:index, :show]
      # ...
    end
    
  • Alexey Ilyichev

    Alexey Ilyichev December 27th, 2010 @ 03:36 PM

    Originally we wanted to split some of our controllers into mixins, containing sets of actions, because in our app we have many namespaces that share alot of common actions, but not totally same. And some actions that should belong to different mixins might have the same before_filters, so it would be nice if we could declare the same filter in these mixins and not get a conflict like this.

  • 2kan

    2kan December 28th, 2010 @ 10:45 AM

    +1 I expected it being called once for both #index and #show too.

  • tikh

    tikh December 29th, 2010 @ 09:52 AM

    • Tag set to action_controller, before_filter

    +1

  • Alexey Ilyichev

    Alexey Ilyichev December 30th, 2010 @ 09:43 AM

    Seems it was intended this way. See commit ef692162.

  • Neeraj Singh

    Neeraj Singh January 22nd, 2011 @ 02:47 AM

    • Importance changed from “” to “Low”

    This is how callbacks are working right now.

    class A < ApplicationController
     before_filter :ensure_logged_in, :only => [:index] 
     def index
      end
    end
    
    class B < A
     before_filter :ensure_logged_in, :except => [:index] 
     # index will be called without calling ensure_logged_in
     def index
     end
    end
    

    If the behavior of the callback is changed to not to remove duplicate callbacks then a subclass always has to worry about unwanted changes propagating to it. That would mean any subclass will have to guard itself by using skip_before_filter then use its own filter.

    On the flip side I do think inheritance means subclass probably wants the behavior given to parent class.

    This change would potentially break existing apps if they are relying in the feature I illustrated above.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages