This project is archived and is in readonly mode.

#285 ✓ wontfix
Tammo Freese

alias_method_chain limits extensibility

Reported by Tammo Freese | May 31st, 2008 @ 11:22 AM

There are two problems with the current implementaton of alias_method_chain.

1) If a method defined in a superclass is aliased, replacing the method in the superclass has no effect:

class A

def foo; "foo" end

end

class B < A

def foo_with_bar; foo_without_bar + " with bar" end

alias_method_chain :foo, :bar

end

class A

def foo; "new foo" end

end

B.new.foo # => "foo with bar"

I would have expected "new foo with bar"

2) While ...without... methods can be replaced, replacing ...with... methods has no effect:

u = User.find(:first)

u.save # => true

class ActiveRecord::Base

def save_with_validation

raise

end

end

u.save # => true

I would have expected an exception.

I have attached a patch that fixes both problems.

Comments and changes to this ticket

  • Pratik

    Pratik July 11th, 2008 @ 10:57 PM

    • State changed from “new” to “wontfix”
    • Tag set to activesupport, alias_method_chain, patch

    alias_method_chain is just supposed to be a shortcut for two alias_method statements. Usecase like yours, deserves a solution of it's own.

    Thanks.

  • Tammo Freese

    Tammo Freese July 11th, 2008 @ 11:40 PM

    In your opinion, is it OK that you cannot extend/override any of

    rails's ...with... methods?

  • Steve Purcell

    Steve Purcell July 29th, 2008 @ 02:11 PM

    This seems to be a useful patch that addresses a real problem/limitation, so I'm +1. Plus, it's small and tested; Pratik, maybe you can reconsider?

  • Pratik

    Pratik July 29th, 2008 @ 02:15 PM

    • Assigned user set to “Joshua Peek”
  • DHH

    DHH September 4th, 2008 @ 04:13 PM

    • State changed from “wontfix” to “open”

    target_method_exists can use instance_method_defined?(false) instead.

  • Joshua Peek

    Joshua Peek September 7th, 2008 @ 04:12 PM

    • Assigned user changed from “Joshua Peek” to “DHH”

    -1 I don't like the idea of replacing the alias with a delegate method call.

    @David I think that only cleans up the "target_method_exists = (instance_methods + private_instance_methods).include?(RUBY_VERSION < '1.9' ? with_method : with_method.to_sym)" busy.

  • DHH

    DHH September 7th, 2008 @ 07:22 PM

    The reason for this is to allow someone to overwrite part of the chain to inject their own behavior. Which I think is very reasonable. If you have another way of achieving the same result, let's discuss that.

    Tammo, perhaps you can post the full example somewhere so Josh can see the use case? I didn't really care for this much until I saw the use case either.

  • Tammo Freese

    Tammo Freese September 7th, 2008 @ 09:50 PM

    Using alias_method instead of a delegate method call makes overwriting/ extending with methods a major pain.

    Let's say you would like to disable benchmarking, but keep the logger for the rest of the application (or you would like to implement your own benchmarking).

    The code

    class ActionController::Base def perform_action_with_benchmark(*args, &block)

      perform_action_without_benchmark(*args, &block)
    
    

    end end

    looks right, but does not work, since overwriting with methods has no effect at all.

    It is possible to workaround that, but the resulting code

    a) looks like an implementation error, and b) depends on the exact order of the chaining in rails:

    class ActionController::Base def perform_action_without_rescue(*args, &block)

      perform_action_without_benchmark(*args, &block)
    
    

    end end

  • Joshua Peek

    Joshua Peek September 7th, 2008 @ 11:14 PM

    The use case seems reasonable. I wonder if aliasing it back works?

    
    alias_method :perform_action, :perform_action_without_benchmark
    

    I also think those (ActionController) mixins should be done without alias_method_chain. Another ticket for another day.

  • Tammo Freese

    Tammo Freese September 8th, 2008 @ 08:06 AM

    Hello Joshua,

    aliasing back will not work correctly.

    alias_method :perform_action, :perform_action_without_benchmark

    will disable benchmarking, but it will disable a lot more than that, since it throws out every aspect added to perform_action after benchmarking.

    In Rails 2.1, this would mean that - rescue_from would not work anymore, - action caching would not work anymore, - every plugin extension to perform_action would be lost.

  • Joshua Peek

    Joshua Peek September 8th, 2008 @ 03:49 PM

    Is swapping out ActionController benchmark your real goal, or was it just an example? If it is, I'd like to see how we can do this w/o alias_method_chain.

  • Tammo Freese

    Tammo Freese September 9th, 2008 @ 11:15 AM

    We found the problem with …with… methods when implementing a custom benchmarking.

    However, in my opinion, every …with… method in rails is a potential extension point for an application or a plugin, and should therefore be extensible.

  • charly71

    charly71 September 21st, 2008 @ 11:01 PM

    • Tag changed from activesupport, alias_method_chain, patch to activesupport, alias_method_chain, patch

    Hi,

    I started a few blog posts on http://ruby.simapse.com about the decorator pattern and alias_method_chain. You might want to have a look.

    Anyhow a suggested approach would be to move (almost?) all instance methods of ActiveRecord::Base into the module ActiveRecord::Core, and put the later on top of all decorating modules (Core makes sens since it's the one to be deCOREated).

    Their would be no aliasing, the method_without_feature is replaced by super and voila, that's it.

    I gave it a try on 'save' and all the tests (cases/base) passed.

    With/without methods are gone there's just one and only save (seems to me less messy when you have many decorators). If i want to save without transaction, i override save in :

    module AR::Transaction def save; super; end end

    Simple, easy, consistant and i don't think you loose any convinience alias_method_chain brings you, and it should break anything (or not much). I still have to dig in though.

    Attached is my patch with save.

    If you think it's worth the try i can go on and refactor it for every method exposed in AR::Base on a github fork.

    cheers

  • Tammo Freese

    Tammo Freese September 25th, 2008 @ 04:12 PM

    Hello charly71,

    I see three problems with this approach:

    1) To decorate one of the save methods in the modules (extending it, not overriding it) you would still need to resort to alias_method_chain or something similar.

    2) You lose the possiblity to call the with/without methods directly on the objects. (I can't tell for sure whether this is needed or not.)

    3) You can only extend inside the module itself, but not inside classes or subclasses. As a result, you cannot restrict an extension to a subtree of the inheritance hierarchy, but it's always all or nothing. As an example, you cannot easily disable benchmarking for a single subclass of ActionController::Base, but only globally.

  • charly71

    charly71 September 25th, 2008 @ 05:40 PM

    Hi Tammo,

    yes i got immediatly aware of no 3) after posting and felt stupid.

    regarding no 2) the problem is when you call save_without_transaction, you're probably calling save_with_validation and save_with_dirty, and it is a bit misleading (you're not calling the pure, immaculate save...). Even though it is probably what you want, it doesn't give you more modularity (cherrypicking) than having a chain of 'super' called.

    I do agree though that if you want to override (for just one model down the inheritance chain) your benchmarking or transaction or whatever you'll have to resolve to some magic (which still needs to be boiled in the pot) BUT..... only in your plugin :

    module MyValidation skip :save, :in=>Validation def save "blah"; super; "blah" end end

    Yet i'm speculating because i haven't resolved the magic piece ('super' is giving me some headaches, hum any ruby ninja around ?)

  • charly71

    charly71 September 26th, 2008 @ 03:43 PM

    Hi again,

    i hope i'm not polluting this ticket, but since Joshua said mixins in ActionController should be done without alias_method_chain...

    Anyway the attached code says it all and adresses a lot of issues.

    The uninclude file was taken from here :

    http://github.com/yrashk/rbmodex...>

    It is then easy to skip a module on the fly unincluding and reincluding. I'm not sure whats the drawbacks or limitations but it certainly opens up the possibilities.

  • sliu

    sliu October 7th, 2008 @ 03:47 AM

    alias_method_chain makes easy AOP, while we need it more manageable, like "insert new feature to specified position of the chain", "rewrite/remove/move specified feature of the chain", "call pure function", etc. thus, chain should be exposed and can be operated, and features in it should be manageable. all of them are not only behaviours to be execute, but also data structures can be operated.

    big project?

    I think it makes sense. just a suggestion to core team to consider.

    I even like it be in ruby grammar(I prefer ruby includes raw event-driven support, annotation support...) -- brainstorm :)

  • sliu

    sliu November 17th, 2008 @ 07:09 AM

    finally, I find class Hook in merb's extlib fit my needs.

    much appreciate to rails team who bring and maintain the amazing rails framework.

    I have a big dream: keep open mind to merge rails and merb community together.

    rails and merb are now more like struts and webwork, they finally merged their projects and communities.

    I think whether the dream can be realized primarily depends on not tech, not history. IT IS ALL ABOUT PEOPLE WHO OWE THEM.

  • Joshua Peek

    Joshua Peek January 19th, 2009 @ 07:14 PM

    • State changed from “open” to “wontfix”

    Yehuda is out to kill alias_method_chain. Expect it to be gone in 3.0 ;)

  • Gaspard Bucher

    Gaspard Bucher February 6th, 2009 @ 08:57 PM

    Good to know Yehuda is on this. This method chaining has always been hard to adapt or debug.

    I am very curious to know what he comes up with !

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Tickets have moved to Github

The new ticket tracker is available at https://github.com/rails/rails/issues

Shared Ticket Bins

Pages