This project is archived and is in readonly mode.

before_filters overwriting each other

#6226

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.

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

Activity

Alexey Ilyichev
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
```
December 27th, 2010 @ 03:02 PM

Oriol Gual

Why don't you just use this:

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

December 27th, 2010 @ 03:32 PM

Alexey Ilyichev
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.

December 27th, 2010 @ 03:36 PM
2kan
+1 I expected it being called once for both #index and #show too.

December 28th, 2010 @ 10:45 AM
tikh
- Tag set to action_controller, before_filter
+1

December 29th, 2010 @ 09:52 AM
Alexey Ilyichev
Seems it was intended this way. See commit ef692162.

December 30th, 2010 @ 09:43 AM
Neeraj Singh
- 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.
January 22nd, 2011 @ 02:47 AM