Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Dependencies cleanup. Fixes #4221.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@4060 5ecf4fe2-1ee6-0310-87b1-e25e094e27de
  • Loading branch information...
commit 9efca53908c09b7f188183aec6c0a4a2df347316 1 parent 2440349
Nicholas Seckar seckar authored
6 activesupport/CHANGELOG
View
@@ -1,5 +1,11 @@
*SVN*
+* When possible, avoid incorrectly obtaining constants from parent modules. Fixes #4221. [Nicholas Seckar]
+
+* Add more tests for dependencies; refactor existing cases. [Nicholas Seckar]
+
+* Move Module#parent and Module#as_load_path into core_ext. Add Module#parent. [Nicholas Seckar]
+
* Add CachingTools::HashCaching to simplify the creation of nested, autofilling hashes. [Nicholas Seckar]
* Remove a hack intended to avoid unloading the same class twice, but which would not work anyways. [Nicholas Seckar]
4 activesupport/lib/active_support/core_ext/module.rb
View
@@ -1,3 +1,5 @@
require File.dirname(__FILE__) + '/module/inclusion'
require File.dirname(__FILE__) + '/module/attribute_accessors'
-require File.dirname(__FILE__) + '/module/delegation'
+require File.dirname(__FILE__) + '/module/delegation'
+require File.dirname(__FILE__) + '/module/introspection'
+require File.dirname(__FILE__) + '/module/loading'
21 activesupport/lib/active_support/core_ext/module/introspection.rb
View
@@ -0,0 +1,21 @@
+class Module
+ # Return the module which contains this one; if this is a root module, such as
+ # +::MyModule+, then Object is returned.
+ def parent
+ parent_name = name.split('::')[0..-2] * '::'
+ parent_name.empty? ? Object : parent_name.constantize
+ end
+
+ # Return all the parents of this module, ordered from nested outwards. The
+ # receiver is not contained within the result.
+ def parents
+ parents = []
+ parts = name.split('::')[0..-2]
+ until parts.empty?
+ parents << (parts * '::').constantize
+ parts.pop
+ end
+ parents << Object unless parents.include? Object
+ parents
+ end
+end
13 activesupport/lib/active_support/core_ext/module/loading.rb
View
@@ -0,0 +1,13 @@
+class Module
+ def as_load_path
+ if self == Object || self == Kernel
+ ''
+ elsif is_a? Class
+ parent == self ? '' : parent.as_load_path
+ else
+ name.split('::').collect do |word|
+ word.underscore
+ end * '/'
+ end
+ end
+end
23 activesupport/lib/active_support/dependencies.rb
View
@@ -78,23 +78,6 @@ class Module #:nodoc:
# Rename the original handler so we can chain it to the new one
alias :rails_original_const_missing :const_missing
- def parent
- parent_name = name.split('::')[0..-2] * '::'
- parent_name.empty? ? Object : parent_name.constantize
- end
-
- def as_load_path
- if self == Object || self == Kernel
- ''
- elsif is_a? Class
- parent == self ? '' : parent.as_load_path
- else
- name.split('::').collect do |word|
- word.underscore
- end * '/'
- end
- end
-
# Use const_missing to autoload associations so we don't have to
# require_association when using single-table inheritance.
def const_missing(class_id)
@@ -116,7 +99,11 @@ def const_missing(class_id)
return mod
end
- if parent && parent != self
+ # Attempt to access the name from the parent, unless we don't have a valid
+ # parent, or the constant is already defined in the parent. If the latter
+ # is the case, then we are being queried via self::class_id, and we should
+ # avoid returning the constant from the parent if possible.
+ if parent && parent != self && ! parents.any? { |p| p.const_defined?(class_id) }
suppress(NameError) do
return parent.send(:const_missing, class_id)
end
2  activesupport/test/autoloading_fixtures/module_folder/nested_class.rb
View
@@ -0,0 +1,2 @@
+class ModuleFolder::NestedClass
+end
2  activesupport/test/autoloading_fixtures/module_folder/nested_sibling.rb
View
@@ -0,0 +1,2 @@
+class ModuleFolder::NestedSibling
+end
16 activesupport/test/core_ext/module_test.rb
View
@@ -82,4 +82,20 @@ def test_missing_delegation_target
assert_raises(ArgumentError) { eval($nowhere) }
assert_raises(ArgumentError) { eval($noplace) }
end
+
+ def test_parent
+ assert_equal Yz::Zy, Yz::Zy::Cd.parent
+ assert_equal Yz, Yz::Zy.parent
+ assert_equal Object, Yz.parent
+ end
+
+ def test_parents
+ assert_equal [Yz::Zy, Yz, Object], Yz::Zy::Cd.parents
+ assert_equal [Yz, Object], Yz::Zy.parents
+ end
+
+ def test_as_load_path
+ assert_equal 'yz/zy', Yz::Zy.as_load_path
+ assert_equal 'yz', Yz.as_load_path
+ end
end
175 activesupport/test/dependencies_test.rb
View
@@ -7,6 +7,17 @@ class DependenciesTest < Test::Unit::TestCase
def teardown
Dependencies.clear
end
+
+ def with_loading(from_dir = nil)
+ prior_path = $LOAD_PATH.clone
+ $LOAD_PATH.unshift "#{File.dirname(__FILE__)}/#{from_dir}" if from_dir
+ old_mechanism, Dependencies.mechanism = Dependencies.mechanism, :load
+ yield
+ ensure
+ $LOAD_PATH.clear
+ $LOAD_PATH.concat prior_path
+ Dependencies.mechanism = old_mechanism
+ end
def test_tracking_loaded_files
require_dependency(File.dirname(__FILE__) + "/dependencies/service_one")
@@ -29,76 +40,68 @@ def test_missing_association_raises_nothing
end
def test_dependency_which_raises_exception_isnt_added_to_loaded_set
- old_mechanism, Dependencies.mechanism = Dependencies.mechanism, :load
+ with_loading do
+ filename = "#{File.dirname(__FILE__)}/dependencies/raises_exception"
+ $raises_exception_load_count = 0
- filename = "#{File.dirname(__FILE__)}/dependencies/raises_exception"
- $raises_exception_load_count = 0
+ 5.times do |count|
+ assert_raises(RuntimeError) { require_dependency filename }
+ assert_equal count + 1, $raises_exception_load_count
- 5.times do |count|
- assert_raises(RuntimeError) { require_dependency filename }
- assert_equal count + 1, $raises_exception_load_count
-
- assert !Dependencies.loaded.include?(filename)
- assert !Dependencies.history.include?(filename)
+ assert !Dependencies.loaded.include?(filename)
+ assert !Dependencies.history.include?(filename)
+ end
end
- ensure
- Dependencies.mechanism = old_mechanism
end
def test_warnings_should_be_enabled_on_first_load
- old_mechanism, Dependencies.mechanism = Dependencies.mechanism, :load
- old_warnings, Dependencies.warnings_on_first_load = Dependencies.warnings_on_first_load, true
+ with_loading do
+ old_warnings, Dependencies.warnings_on_first_load = Dependencies.warnings_on_first_load, true
- filename = "#{File.dirname(__FILE__)}/dependencies/check_warnings"
- $check_warnings_load_count = 0
+ filename = "#{File.dirname(__FILE__)}/dependencies/check_warnings"
+ $check_warnings_load_count = 0
- assert !Dependencies.loaded.include?(filename)
- assert !Dependencies.history.include?(filename)
+ assert !Dependencies.loaded.include?(filename)
+ assert !Dependencies.history.include?(filename)
- silence_warnings { require_dependency filename }
- assert_equal 1, $check_warnings_load_count
- assert_equal true, $checked_verbose, 'On first load warnings should be enabled.'
+ silence_warnings { require_dependency filename }
+ assert_equal 1, $check_warnings_load_count
+ assert_equal true, $checked_verbose, 'On first load warnings should be enabled.'
- assert Dependencies.loaded.include?(filename)
- Dependencies.clear
- assert !Dependencies.loaded.include?(filename)
- assert Dependencies.history.include?(filename)
+ assert Dependencies.loaded.include?(filename)
+ Dependencies.clear
+ assert !Dependencies.loaded.include?(filename)
+ assert Dependencies.history.include?(filename)
- silence_warnings { require_dependency filename }
- assert_equal 2, $check_warnings_load_count
- assert_equal nil, $checked_verbose, 'After first load warnings should be left alone.'
+ silence_warnings { require_dependency filename }
+ assert_equal 2, $check_warnings_load_count
+ assert_equal nil, $checked_verbose, 'After first load warnings should be left alone.'
- assert Dependencies.loaded.include?(filename)
- Dependencies.clear
- assert !Dependencies.loaded.include?(filename)
- assert Dependencies.history.include?(filename)
+ assert Dependencies.loaded.include?(filename)
+ Dependencies.clear
+ assert !Dependencies.loaded.include?(filename)
+ assert Dependencies.history.include?(filename)
- enable_warnings { require_dependency filename }
- assert_equal 3, $check_warnings_load_count
- assert_equal true, $checked_verbose, 'After first load warnings should be left alone.'
+ enable_warnings { require_dependency filename }
+ assert_equal 3, $check_warnings_load_count
+ assert_equal true, $checked_verbose, 'After first load warnings should be left alone.'
- assert Dependencies.loaded.include?(filename)
- ensure
- Dependencies.mechanism = old_mechanism
- Dependencies.warnings_on_first_load = old_warnings
+ assert Dependencies.loaded.include?(filename)
+ end
end
def test_mutual_dependencies_dont_infinite_loop
- $LOAD_PATH.unshift "#{File.dirname(__FILE__)}/dependencies"
- old_mechanism, Dependencies.mechanism = Dependencies.mechanism, :load
+ with_loading 'dependencies' do
+ $mutual_dependencies_count = 0
+ assert_nothing_raised { require_dependency 'mutual_one' }
+ assert_equal 2, $mutual_dependencies_count
- $mutual_dependencies_count = 0
- assert_nothing_raised { require_dependency 'mutual_one' }
- assert_equal 2, $mutual_dependencies_count
+ Dependencies.clear
- Dependencies.clear
-
- $mutual_dependencies_count = 0
- assert_nothing_raised { require_dependency 'mutual_two' }
- assert_equal 2, $mutual_dependencies_count
- ensure
- $LOAD_PATH.shift
- Dependencies.mechanism = old_mechanism
+ $mutual_dependencies_count = 0
+ assert_nothing_raised { require_dependency 'mutual_two' }
+ assert_equal 2, $mutual_dependencies_count
+ end
end
def test_as_load_path
@@ -106,43 +109,53 @@ def test_as_load_path
end
def test_module_loading
- begin
- $LOAD_PATH.unshift "#{File.dirname(__FILE__)}/autoloading_fixtures"
- old_mechanism, Dependencies.mechanism = Dependencies.mechanism, :load
-
+ with_loading 'autoloading_fixtures' do
assert_kind_of Module, A
assert_kind_of Class, A::B
assert_kind_of Class, A::C::D
assert_kind_of Class, A::C::E::F
- ensure
- $LOAD_PATH.shift
- Dependencies.mechanism = old_mechanism
end
end
- def test_non_existing_cost_raises_nameerrror
- begin
- $LOAD_PATH.unshift "#{File.dirname(__FILE__)}/autoloading_fixtures"
- old_mechanism, Dependencies.mechanism = Dependencies.mechanism, :load
- assert_raises(NameError) do
- DoesNotExist
- end
-
- assert_raises(NameError) do
- NoModule::DoesNotExist
- end
-
- assert_raises(NameError) do
- A::DoesNotExist
- end
-
- assert_raises(NameError) do
- A::B::DoesNotExist
- end
- ensure
- $LOAD_PATH.shift
- Dependencies.mechanism = old_mechanism
+ def test_non_existing_const_raises_name_error
+ with_loading 'autoloading_fixtures' do
+ assert_raises(NameError) { DoesNotExist }
+ assert_raises(NameError) { NoModule::DoesNotExist }
+ assert_raises(NameError) { A::DoesNotExist }
+ assert_raises(NameError) { A::B::DoesNotExist }
end
-
end
+
+ def test_directories_should_manifest_as_modules
+ with_loading 'autoloading_fixtures' do
+ assert_kind_of Module, ModuleFolder
+ Object.send :remove_const, :ModuleFolder
+ end
+ end
+
+ def test_nested_class_access
+ with_loading 'autoloading_fixtures' do
+ assert_kind_of Class, ModuleFolder::NestedClass
+ Object.send :remove_const, :ModuleFolder
+ end
+ end
+
+ def test_nested_class_can_access_sibling
+ with_loading 'autoloading_fixtures' do
+ sibling = ModuleFolder::NestedClass.class_eval "NestedSibling"
+ assert defined?(ModuleFolder::NestedSibling)
+ assert_equal ModuleFolder::NestedSibling, sibling
+ Object.send :remove_const, :ModuleFolder
+ end
+ end
+
+ def failing_test_access_thru_and_upwards_fails
+ with_loading 'autoloading_fixtures' do
+ assert ! defined?(ModuleFolder)
+ assert_raises(NameError) { ModuleFolder::Object }
+ assert_raises(NameError) { ModuleFolder::NestedClass::Object }
+ Object.send :remove_const, :ModuleFolder
+ end
+ end
+
end
Please sign in to comment.
Something went wrong with that request. Please try again.