Permalink
Browse files

prevent Dependencies#remove_const from autoloading parents [fixes #8301]

  • Loading branch information...
1 parent 5aade82 commit 46ebce6b49d8c646241a11821c39e6bbc20d61d5 @fxn fxn committed Nov 27, 2012
Showing with 54 additions and 32 deletions.
  1. +44 −32 activesupport/lib/active_support/dependencies.rb
  2. +10 −0 activesupport/test/dependencies_test.rb
@@ -644,46 +644,58 @@ def remove_constant(const) #:nodoc:
normalized = const.to_s.sub(/\A::/, '')
normalized.sub!(/\A(Object::)+/, '')
- constants = normalized.split('::')
- to_remove = constants.pop
- parent_name = constants.empty? ? 'Object' : constants.join('::')
+ constants = normalized.split('::')
+ to_remove = constants.pop
- if parent = safe_constantize(parent_name)
- log "removing constant #{const}"
-
- # In an autoloaded user.rb like this
- #
- # autoload :Foo, 'foo'
- #
- # class User < ActiveRecord::Base
- # end
- #
- # we correctly register "Foo" as being autoloaded. But if the app does
- # not use the "Foo" constant we need to be careful not to trigger
- # loading "foo.rb" ourselves. While #const_defined? and #const_get? do
- # require the file, #autoload? and #remove_const don't.
+ if constants.empty?
+ parent = Object
+ else
+ # This method is robust to non-reachable constants.
#
- # We are going to remove the constant nonetheless ---which exists as
- # far as Ruby is concerned--- because if the user removes the macro
- # call from a class or module that were not autoloaded, as in the
- # example above with Object, accessing to that constant must err.
- unless parent.autoload?(to_remove)
- begin
- constantized = parent.const_get(to_remove, false)
- rescue NameError
- log "the constant #{const} is not reachable anymore, skipping"
- return
- else
- constantized.before_remove_const if constantized.respond_to?(:before_remove_const)
- end
- end
+ # Non-reachable constants may be passed if some of the parents were
+ # autoloaded and already removed. It is easier to do a sanity check
+ # here than require the caller to be clever. We check the parent
+ # rather than the very const argument because we do not want to
+ # trigger Kernel#autoloads, see the comment below.
+ parent_name = constants.join('::')
+ return unless qualified_const_defined?(parent_name)
+ parent = constantize(parent_name)
+ end
+ log "removing constant #{const}"
+
+ # In an autoloaded user.rb like this
+ #
+ # autoload :Foo, 'foo'
+ #
+ # class User < ActiveRecord::Base
+ # end
+ #
+ # we correctly register "Foo" as being autoloaded. But if the app does
+ # not use the "Foo" constant we need to be careful not to trigger
+ # loading "foo.rb" ourselves. While #const_defined? and #const_get? do
+ # require the file, #autoload? and #remove_const don't.
+ #
+ # We are going to remove the constant nonetheless ---which exists as
+ # far as Ruby is concerned--- because if the user removes the macro
+ # call from a class or module that were not autoloaded, as in the
+ # example above with Object, accessing to that constant must err.
+ unless parent.autoload?(to_remove)
begin
- parent.instance_eval { remove_const to_remove }
+ constantized = parent.const_get(to_remove, false)
rescue NameError
log "the constant #{const} is not reachable anymore, skipping"
+ return
+ else
+ constantized.before_remove_const if constantized.respond_to?(:before_remove_const)
end
end
+
+ begin
+ parent.instance_eval { remove_const to_remove }
+ rescue NameError
+ log "the constant #{const} is not reachable anymore, skipping"
+ end
end
protected
@@ -938,6 +938,16 @@ def test_remove_constant_does_not_trigger_loading_autoloads
assert !defined?(ShouldNotBeAutoloaded)
end
+ def test_remove_constant_does_not_autoload_already_removed_parents_as_a_side_effect
+ with_autoloading_fixtures do
+ ::A
+ ::A::B
+ ActiveSupport::Dependencies.remove_constant('A')
+ ActiveSupport::Dependencies.remove_constant('A::B')
+ assert !defined?(A)
+ end
+ end
+
def test_load_once_constants_should_not_be_unloaded
with_autoloading_fixtures do
ActiveSupport::Dependencies.autoload_once_paths = ActiveSupport::Dependencies.autoload_paths

0 comments on commit 46ebce6

Please sign in to comment.