Replace occurences of alias_method_chain with their Module#prepend counterpart #19413

kirs · 2015-03-19T15:29:48Z

Earlier @fbernier proposed a cool idea of replacing alias_method_chain with Module#prepend in #17394.
I guess it's now time to merge it 🚀
/cc @rafaelfranca

fbernier · 2015-03-19T15:37:36Z

Thanks for this, looks like some tests are failing. I will be putting some more work on it.

kaspth · 2015-03-19T15:48:19Z

activesupport/lib/active_support/core_ext/marshal.rb

-    def load_with_autoloading(source)
-      load_without_autoloading(source)
+module MarshalWithAutoloading
+    def load(source)


Outdent this by two spaces

kaspth · 2015-03-19T15:55:15Z

Should we follow through with a deprecation on alias_method_chain?

kirs · 2015-03-19T22:22:54Z

Thanks for review! I've pushed some updated parts.

@kaspth it would be great to deprecate alias_method_chain, but we need first to update the JSON encoding too: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/object/json.rb#L41

I tried it and I also tried to fix the failing test, but I had no success yet. I will spend some more time on it tomorrow 💛

kaspth · 2015-03-19T22:28:09Z

Well, the question was more aimed at any Core Team members that might have happened to listen in. But yes, do look more into it 😄

Kasper

Den 19/03/2015 kl. 23.23 skrev Kir Shatrov notifications@github.com:

Thanks for review! I've pushed some updated parts.

@kaspth it would be great to deprecate alias_method_chain, but we need first to update the JSON encoding too: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/object/json.rb#L41

I tried it and I also tried to fix the failing test, but I had no success yet. I will spend some more time on it tomorrow

—
Reply to this email directly or view it on GitHub.

fbernier · 2015-03-20T11:36:39Z

The tests are failing here because of a SystemStackError. After looking into it, I highly suspect it being a ruby bug and I've found this issue which sounds like our problem.

If that's the case, this particular modification will have to wait again I guess.

yuki24 · 2015-03-20T12:00:42Z

@fbernier you are right. We can't just replace #alias_chain_method with #prepend right now due to that Ruby bug and this only works on Ruby 2.3-head since it's not been backported to 2.2-head yet.

@kirs Regarding #to_json, you can do something like this:

module ActiveSupport
  module CoreExt
    module ToJsonWithActiveSupportEncoder
      def to_json(options = nil)
        if options.is_a?(::JSON::State)
          # Called from JSON.{generate,dump}, forward it to JSON gem's to_json
          super
        else
          # to_json is being invoked directly, use ActiveSupport's encoder
          ActiveSupport::JSON.encode(self, options)
        end
       end
     end
   end
 end

[Object, Array, FalseClass, Float, Hash, Integer, NilClass, String, TrueClass, Enumerable].reverse_each do |klass|
  klass.prepend ActiveSupport::CoreExt::ToJsonWithActiveSupportEncoder
end

The key here is to use #reverse_each, not #reach.

kirs · 2015-03-20T19:36:08Z

Thanks @yuki24!

I updated all the code.
I suggest that now we deprecate alias_method_chain, migrate to Module#prepend everywhere except of Range and then patch Range as soon as the bug in MRI will be fixed.

@kaspth @rafaelfranca @fbernier what do you think?

yuki24 · 2015-03-20T20:59:51Z

activesupport/lib/active_support/core_ext/marshal.rb

-      else
-        raise exc
-      end
+module MarshalWithAutoloading


I think this module should have the ActiveSupport namespace.

rafaelfranca · 2015-03-20T21:03:30Z

I suggest that now we deprecate alias_method_chain, migrate to Module#prepend everywhere except of Range and then patch Range as soon as the bug in MRI will be fixed.

👍

rafaelfranca · 2015-03-20T21:03:52Z

But lets add the deprecation in a different PR.

@fbernier

Thanks @fbernier for suggestion! <3 At this moment we can use Module#prepend in all all cases except of Range because of the bug [1] in MRI 2.2 [1] https://bugs.ruby-lang.org/issues/10847

kirs · 2015-03-20T21:07:24Z

@rafaelfranca updated with the namespaced module. I hope it's ready for merge 🚀

rafaelfranca · 2015-03-20T21:08:37Z

activesupport/lib/active_support/core_ext/marshal.rb

-    def load_with_autoloading(source)
-      load_without_autoloading(source)
+module ActiveSupport
+  module MarshalWithAutoloading


# :nodoc:

rafaelfranca · 2015-03-20T21:09:13Z

I commented but don't need to change. I'll do.

Replace occurences of alias_method_chain with their Module#prepend counterpart

…as discussed rails#19413

…Hash(ActiveSupport::ToJsonWithActiveSupportEncoder)#to_json for Ruby 2.8.0 This pull request addresses failures at https://buildkite.com/rails/rails/builds/70219#79d96882-6c51-4854-8cab-28f50ac8bca1 According to https://bugs.ruby-lang.org/issues/16973 This is an expected change in Ruby. These failures has been addressed by changing the order of prepend as suggested. ```diff % git diff diff --git a/activesupport/test/json/encoding_test.rb b/activesupport/test/json/encoding_test.rb index 30a3b8e5a0..1328041bf7 100644 --- a/activesupport/test/json/encoding_test.rb +++ b/activesupport/test/json/encoding_test.rb @@ -186,6 +186,8 @@ def test_hash_should_pass_encoding_options_to_children_in_to_json country: "UK" } } + p person.method(:to_json) + pp person.class.ancestors json = person.to_json only: [:address, :city] assert_equal(%({"address":{"city":"London"}}), json) @@ -287,6 +289,8 @@ def test_array_to_json_should_not_keep_options_around f.bar = "world" array = [f, { "foo" => "other_foo", "test" => "other_test" }] + p array.method(:to_json) + pp array.class.ancestors assert_equal([{ "foo" => "hello", "bar" => "world" }, { "foo" => "other_foo", "test" => "other_test" }], ActiveSupport::JSON.decode(array.to_json)) end % ``` * Ruby 2.8.0 without this fix uses `Array(JSON::Ext::Generator::GeneratorMethods::Array)#to_json`, which should use `Array(ActiveSupport::ToJsonWithActiveSupportEncoder)#to_json` ``` % bin/test test/json/encoding_test.rb -n test_array_to_json_should_not_keep_options_around Run options: -n test_array_to_json_should_not_keep_options_around --seed 33311 [Array, JSON::Ext::Generator::GeneratorMethods::Array, ActiveSupport::ToJsonWithActiveSupportEncoder, Enumerable, ActiveSupport::ToJsonWithActiveSupportEncoder, Object, JSON::Ext::Generator::GeneratorMethods::Object, ActiveSupport::Tryable, Kernel, BasicObject] F Failure: TestJSONEncoding#test_array_to_json_should_not_keep_options_around [/Users/yahonda/src/github.com/rails/rails/activesupport/test/json/encoding_test.rb:294]: --- expected +++ actual @@ -1 +1 @@ -[{"foo"=>"hello", "bar"=>"world"}, {"foo"=>"other_foo", "test"=>"other_test"}] +["#<TestJSONEncoding::CustomWithOptions:0xXXXXXX>", {"foo"=>"other_foo", "test"=>"other_test"}] bin/test test/json/encoding_test.rb:286 Finished in 0.015486s, 64.5745 runs/s, 64.5745 assertions/s. 1 runs, 1 assertions, 1 failures, 0 errors, 0 skips % ``` * Ruby 2.8.0 with this fix uses `Array(ActiveSupport::ToJsonWithActiveSupportEncoder)#to_json` ``` % bin/test test/json/encoding_test.rb -n test_array_to_json_should_not_keep_options_around Run options: -n test_array_to_json_should_not_keep_options_around --seed 12193 [ActiveSupport::ToJsonWithActiveSupportEncoder, Array, JSON::Ext::Generator::GeneratorMethods::Array, ActiveSupport::ToJsonWithActiveSupportEncoder, Enumerable, ActiveSupport::ToJsonWithActiveSupportEncoder, Object, JSON::Ext::Generator::GeneratorMethods::Object, ActiveSupport::Tryable, Kernel, BasicObject] . Finished in 0.008070s, 123.9157 runs/s, 123.9157 assertions/s. 1 runs, 1 assertions, 0 failures, 0 errors, 0 skips % ``` * Ruby 2.8.0 without this fix uses `Hash(JSON::Ext::Generator::GeneratorMethods::Hash)#to_json`, which should use `Hash(ActiveSupport::ToJsonWithActiveSupportEncoder)#to_json` ``` % bin/test test/json/encoding_test.rb -n test_hash_should_pass_encoding_options_to_children_in_to_json Run options: -n test_hash_should_pass_encoding_options_to_children_in_to_json --seed 18064 [Hash, JSON::Ext::Generator::GeneratorMethods::Hash, ActiveSupport::ToJsonWithActiveSupportEncoder, Enumerable, ActiveSupport::ToJsonWithActiveSupportEncoder, Object, JSON::Ext::Generator::GeneratorMethods::Object, ActiveSupport::Tryable, Kernel, BasicObject] F Failure: TestJSONEncoding#test_hash_should_pass_encoding_options_to_children_in_to_json [/Users/yahonda/src/github.com/rails/rails/activesupport/test/json/encoding_test.rb:193]: --- expected +++ actual @@ -1 +1 @@ -"{\"address\":{\"city\":\"London\"}}" +"{\"name\":\"John\",\"address\":{\"city\":\"London\",\"country\":\"UK\"}}" bin/test test/json/encoding_test.rb:181 Finished in 0.015009s, 66.6267 runs/s, 66.6267 assertions/s. 1 runs, 1 assertions, 1 failures, 0 errors, 0 skips % ``` * Ruby 2.8.0 with this fix uses `Hash(ActiveSupport::ToJsonWithActiveSupportEncoder)#to_json` ``` % bin/test test/json/encoding_test.rb -n test_hash_should_pass_encoding_options_to_children_in_to_json Run options: -n test_hash_should_pass_encoding_options_to_children_in_to_json --seed 56794 [ActiveSupport::ToJsonWithActiveSupportEncoder, Hash, JSON::Ext::Generator::GeneratorMethods::Hash, ActiveSupport::ToJsonWithActiveSupportEncoder, Enumerable, ActiveSupport::ToJsonWithActiveSupportEncoder, Object, JSON::Ext::Generator::GeneratorMethods::Object, ActiveSupport::Tryable, Kernel, BasicObject] . Finished in 0.007434s, 134.5171 runs/s, 134.5171 assertions/s. 1 runs, 1 assertions, 0 failures, 0 errors, 0 skips % ``` Refer ruby/ruby#3181 ruby/ruby#2936 https://bugs.ruby-lang.org/issues/9573 rails#19413

kirs changed the title ~~Replace occurences of alias_method_chain with their Module#prepend countrypart~~ Replace occurences of alias_method_chain with their Module#prepend counterpart Mar 19, 2015

kaspth reviewed Mar 19, 2015
View reviewed changes

yuki24 reviewed Mar 20, 2015
View reviewed changes

rafaelfranca added the activesupport label Mar 20, 2015

Use Module#prepend instead of alias_method_chain

d5bddc1

Thanks @fbernier for suggestion! <3 At this moment we can use Module#prepend in all all cases except of Range because of the bug [1] in MRI 2.2 [1] https://bugs.ruby-lang.org/issues/10847

rafaelfranca reviewed Mar 20, 2015
View reviewed changes

rafaelfranca added a commit that referenced this pull request Mar 20, 2015

Merge pull request #19413 from kirs/replace-alias_method_chain

c35ebe1

Replace occurences of alias_method_chain with their Module#prepend counterpart

rafaelfranca merged commit c35ebe1 into rails:master Mar 20, 2015

kirs mentioned this pull request Mar 20, 2015

Deprecate alias_method_chain in favour of Module#prepend #19434

Merged

jonatack pushed a commit to jonatack/rails that referenced this pull request Mar 24, 2015

Deprecate alias_method_chain in favour of Module#prepend

cf11406

…as discussed rails#19413

eileencodes mentioned this pull request Aug 26, 2015

Update for Rails 5 rails/actionpack-xml_parser#12

Merged

connorshea mentioned this pull request Feb 25, 2016

alias_method_chain is deprecated attr-encrypted/attr_encrypted#193

Closed

yahonda mentioned this pull request Jun 22, 2020

Use ActiveSupport::ToJsonWithActiveSupportEncoder#to_json for Ruby 2.8.0 #39697

Merged

ivoanjo mentioned this pull request Jun 15, 2021

Add context inheritance via prepend instead of changing Thread TwP/logging#230

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace occurences of alias_method_chain with their Module#prepend counterpart #19413

Replace occurences of alias_method_chain with their Module#prepend counterpart #19413

kirs commented Mar 19, 2015

fbernier commented Mar 19, 2015

kaspth Mar 19, 2015

kaspth commented Mar 19, 2015

kirs commented Mar 19, 2015

kaspth commented Mar 19, 2015

fbernier commented Mar 20, 2015

yuki24 commented Mar 20, 2015

kirs commented Mar 20, 2015

yuki24 Mar 20, 2015

rafaelfranca Mar 20, 2015

rafaelfranca commented Mar 20, 2015

rafaelfranca commented Mar 20, 2015

kirs commented Mar 20, 2015

rafaelfranca Mar 20, 2015

rafaelfranca commented Mar 20, 2015

Replace occurences of alias_method_chain with their Module#prepend counterpart #19413

Replace occurences of alias_method_chain with their Module#prepend counterpart #19413

Conversation

kirs commented Mar 19, 2015

fbernier commented Mar 19, 2015

kaspth Mar 19, 2015

Choose a reason for hiding this comment

kaspth commented Mar 19, 2015

kirs commented Mar 19, 2015

kaspth commented Mar 19, 2015

fbernier commented Mar 20, 2015

yuki24 commented Mar 20, 2015

kirs commented Mar 20, 2015

yuki24 Mar 20, 2015

Choose a reason for hiding this comment

rafaelfranca Mar 20, 2015

Choose a reason for hiding this comment

rafaelfranca commented Mar 20, 2015

rafaelfranca commented Mar 20, 2015

kirs commented Mar 20, 2015

rafaelfranca Mar 20, 2015

Choose a reason for hiding this comment

rafaelfranca commented Mar 20, 2015