Permalink
Browse files

Use class_eval or instance_eval when triggering lazy load hooks:

- When lazy load hooks were triggered we were using
  `Object.instance_eval` which evaluates the block in the context of
  the class being passed. Most of the time that class was a
  `Class`. If one wants to define a instance method on the class then
  it wasn't possible.

  ```ruby
    class A; end;
    A.instance_eval do
      def foo
        puts 'bar'
      end
    end
    A.new.foo #> NoMethodError: undefined method `foo`
    A.foo #> bar
  ```
- This PR checks what object is passed when triggering the hooks and
  either call `class_eval` or `instance_eval`. My rational and assumptions being
  that if an instance of a class is passed, then the blocks needs to
  evaluate in the context of that instance (i.e. defining a method
  should only define it on that instance).
  On the other hand, if a Class or Module is passed when triggering
  hooks, then defining a method should define it on the class itself
- #32776 Pushed me to introduce this change
  • Loading branch information...
Edouard-chin committed Jul 4, 2018
1 parent 0a6b866 commit 6cf7a0b0e9eaaa57fba0b0cea0f3015664baa0d8
Showing with 54 additions and 1 deletion.
  1. +5 −1 activesupport/lib/active_support/lazy_load_hooks.rb
  2. +49 −0 activesupport/test/lazy_load_hooks_test.rb
@@ -68,7 +68,11 @@ def execute_hook(name, base, options, block)
if options[:yield]
block.call(base)
else
base.instance_eval(&block)
if base.is_a?(Class) || base.is_a?(Module)
base.class_eval(&block)
else
base.instance_eval(&block)
end
end
end
end
@@ -1,6 +1,7 @@
# frozen_string_literal: true
require "abstract_unit"
require "active_support/core_ext/module/remove_method"
class LazyLoadHooksTest < ActiveSupport::TestCase
def test_basic_hook
@@ -125,6 +126,54 @@ def test_hook_with_yield_true_afterward
assert_equal 7, i
end
def test_hook_uses_class_eval_when_base_is_a_class
ActiveSupport.on_load(:uses_class_eval) do
def first_wrestler
"John Cena"
end
end
ActiveSupport.run_load_hooks(:uses_class_eval, FakeContext)
assert_equal "John Cena", FakeContext.new(0).first_wrestler
ensure
FakeContext.remove_possible_method(:first_wrestler)
end
def test_hook_uses_class_eval_when_base_is_a_module
mod = Module.new
ActiveSupport.on_load(:uses_class_eval2) do
def last_wrestler
"Dwayne Johnson"
end
end
ActiveSupport.run_load_hooks(:uses_class_eval2, mod)
klass = Class.new do
include mod
end
assert_equal "Dwayne Johnson", klass.new.last_wrestler
end
def test_hook_uses_instance_eval_when_base_is_an_instance
ActiveSupport.on_load(:uses_instance_eval) do
def second_wrestler
"Hulk Hogan"
end
end
context = FakeContext.new(1)
ActiveSupport.run_load_hooks(:uses_instance_eval, context)
assert_raises NoMethodError do
FakeContext.new(2).second_wrestler
end
assert_raises NoMethodError do
FakeContext.second_wrestler
end
assert_equal "Hulk Hogan", context.second_wrestler
end
private
def incr_amt

0 comments on commit 6cf7a0b

Please sign in to comment.