Skip to content

Commit

Permalink
Rename all action callbacks from *_filter to *_action
Browse files Browse the repository at this point in the history
  • Loading branch information
@dhh
dhh committed Dec 7, 2012
1 parent c205284 commit 9d62e04
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 69 deletions.
27 changes: 27 additions & 0 deletions actionpack/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,31 @@
## Rails 4.0.0 (unreleased) ##

* Rename all action callbacks from *_filter to *_action to avoid the misconception that these
callbacks are only suited for transforming or halting the response. With the new style,
it's more inviting to use them as they were intended, like setting shared ivars for views.

Example:

class PeopleController < ActionController::Base
before_action :set_person, except: [ :index, :new, :create ]
before_action :ensure_permission, only: [ :edit, :update ]

...

private
def set_person
@person = current_account.people.find(params[:id])
end

def ensure_permission
current_person.change_change?(@person)

This comment has been minimized.

Sign in to view

Copy link
@garethrees

garethrees Dec 7, 2012

Contributor

Should that read current_person.can_change?(@person)?

This comment has been minimized.

Sign in to view

Copy link
@dhh

dhh via email Dec 7, 2012

Author Member

This comment has been minimized.

Sign in to view

Copy link
@garethrees

garethrees Dec 7, 2012

Contributor

No problem, nice change 👍
end
end

The old *_filter methods still work with no deprecation notice.

*DHH*

* Add :if / :unless conditions to fragment cache:

<%= cache @model, if: some_condition(@model) do %>
Expand Down
150 changes: 86 additions & 64 deletions actionpack/lib/abstract_controller/callbacks.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,21 @@ def _normalize_callback_option(options, from, to) # :nodoc:
end
end

# Skip before, after, and around filters matching any of the names
# Skip before, after, and around action callbacks matching any of the names
# Aliased as skip_filter.
#
# ==== Parameters
# * <tt>names</tt> - A list of valid names that could be used for
# callbacks. Note that skipping uses Ruby equality, so it's
# impossible to skip a callback defined using an anonymous proc
# using #skip_filter
def skip_filter(*names)
skip_before_filter(*names)
skip_after_filter(*names)
skip_around_filter(*names)
def skip_action_callback(*names)
skip_before_action(*names)
skip_after_action(*names)
skip_around_action(*names)
end

alias_method :skip_filter, :skip_action_callback

# Take callback names and an optional callback proc, normalize them,
# then call the block with each callback. This allows us to abstract
Expand All @@ -75,119 +78,138 @@ def _insert_callbacks(callbacks, block = nil)
end

##
# :method: before_filter
# :method: before_action
#
# :call-seq: before_filter(names, block)
# :call-seq: before_action(names, block)
#
# Append a before filter. See _insert_callbacks for parameter details.
# Append a callback before actions. See _insert_callbacks for parameter details.
# Aliased as before_filter.

##
# :method: prepend_before_filter
# :method: prepend_before_action
#
# :call-seq: prepend_before_filter(names, block)
# :call-seq: prepend_before_action(names, block)
#
# Prepend a before filter. See _insert_callbacks for parameter details.
# Prepend a callback before actions. See _insert_callbacks for parameter details.
# Aliased as prepend_before_action.

##
# :method: skip_before_filter
# :method: skip_before_action
#
# :call-seq: skip_before_filter(names)
# :call-seq: skip_before_action(names)
#
# Skip a before filter. See _insert_callbacks for parameter details.
# Skip a callback before actions. See _insert_callbacks for parameter details.
# Aliased as skip_before_filter.

##
# :method: append_before_filter
# :method: append_before_action
#
# :call-seq: append_before_filter(names, block)
# :call-seq: append_before_action(names, block)
#
# Append a before filter. See _insert_callbacks for parameter details.
# Append a callback before actions. See _insert_callbacks for parameter details.
# Aliased as append_before_filter.

##
# :method: after_filter
# :method: after_action
#
# :call-seq: after_filter(names, block)
# :call-seq: after_action(names, block)
#
# Append an after filter. See _insert_callbacks for parameter details.
# Append a callback after actions. See _insert_callbacks for parameter details.
# Aliased as after_filter.

##
# :method: prepend_after_filter
# :method: prepend_after_action
#
# :call-seq: prepend_after_filter(names, block)
# :call-seq: prepend_after_action(names, block)
#
# Prepend an after filter. See _insert_callbacks for parameter details.
# Prepend a callback after actions. See _insert_callbacks for parameter details.
# Aliased as prepend_after_filter.

##
# :method: skip_after_filter
# :method: skip_after_action
#
# :call-seq: skip_after_filter(names)
# :call-seq: skip_after_action(names)
#
# Skip an after filter. See _insert_callbacks for parameter details.
# Skip a callback after actions. See _insert_callbacks for parameter details.
# Aliased as skip_after_filter.

##
# :method: append_after_filter
# :method: append_after_action
#
# :call-seq: append_after_filter(names, block)
# :call-seq: append_after_action(names, block)
#
# Append an after filter. See _insert_callbacks for parameter details.
# Append a callback after actions. See _insert_callbacks for parameter details.
# Aliased as append_after_filter.

##
# :method: around_filter
# :method: around_action
#
# :call-seq: around_filter(names, block)
# :call-seq: around_action(names, block)
#
# Append an around filter. See _insert_callbacks for parameter details.
# Append a callback around actions. See _insert_callbacks for parameter details.
# Aliased as around_filter.

##
# :method: prepend_around_filter
# :method: prepend_around_action
#
# :call-seq: prepend_around_filter(names, block)
# :call-seq: prepend_around_action(names, block)
#
# Prepend an around filter. See _insert_callbacks for parameter details.
# Prepend a callback around actions. See _insert_callbacks for parameter details.
# Aliased as prepend_around_filter.

##
# :method: skip_around_filter
# :method: skip_around_action
#
# :call-seq: skip_around_filter(names)
# :call-seq: skip_around_action(names)
#
# Skip an around filter. See _insert_callbacks for parameter details.
# Skip a callback around actions. See _insert_callbacks for parameter details.
# Aliased as skip_around_filter.

##
# :method: append_around_filter
# :method: append_around_action
#
# :call-seq: append_around_filter(names, block)
# :call-seq: append_around_action(names, block)
#
# Append an around filter. See _insert_callbacks for parameter details.
# Append a callback around actions. See _insert_callbacks for parameter details.
# Aliased as append_around_filter.

# set up before_filter, prepend_before_filter, skip_before_filter, etc.
# set up before_action, prepend_before_action, skip_before_action, etc.
# for each of before, after, and around.
[:before, :after, :around].each do |filter|
[:before, :after, :around].each do |callback|
class_eval <<-RUBY_EVAL, __FILE__, __LINE__ + 1
# Append a before, after or around filter. See _insert_callbacks
# Append a before, after or around callback. See _insert_callbacks
# for details on the allowed parameters.
def #{filter}_filter(*names, &blk) # def before_filter(*names, &blk)
_insert_callbacks(names, blk) do |name, options| # _insert_callbacks(names, blk) do |name, options|
set_callback(:process_action, :#{filter}, name, options) # set_callback(:process_action, :before, name, options)
end # end
end # end
def #{callback}_action(*names, &blk) # def before_action(*names, &blk)
_insert_callbacks(names, blk) do |name, options| # _insert_callbacks(names, blk) do |name, options|
set_callback(:process_action, :#{callback}, name, options) # set_callback(:process_action, :before, name, options)
end # end
end # end
# Prepend a before, after or around filter. See _insert_callbacks
alias_method :#{callback}_filter, :#{callback}_action
# Prepend a before, after or around callback. See _insert_callbacks
# for details on the allowed parameters.
def prepend_#{filter}_filter(*names, &blk) # def prepend_before_filter(*names, &blk)
_insert_callbacks(names, blk) do |name, options| # _insert_callbacks(names, blk) do |name, options|
set_callback(:process_action, :#{filter}, name, options.merge(:prepend => true)) # set_callback(:process_action, :before, name, options.merge(:prepend => true))
end # end
end # end
def prepend_#{callback}_action(*names, &blk) # def prepend_before_action(*names, &blk)
_insert_callbacks(names, blk) do |name, options| # _insert_callbacks(names, blk) do |name, options|
set_callback(:process_action, :#{callback}, name, options.merge(:prepend => true)) # set_callback(:process_action, :before, name, options.merge(:prepend => true))
end # end
end # end
alias_method :prepend_#{callback}_filter, :prepend_#{callback}_action
# Skip a before, after or around filter. See _insert_callbacks
# Skip a before, after or around callback. See _insert_callbacks
# for details on the allowed parameters.
def skip_#{filter}_filter(*names) # def skip_before_filter(*names)
_insert_callbacks(names) do |name, options| # _insert_callbacks(names) do |name, options|
skip_callback(:process_action, :#{filter}, name, options) # skip_callback(:process_action, :before, name, options)
end # end
end # end
# *_filter is the same as append_*_filter
alias_method :append_#{filter}_filter, :#{filter}_filter # alias_method :append_before_filter, :before_filter
def skip_#{callback}_action(*names) # def skip_before_action(*names)
_insert_callbacks(names) do |name, options| # _insert_callbacks(names) do |name, options|
skip_callback(:process_action, :#{callback}, name, options) # skip_callback(:process_action, :before, name, options)
end # end
end # end
alias_method :skip_#{callback}_filter, :skip_#{callback}_action
# *_action is the same as append_*_action
alias_method :append_#{callback}_action, :#{callback}_action # alias_method :append_before_action, :before_action
alias_method :append_#{callback}_filter, :#{callback}_action # alias_method :append_before_filter, :before_action
RUBY_EVAL
end
end
Expand Down
10 changes: 5 additions & 5 deletions actionpack/test/abstract/callbacks_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,11 @@ def setup
end

class Callback3 < ControllerWithCallbacks
before_filter do |c|
before_action do |c|
c.instance_variable_set("@text", "Hello world")
end

after_filter do |c|
after_action do |c|
c.instance_variable_set("@second", "Goodbye")
end

Expand All @@ -114,8 +114,8 @@ def setup
end

class CallbacksWithConditions < ControllerWithCallbacks
before_filter :list, :only => :index
before_filter :authenticate, :except => :index
before_action :list, :only => :index
before_action :authenticate, :except => :index

def index
self.response_body = @list.join(", ")
Expand Down Expand Up @@ -202,7 +202,7 @@ def setup
end

class ChangedConditions < Callback2
before_filter :first, :only => :index
before_action :first, :only => :index

def not_index
@text ||= nil
Expand Down

32 comments on commit 9d62e04

@jeremy
Copy link
Member

@jeremy jeremy commented on 9d62e04 Dec 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@luizkowalski
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice one!

@kristianfreeman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this to action makes a lot more sense for people learning these things (like me) in understanding that there's a verb going on here (as in :authenticate as an action). Thanks for this.
👍

@josh
Copy link
Contributor

@josh josh commented on 9d62e04 Dec 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dig it. ⚡

@natebird
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small changes like this is Rails that make it better semantically are what keep me using it. Great change.

@giedriusr
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@wildchild
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I miss Merb's before and after.

@milushov
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@dvarelap
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@AlexanderPavlenko
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the return value (true or false) still control the flow?
It looks more natural for me to expect method with such a name before_action to raise exception or call some another method like halt!.

@glongman
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for me

@fxn
Copy link
Member

@fxn fxn commented on 9d62e04 Dec 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value was taken into account in the early days, but it's been a long time since filters are halted by rendering or redirecting.

@3den
Copy link

@3den 3den commented on 9d62e04 Dec 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhh why not just before :action, :do_something?

@dhh
Copy link
Member Author

@dhh dhh commented on 9d62e04 Dec 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marcelo, because that's terribly ambiguous. before what? before rendering? before :authenticate "before authenticate"? before_action :authenticate is much clearer.

@mislav
Copy link
Member

@mislav mislav commented on 9d62e04 Dec 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the example in the documentation:

def set_person
  @person = current_account.people.find(params[:id])
end

Having a separate method just to set an ivar is an antipattern. I don't want to teach this to less experienced developer.

A much better implementation would be: (although, then it quits being an example for before_action)

def current_person
  @current_person ||= current_account.people.find(params[:id])
end

@qingwang
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before_filter did confuse me a little bit at the beginning. Nice change. I was thinking before_do , after_do .

@dhh
Copy link
Member Author

@dhh dhh commented on 9d62e04 Dec 7, 2012 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@3den
Copy link

@3den 3den commented on 9d62e04 Dec 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dhh, Thanks for your reply. One of the best thinks about rails is it clean and easy to read syntax. For sure _action is more meaningful than _filter but both add redundancy to de code. In plain english before :save, :calculate_total reads much cleaner than before_action :save, :calculate_total.

@dhh
Copy link
Member Author

@dhh dhh commented on 9d62e04 Dec 7, 2012 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@avgerin0s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@hron84
Copy link

@hron84 hron84 commented on 9d62e04 Dec 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@matthewrobertson
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel 😐 about this. I think it would have been nice if it were called before_action from the beginning of time but I dunno if you actually get a lot of milage from the principle of least surprise . As someone thats been coding rails for a while I'm going to be like: "WTF is a before_action!?" If it aint broke, why fix it?

@muescha
Copy link

@muescha muescha commented on 9d62e04 Dec 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not an alias method which inform about the removing version - dont take all the old legacy code with every version. better clean it. too much similar methods also confuses newcomers

alias_deprecated_method 4.1, :old, :new

@steveklabnik
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@muescha because it is not deprecated at this time. Just introducing a new name. If it's significantly nicer, we'll deprecate the old one.

@PikachuEXE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name makes much more sense! 👍

@ck3g
Copy link
Contributor

@ck3g ck3g commented on 9d62e04 Dec 8, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@hpyhacking
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

对代码有洁癖

@knwang
Copy link

@knwang knwang commented on 9d62e04 Dec 15, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. +1

@jocubeit
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think _action is self-explanatory. It'd be nice though if we had a backport for 3.2.10, just one more thing we need to remember to do for the upgrade. Before you all retaliate I know before_filter and after_filter aren't deprecated yet, but most will want to use the new syntax, so moving to _action will become the norm.

@carlosantoniodasilva
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MeetDom sorry, no new features are backported to 3.2, only bug fixes. But it should be dead easy to alias the methods before/after filter in your application controller, and start using as before/after action.

@examplecode
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good job ,it's more appropriate

@liuganggang
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

长姿势了:+1

Please sign in to comment.