[ActiveSupport] Fix for #20489 - ActiveSupport::Concern#class_methods affects parent classes

[knovoselic]

This PR includes a test that reproduces the issue described in #20489 and a fix for it. Here's a short explanation of the issue by @ewoutkleinsmann:

ActiveSupport::Concern's class_methods uses const_defined? without a second argument and therefore also searches its ancestors for a ClassMethods module. This results in concerns included in a child class that add class methods to a parent class.

[rafaelfranca]

Tests are broken, could you take a look?

[knovoselic]

@rafaelfranca I was pretty tired so I thought I'd do it today. Should've left a comment, sorry about that. I was working on master. I just created an empty pull request, just to see if all tests on master are passing, and they are not:
#20502

Should I rebase my changes on top of another branch (one for which the tests are passing) and try again? Which one would you recommend (maybe 4-2-stable)?
Right now I do not know which tests are failing because of my change and which were failing before.

[rafaelfranca]

master is green as you can see here https://travis-ci.org/rails/rails/branches. You patch broke two tests. ConfigurableActiveSupport#test_configuration_accessors_are_not_available_on_instance and ConfigurableActiveSupport#test_configuration_is_crystalizeable. They are not broken on master.

[knovoselic]

@rafaelfranca Ok, thanks. I didn't see that there's Allowed Failures section. I'll try to figure out why ConfigurableActiveSupport#test_configuration_accessors_are_not_available_on_instance and ConfigurableActiveSupport#test_configuration_is_crystalizeable are failing.

[knovoselic]

The problem was in
::Object.__send__(:include, Bar)

I've changed the test to reduce pollution, but it still needs to include Qux in Object (couldn't figure out how to reproduce the bug differently). I'd appreciate any suggestion on how to improve the test.

[rafaelfranca]

Can we have one more level of constants? Or this bug only happen if the ClassMethods is defined at ``::Object`?

[knovoselic]

If I understood it correctly, it happens only when ClassMethods is defined in one of Module's ancestors, and Module.ancestors returns:

irb(main):001:0> Module.ancestors
[
  [0] Module < Object,
  [1] Object < BasicObject,
  [2] Kernel,
  [3] BasicObject
]

[rafaelfranca]

O_O. How is this bug happening in the wild? Is someone opening one of these Modules?

[knovoselic]

This line was triggering the bug in my app:
https://github.com/mongoid/mongoid/blob/master/lib/mongoid/extensions/object.rb#L274

[rafaelfranca]

Is this extend needed? mongoid does not extend it.

[knovoselic]

No, I just need to manually call Object.extend in that case. Should I remove it and add Object.extend call?

[rafaelfranca]

Yes. Looks good

[rafaelfranca]

Could you squash your commits? I'm going to merge it.

BTW, great work

[sgrif]

👍 on great work, this was an excellent find.

[knovoselic]

Thanks :) I've squashed the commits, let me know if you need anything else.

[rafaelfranca]

Backported as d06e425