Skip to content

Commit

Permalink
defining a named_scope which overwrites an existing method is now all…
Browse files Browse the repository at this point in the history
…owed we just log a warning.

This was motivated by the fact that :open is defined on all classes
as such the named_scope "open" can never be used, without hacking
ActiveRecord with an "undef_method" [#4083 state:resolved]

Signed-off-by: wycats <wycats@gmail.com>
  • Loading branch information
matthewrudy authored and wycats committed Mar 28, 2010
1 parent 77a2a3d commit b0967cc
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 3 deletions.
3 changes: 2 additions & 1 deletion activerecord/lib/active_record/named_scope.rb
Expand Up @@ -102,7 +102,8 @@ def scope(name, options = {}, &block)
name = name.to_sym

if !scopes[name] && respond_to?(name, true)
raise ArgumentError, "Cannot define scope :#{name} because #{self.name}.#{name} method already exists."
logger.warn "Creating scope :#{name}. " \
"Overwriting existing method #{self.name}.#{name}."
end

scopes[name] = lambda do |parent_scope, *args|
Expand Down
17 changes: 15 additions & 2 deletions activerecord/test/cases/named_scope_test.rb
Expand Up @@ -371,8 +371,21 @@ def test_table_names_for_chaining_scopes_with_and_without_table_name_included
end

def test_named_scopes_with_reserved_names
[:where, :with_scope].each do |protected_method|
assert_raises(ArgumentError) { Topic.scope protected_method }
class << Topic
def public_method; end
public :public_method

def protected_method; end
protected :protected_method

def private_method; end
private :private_method
end

[:public_method, :protected_method, :private_method].each do |reserved_method|
assert Topic.respond_to?(reserved_method, true)
ActiveRecord::Base.logger.expects(:warn)
Topic.scope(reserved_method)
end
end

Expand Down

2 comments on commit b0967cc

@matthewrudy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for committing this @wycats.
I spent a lot of energy arguing about this,
but then gave up.

I think it's the correct behaviour to have a warning.

Take care.

@sergeych
Copy link

Choose a reason for hiding this comment

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

+1 lets change it from warn to debug/trace, it is a correctm excpected behavoir. Just spend alf hour with my colleague because of this warning: we thought that we have to rewrite our code to get rid ov scope overriding ;)

Please sign in to comment.