Skip to content
This repository
Browse code

Fix a bug where requires inside of autoloads were being added to the …

…autoloaded_constants list, causing mayhem. [#5165 state:resolved]
  • Loading branch information...
commit 1b97701e51667e6040b4c576cce9234edef1019e 1 parent 13df581
Yehuda Katz authored July 26, 2010
17  activesupport/lib/active_support/dependencies.rb
@@ -72,10 +72,6 @@ def self.locked(*methods)
72 72
         methods.each { |m| class_eval "def #{m}(*) lock { super } end", __FILE__, __LINE__ }
73 73
       end
74 74
 
75  
-      def get(key)
76  
-        (val = assoc(key)) ? val[1] : []
77  
-      end
78  
-
79 75
       locked :concat, :each, :delete_if, :<<
80 76
 
81 77
       def new_constants_for(frames)
@@ -85,7 +81,18 @@ def new_constants_for(frames)
85 81
           next unless mod.is_a?(Module)
86 82
 
87 83
           new_constants = mod.local_constant_names - prior_constants
88  
-          get(mod_name).concat(new_constants)
  84
+
  85
+          # If we are checking for constants under, say, :Object, nested under something
  86
+          # else that is checking for constants also under :Object, make sure the
  87
+          # parent knows that we have found, and taken care of, the constant.
  88
+          #
  89
+          # In particular, this means that since Kernel.require discards the constants
  90
+          # it finds, parents will be notified that about those constants, and not
  91
+          # consider them "new". As a result, they will not be added to the
  92
+          # autoloaded_constants list.
  93
+          each do |key, value|
  94
+            value.concat(new_constants) if key == mod_name
  95
+          end
89 96
 
90 97
           new_constants.each do |suffix|
91 98
             constants << ([mod_name, suffix] - ["Object"]).join("::")
3  activesupport/test/autoloading_fixtures/load_path/loaded_constant.rb
... ...
@@ -0,0 +1,3 @@
  1
+module LoadedConstant
  2
+end
  3
+
4  activesupport/test/autoloading_fixtures/loads_constant.rb
... ...
@@ -0,0 +1,4 @@
  1
+module LoadsConstant
  2
+end
  3
+
  4
+RequiresConstant
5  activesupport/test/autoloading_fixtures/requires_constant.rb
... ...
@@ -0,0 +1,5 @@
  1
+require "loaded_constant"
  2
+
  3
+module RequiresConstant
  4
+end
  5
+
49  activesupport/test/dependencies_test.rb
@@ -213,6 +213,48 @@ def test_nested_class_can_access_sibling
213 213
     end
214 214
   end
215 215
 
  216
+  def test_doesnt_break_normal_require
  217
+    path = File.expand_path("../autoloading_fixtures/load_path", __FILE__)
  218
+    original_path = $:.dup
  219
+    original_features = $".dup
  220
+    $:.push(path)
  221
+
  222
+    with_autoloading_fixtures do
  223
+      RequiresConstant
  224
+      assert defined?(RequiresConstant)
  225
+      assert defined?(LoadedConstant)
  226
+      ActiveSupport::Dependencies.clear
  227
+      RequiresConstant
  228
+      assert defined?(RequiresConstant)
  229
+      assert defined?(LoadedConstant)
  230
+    end
  231
+  ensure
  232
+    remove_constants(:RequiresConstant, :LoadedConstant, :LoadsConstant)
  233
+    $".replace(original_features)
  234
+    $:.replace(original_path)
  235
+  end
  236
+
  237
+  def test_doesnt_break_normal_require_nested
  238
+    path = File.expand_path("../autoloading_fixtures/load_path", __FILE__)
  239
+    original_path = $:.dup
  240
+    original_features = $".dup
  241
+    $:.push(path)
  242
+
  243
+    with_autoloading_fixtures do
  244
+      LoadsConstant
  245
+      assert defined?(LoadsConstant)
  246
+      assert defined?(LoadedConstant)
  247
+      ActiveSupport::Dependencies.clear
  248
+      LoadsConstant
  249
+      assert defined?(LoadsConstant)
  250
+      assert defined?(LoadedConstant)
  251
+    end
  252
+  ensure
  253
+    remove_constants(:RequiresConstant, :LoadedConstant, :LoadsConstant)
  254
+    $".replace(original_features)
  255
+    $:.replace(original_path)
  256
+  end
  257
+
216 258
   def failing_test_access_thru_and_upwards_fails
217 259
     with_autoloading_fixtures do
218 260
       assert ! defined?(ModuleFolder)
@@ -797,4 +839,11 @@ def test_unhook
797 839
   ensure
798 840
     ActiveSupport::Dependencies.hook!
799 841
   end
  842
+
  843
+private
  844
+  def remove_constants(*constants)
  845
+    constants.each do |constant|
  846
+      Object.send(:remove_const, constant) if Object.const_defined?(constant)
  847
+    end
  848
+  end
800 849
 end

11 notes on commit 1b97701

Konstantin Haase
rkh commented on 1b97701 July 26, 2010

Yay, merging ahead.

Rolly
luma commented on 1b97701 July 26, 2010

Nice work!

Willem van Bergen

Rails 3.0.0.rc won't autoload constant from the ROOT/lib directory anymore. This might or might not be intentional. Rails 3.0.0.beta4 was still autoloading constants from this directory.

It can easily be solved / worked around by adding this to your application.rb:

  # Custom directories with classes and modules you want to be autoloadable.
  config.autoload_paths += %W(#{config.root}/lib)
Mitya

2wvanbergen: For me Rails 3.0.0.beta4 does the same.

Darcy Laycock
Sutto commented on 1b97701 July 26, 2010

BadMinus: Not for me - as an example, I have an initializer I set the default form builder for formtastic in and the form builder was in lib. In beta4 it worked fine but in rc1 it needed the line above to work

Willem van Bergen

This definitely changed between beta4 and rc. I am loading tons of code this way from the lib folder that suddenly stopped working. Maybe you were working with a HEAD version instead of the released beta4 gem?

Note that this change isn't bad per se in my opinion, but a heads-up might have been nice.

Konstantin Haase
rkh commented on 1b97701 July 27, 2010

This change was intentionally, if I remember correctly. Seem unable to find the commit however.

Nicolas Blanco

indeed. In Rails 3b4 if your modules names or class names reflected the name of the file in folder "lib" (for example Module Foo in file named foo.rb), the file foo.rb were automaticaly required. This is not the case anymore in RC... Just wanted to know if that's a bug or feature...

Mitya

Sorry. I forgot, changed to git master :)

Matthew O'Riordan

I have the same problem. Is it intentional to no longer automatically include files in the /lib/ director? Surely not? I did fix it with
config.autoload_paths += %W(#{config.root}/lib)
in the application.rb file.

Konstantin Haase
rkh commented on 1b97701 August 27, 2010

It is intentional. Autoloading is not thread-safe and the lib folder is unlikely to be always loaded completely. There is a discussion on the Rails core ML atm.

Please sign in to comment.
Something went wrong with that request. Please try again.