Fix warning: circular require considered harmful.

[kennyj]

I saw some warning when executing rails own testcases.

I don't know 8b05cfa is best way ;)
Please review it !

[rafaelfranca]

I think is better to remove :phone_number from the store_accessor

[kennyj]

In first, I removed it. But I got test failure.

  1) Failure:
test_0020_all stored attributes are returned(StoreTest) [test/cases/store_test.rb:139]:
--- expected
+++ actual
@@ -1 +1 @@
-[:color, :homepage, :favorite_food, :phone_number]
+[:color, :homepage, :favorite_food]

If remove one, the following lines aren't executed.

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/store.rb#L88-89

[rafaelfranca]

The problem is that the warning will be showed either in the application so I don't think we should define the store_attribute and also the custom accessor. If this test fails we can remove :phone_number from the assertion

[kennyj]

I agree with you. I'll change this.

[kennyj]

I updated and rebased.

@fxn please comment this especially 2a6f57f ?

[rafaelfranca]

@kennyj I cherry picked 1f42db8. Could you update this pull request to include only 2a6f57f?

[kennyj]

@rafaelfranca done.

[fxn]

Hi, two questions over here:

1. I have run the AS test suite and see some warnings, but none of them seem related to this. Have also run ruby -W -Ilib lib/active_support/deprecation.rb and see no warning. How to you get them?
2. When this file is evaluated, since the require calls are moved to the body of the class they are still being executed when the file is interpreted, as they did in the top level. How can this patch change anything related to circular dependencies?

[rafaelfranca]

1. Active Model and Active Record suites show the warning.

[fxn]

The interesting thing is that the warning is raised after the Deprecation class is defined. After following backtraces I have tracked down the issue to this (executed under activesupport):

$ ruby -W -Ilib -ractive_support -ractive_support/deprecation -e1

and seem to be related to the fact that active_support.rb sets an autoload for :Deprecation. I bet something funky is going on related to autoload. I'll leave it for tonight, will keep you guys posted interesting puzzle :).

[kennyj]

@fxn

1. When executing rake test in the activerecord, we got this error.
2. I suspect the cause of problem is auctoload :Deprecation.
   first, we require active_support/deprecation.
   second we require active_support/derecation/instance_delegator (example) in the active_support/deprecation,
   third in the instance_delegator.rb, we reference ActiveSupport::Deprecation, that is the target of autoload.
   I have same recognition that you mentioned in the second comment.

BTW: In the first draft about this PR,

class ActiveSupport::Deprecation; end # ★added
require 'active_support/core_ext/module/deprecation'
require 'active_support/deprecation/instance_delegator'
require 'active_support/deprecation/behaviors'
require 'active_support/deprecation/reporting'
require 'active_support/deprecation/method_wrappers'
require 'active_support/deprecation/proxy_wrappers'
require 'singleton'

module ActiveSupport
  # \Deprecation specifies the API used by Rails to deprecate methods, instance
  # variables, objects and constants.
  class Deprecation
    include Singleton

[fxn]

I can't think of a better way to avoid this warning. Everything seems used just fine, problem is since we defer loading to autoload, the first time you try to define the constant the autoload is triggered, so generally speaking you reopen the constant if you are in a different file.

I'll keep thinking about it, but let's merge. I'll add a comment later.