Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Callback collision #1777

Closed
wants to merge 3 commits into from

10 participants

@mandrews

Rebased version of previous pull request see:
#269

@sikachu
Collaborator

@josevalim bro, review please :)

@bgentry

@mandrews thanks for staying on top of this. Looking forward to seeing this merged before 3.1

@josevalim
Owner

Thanks for the pull request but I am a bit afraid of this having collateral effects. For example, the options :only and :except used in controllers are not removed from the options hash, so we can have different behavior after applying this commit as callbacks will be no longer considered identical just by their name.

That is just example, we may have other cases.

@bgentry

@josevalim in the other pull request, @mandrews mentioned this as a possibility which you liked:

@josevalim- I agree that hard coding :on is not appropriate. How about instead of white listing options that imply difference, we black list options that determine equivalence.

In this case, how would you like that to be implemented? I really want a fix for this issue to be included in the next releases so I'm willing to spend some time to make that happen in the next day or 2.

@josevalim
Owner

@bgentry Yeah, I remember that. Although I could see such flaws just after looking at your pull request as I saw :per_key and remembered of :only and :except. I think our best option would be to allow :on => [:create, :update] or something similar, which I believe I also commented on the other thread. Sorry for the back and forth.

@sikachu
Collaborator

What's the status of this bro @josevalim?

@josevalim
Owner

The status is the same as in my last comment. :)

@isaacsanders

@mandrews Is this still an issue?

@mandrews

@isaacsanders without the changes the unit test still fails on master. If the approach to fixing the issue is OK I can easily make another pull request (git pull --rebase fails).

@frodsan

@josevalim any news?

@frodsan

@steveklabnik Can we close it and keep it with #6936?

@steveklabnik
Collaborator

I think so, but let's wait until SOMETHING gets accepted before we get close-happy.

@spastorino
Owner

I've merged #6936 can we close this now?

@steveklabnik
Collaborator

Seems so!

@kulbida

Found this issue...
I have a code something like

class User < ActiveRecord::Base
  ...
  before_validation(:on => :create) {:set_current_ratio}
  before_validation(:on => :create) {:set_free_until }
  ...
end

What I'm getting with User._validation_callbacks.map(&:filter) is ["_callback_before_779", "_callback_before_783"]
What is this _callback_before_783 and why I can't get a name like set_current_ratio of the callback?
Thanks.

@kulbida
  before_validation :set_current_ratio, :on => :create
  before_validation :set_free_until, :on => :create

did the magic but why this

  before_validation(:on => :create) {:set_current_ratio}
  before_validation(:on => :create) {:set_free_until }

doesn't work?

@pixeltrix
Owner

@kulbida because you're passing a block in the former example - you need to call the methods directly:

before_validation(:on => :create){ set_current_ratio }
before_validation(:on => :create){ set_free_until }

For your own benefit in the future you'd be better off asking questions either on the Rails Talk mailing list or Stack Overflow since you're more likely to have a positive response than adding a comment to an issue that was closed three months ago.

@kulbida

Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
51 activerecord/test/cases/callbacks_test.rb
@@ -137,6 +137,34 @@ def history
end
end
+class ContextualCallbacksDeveloper < ActiveRecord::Base
+ set_table_name 'developers'
+
+ before_validation { history << :before_validation }
+ before_validation :before_validation_on_create_and_update, :on => :create
+ before_validation :before_validation_on_create_and_update, :on => :update
+
+ validate do
+ history << :validate
+ end
+
+ after_validation { history << :after_validation }
+ after_validation :after_validation_on_create_and_update, :on => :create
+ after_validation :after_validation_on_create_and_update, :on => :update
+
+ def before_validation_on_create_and_update
+ history << "before_validation_on_#{self.validation_context}".to_sym
+ end
+
+ def after_validation_on_create_and_update
+ history << "after_validation_on_#{self.validation_context}".to_sym
+ end
+
+ def history
+ @history ||= []
+ end
+end
+
class CallbackCancellationDeveloper < ActiveRecord::Base
set_table_name 'developers'
@@ -285,6 +313,17 @@ def test_validate_on_create
], david.history
end
+ def test_validate_on_contextual_create
+ david = ContextualCallbacksDeveloper.create('name' => 'David', 'salary' => 1000000)
+ assert_equal [
+ :before_validation,
+ :before_validation_on_create,
+ :validate,
+ :after_validation,
+ :after_validation_on_create
+ ], david.history
+ end
+
def test_update
david = CallbackDeveloper.find(1)
david.save
@@ -344,6 +383,18 @@ def test_validate_on_update
], david.history
end
+ def test_validate_on_contextual_update
+ david = ContextualCallbacksDeveloper.find(1)
+ david.save
+ assert_equal [
+ :before_validation,
+ :before_validation_on_update,
+ :validate,
+ :after_validation,
+ :after_validation_on_update
+ ], david.history
+ end
+
def test_destroy
david = CallbackDeveloper.find(1)
david.destroy
View
12 activesupport/lib/active_support/callbacks.rb
@@ -129,8 +129,12 @@ def next_id
@@_callback_sequence += 1
end
- def matches?(_kind, _filter)
- @kind == _kind && @filter == _filter
+ ISOMORPHIC_OPTIONS = [:if, :unless, :per_key]
+
+ def matches?(_kind, _filter, _options = {})
+ @kind == _kind && @filter == _filter &&
+ @options.reject { |k,v| ISOMORPHIC_OPTIONS.include?(k) } ==
+ _options.reject { |k,v| ISOMORPHIC_OPTIONS.include?(k) }
end
def _update_filter(filter_options, new_options)
@@ -492,7 +496,7 @@ def set_callback(name, *filter_list, &block)
end
filters.each do |filter|
- chain.delete_if {|c| c.matches?(type, filter) }
+ chain.delete_if {|c| c.matches?(type, filter, options) }
end
options[:prepend] ? chain.unshift(*(mapped.reverse)) : chain.push(*mapped)
@@ -511,7 +515,7 @@ def set_callback(name, *filter_list, &block)
def skip_callback(name, *filter_list, &block)
__update_callbacks(name, filter_list, block) do |target, chain, type, filters, options|
filters.each do |filter|
- filter = chain.find {|c| c.matches?(type, filter) }
+ filter = chain.find {|c| c.matches?(type, filter, options) }
if filter && options.any?
new_filter = filter.clone(chain, self)
Something went wrong with that request. Please try again.