Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixes skipping object callback filters #10034

Merged
merged 1 commit into from

3 participants

@benmcredmond

This allows you to skip callbacks that are defined by objects, e.g. for
ActionController:

skip_after_filter MySpecialFilter

Previously this didn't work due to a bug in how Rails compared callbacks
in Callback#matches?. When a callback is compiled, if it's an object
filter (i.e. not a method, proc, etc.), Callback now defines a method on
@klass that is derived from the class name rather than @callback_id.
So, when skip_callback tries to find the appropriate callback to
remove, Callback can regenerate the method name for the filter
object and return the correct value for Callback#matches?.

activesupport/lib/active_support/callbacks.rb
@@ -234,6 +236,14 @@ def recompile_options!
@compiled_options = conditions.flatten.join(" && ")
end
+ def _method_name_for_object_filter(kind, filter)
+ class_name = filter.kind_of?(Class) ? filter.to_s : filter.class.to_s
+ class_name.gsub!(/<|>|#/, '')
+ class_name.gsub!(/\/|:/, "_")
+
+ "_callback_#{kind}_#{class_name.underscore}"
@josevalim Owner

We don't need to underscore it, do we? Just pass the full class name.

you're right. changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca
Owner

Could you add a CHANGELOG entry?

activesupport/lib/active_support/callbacks.rb
@@ -255,6 +264,8 @@ def recompile_options!
# a method is created that calls the before_foo method
# on the object.
def _compile_filter(filter)
+ @is_object_filter = false
@rafaelfranca Owner

Since it is internal could you call it @_is_object_filter?

sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@benmcredmond benmcredmond Fixes skipping object callback filters
This allows you to skip callbacks that are defined by objects, e.g. for
`ActionController`:

    skip_after_filter MySpecialFilter

Previously this didn't work due to a bug in how Rails compared callbacks
in `Callback#matches?`. When a callback is compiled, if it's an object
filter (i.e. not a method, proc, etc.), `Callback` now defines a method on
`@klass` that is derived from the class name rather than `@callback_id`.
So, when `skip_callback` tries to find the appropriate callback to
remove, `Callback` can regenerate the method name for the filter
object and return the correct value for `Callback#matches?`.
e377228
@benmcredmond

@rafaelfranca made those changes.

@rafaelfranca rafaelfranca merged commit c79c698 into rails:master
@rafaelfranca
Owner

This pull request broke the Rails tests. I'll revert it. Please fix the erros and submit another pull request. https://travis-ci.org/rails/rails/builds/6061839

@rafaelfranca rafaelfranca referenced this pull request from a commit
@rafaelfranca rafaelfranca Revert "Merge pull request #10034 from benofsky/fix_skipping_object_c…
…allback_filters"

This reverts commit c79c698, reversing
changes made to ba4c274.

This broke all the tests. See https://travis-ci.org/rails/rails/builds/6061839
49a0f55
@benmcredmond
@tenderlove tenderlove referenced this pull request from a commit
@tenderlove tenderlove Merge branch 'master' into railstest
* master: (44 commits)
  Improve the changelog entry [ci skip]
  Fix explicit names on multiple file fields
  Correctly parse bigint defaults in PostgreSQL
  Move changelog to the top [ci skip]
  Fix indent and remove extra white spaces
  Fix scope chaining + STI
  failing test for #9869
  Improve `belongs_to touch: true` timestamp test
  Sort modules in alphabetical order.
  Avoid an attempt to fetch old record when id was not present in touch callback
  Use the correct pk field from the reflected class to find the old record
  Refactor mail_to to not generate intermediate hashes when adding href
  Ensure mail_to helper does not modify the given html options hash
  Use inspect when writing the foreign key from the reflection
  Use a space after the comment sign when showing the result of commands
  Exclude template files for rdoc API [ci skip]
  template should have generic name
  use | to have more intent revealing code
  Revert "Merge pull request #10034 from benofsky/fix_skipping_object_callback_filters"
  stop depending on callbacks
  ...

Conflicts:
	railties/test/application/rake_test.rb
01034d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 4, 2013
  1. @benmcredmond

    Fixes skipping object callback filters

    benmcredmond authored
    This allows you to skip callbacks that are defined by objects, e.g. for
    `ActionController`:
    
        skip_after_filter MySpecialFilter
    
    Previously this didn't work due to a bug in how Rails compared callbacks
    in `Callback#matches?`. When a callback is compiled, if it's an object
    filter (i.e. not a method, proc, etc.), `Callback` now defines a method on
    `@klass` that is derived from the class name rather than `@callback_id`.
    So, when `skip_callback` tries to find the appropriate callback to
    remove, `Callback` can regenerate the method name for the filter
    object and return the correct value for `Callback#matches?`.
This page is out of date. Refresh to see the latest.
View
3  activesupport/CHANGELOG.md
@@ -14,6 +14,9 @@
*Charles Jones*
+* Fix skipping of filters defined by objects in `ActiveSupport::Callbacks::Callback`.
+
+ *Ben McRedmond*
## Rails 4.0.0.beta1 (February 25, 2013) ##
View
14 activesupport/lib/active_support/callbacks.rb
@@ -131,6 +131,7 @@ def next_id
end
def matches?(_kind, _filter)
+ _filter = _method_name_for_object_filter(_kind, _filter) if @_is_object_filter
@kind == _kind && @filter == _filter
end
@@ -234,6 +235,14 @@ def recompile_options!
@compiled_options = conditions.flatten.join(" && ")
end
+ def _method_name_for_object_filter(kind, filter)
+ class_name = filter.kind_of?(Class) ? filter.to_s : filter.class.to_s
+ class_name.gsub!(/<|>|#/, '')
+ class_name.gsub!(/\/|:/, "_")
+
+ "_callback_#{kind}_#{class_name}"
+ end
+
# Filters support:
#
# Arrays:: Used in conditions. This is used to specify
@@ -255,6 +264,8 @@ def recompile_options!
# a method is created that calls the before_foo method
# on the object.
def _compile_filter(filter)
+ @_is_object_filter = false
+
case filter
when Array
filter.map {|f| _compile_filter(f)}
@@ -269,7 +280,8 @@ def _compile_filter(filter)
method_name << (filter.arity == 1 ? "(self)" : " self, Proc.new ")
else
- method_name = "_callback_#{@kind}_#{next_id}"
+ method_name = _method_name_for_object_filter(kind, filter)
+ @_is_object_filter = true
@klass.send(:define_method, "#{method_name}_object") { filter }
_normalize_legacy_filter(kind, filter)
View
17 activesupport/test/callbacks_test.rb
@@ -66,6 +66,16 @@ def history
end
end
+ class CallbackClass
+ def self.before(model)
+ model.history << [:before_save, :class]
+ end
+
+ def self.after(model)
+ model.history << [:after_save, :class]
+ end
+ end
+
class Person < Record
[:before_save, :after_save].each do |callback_method|
callback_method_sym = callback_method.to_sym
@@ -73,6 +83,7 @@ class Person < Record
send(callback_method, callback_string(callback_method_sym))
send(callback_method, callback_proc(callback_method_sym))
send(callback_method, callback_object(callback_method_sym.to_s.gsub(/_save/, '')))
+ send(callback_method, CallbackClass)
send(callback_method) { |model| model.history << [callback_method_sym, :block] }
end
@@ -86,6 +97,7 @@ class PersonSkipper < Person
skip_callback :save, :after, :before_save_method, :unless => :yes
skip_callback :save, :after, :before_save_method, :if => :no
skip_callback :save, :before, :before_save_method, :unless => :no
+ skip_callback :save, :before, CallbackClass , :if => :yes
def yes; true; end
def no; false; end
end
@@ -430,6 +442,7 @@ def test_skip_person
[:before_save, :object],
[:before_save, :block],
[:after_save, :block],
+ [:after_save, :class],
[:after_save, :object],
[:after_save, :proc],
[:after_save, :string],
@@ -449,8 +462,10 @@ def test_save_person
[:before_save, :string],
[:before_save, :proc],
[:before_save, :object],
+ [:before_save, :class],
[:before_save, :block],
[:after_save, :block],
+ [:after_save, :class],
[:after_save, :object],
[:after_save, :proc],
[:after_save, :string],
@@ -715,8 +730,10 @@ def test_skip_writer
[:before_save, :string],
[:before_save, :proc],
[:before_save, :object],
+ [:before_save, :class],
[:before_save, :block],
[:after_save, :block],
+ [:after_save, :class],
[:after_save, :object],
[:after_save, :proc],
[:after_save, :string],
Something went wrong with that request. Please try again.