This project is archived and is in readonly mode.

#5348 ✓resolved
Jesse Storimer

Visibility of helpers seems all wrong

Reported by Jesse Storimer | August 10th, 2010 @ 06:44 PM | in 3.0.2

In upgrading an app to Rails 3 and noticed that the visibility of helpers seemed to be messed up. The gist below demonstrates how to reproduce the issue.

http://gist.github.com/517669

I used the looksee gem to inspect the lookup path and, indeed, inside products/index.html.erb SignupHelper appears before ProductsHelper.
looksee output: http://gist.github.com/517640

Comments and changes to this ticket

  • Jesse Storimer

    Jesse Storimer August 10th, 2010 @ 06:46 PM

    Ouch. Formatting fail.

    Here's a gist formatted properly: http://gist.github.com/517669

  • Henrik Hodne

    Henrik Hodne August 11th, 2010 @ 01:35 AM

    • Tag set to rails 3.0.rc, actionpack, verified

    I can confirm this.

  • Jesse Storimer

    Jesse Storimer August 18th, 2010 @ 02:14 PM

    From the example I posted above this might seem like a 'quirk', but the real problem arises when there is a situation like this:

    module ProductsHelper
      def title(a)
        ..
      end
    end
    
    module SignupHelper
      def title(a)
        ..
      end
    end
    

    Any calls to the #title helper from products/* templates will silently fail because they are actually calling the #title defined in SignupHelper. This might be more easily illustrated like so:

    module ProductsHelper
      def title(a)
        ..
      end
    end
    
    moudle SignupHelper
      def title(a,b)
        ..
      end
    end
    

    In this case, any calls to #title(a) from products/* templates will raise an ArgumentError, again because it's actually calling SignupHelper's #title(a,b).

    This issue is causing our app to break in subtle ways on Rails 3. Until it's fixed we cannot upgrade.

  • Jesse Storimer

    Jesse Storimer August 19th, 2010 @ 03:27 AM

    • Tag changed from rails 3.0.rc, actionpack, verified to rails 3.0.rc, actionpack, patch, verified

    I was able to find the issue and add a regression test.

    In 37e4deb2 there was a change such that any subclass of ActionController::Base would automatically get helper :all. As shown in my previous comments this can easily break the use of helpers.

    I'm not sure if this is the most elegant solution to the problem, I simply reverted the problematic change I found in 37e4deb2. Either way this gets the test passing and also fixes the issue for my app. Please review so we can get this committed.

  • Jesse Storimer

    Jesse Storimer August 24th, 2010 @ 01:54 PM

    I tried removing the call to helper :all in ActionController::Base and all the tests are still passing, plus I think the code is clearer without that line.

    Here's a new, simpler, patch.

  • José Valim

    José Valim August 24th, 2010 @ 03:55 PM

    • Milestone cleared.
    • State changed from “new” to “open”
    • Assigned user set to “José Valim”
    • Importance changed from “” to “High”

    Hey Jesse,

    helper :all should not be called in ActionController::Base, because, at the point ActionController::Base is loaded, the directory were all the helpers are defined is not specified yet. So action_pack tests will pass, but tests in railties will probably fail.

    However, a good and simple fix is to change the following method:

    http://github.com/rails/rails/blob/master/actionpack/lib/action_con...

    And make it return the helper with the same name as the current controller first. What do you think? :D

  • Evgeniy Dolzhenko

    Evgeniy Dolzhenko August 24th, 2010 @ 07:03 PM

    +1 how that would work with helpers inherited from parent controller?

  • Jesse Storimer

    Jesse Storimer August 24th, 2010 @ 07:07 PM

    José,

    I'd rather not have Rails call helper :all for all my controllers. That doesn't seem like a decision that the framework should make for me. AFAIK this wasn't the default behaviour in 2.3.x. Is there a reason why it was added for 3.0?

    I've attached a patch with the changes I'd like to see. It removes the call to helper :all from ActionController::Base. I verified that all of the ActionPack tests are passing. All of the railties tests are also passing but I had to add a helper declaration to one of the test controllers so it would pick up a helper that doesn't match the default controller/helper naming scheme.

    Thoughts?

  • Mateo Murphy

    Mateo Murphy August 24th, 2010 @ 07:15 PM

    There are other tickets regarding this:

    #3945 #4750

  • José Valim

    José Valim August 24th, 2010 @ 07:19 PM

    I don't believe helper :all will be removed at this point, just before the final version. It was a decision made quite some time ago already.

  • Jesse Storimer

    Jesse Storimer August 24th, 2010 @ 07:19 PM

    I think Matt Schafer put it best:

    Mixing in all helpers into every renderer pretty much defeats the purpose of having per-controller helpers. With the current behavior, you may as well just put everything in ApplicationHelper.

    https://rails.lighthouseapp.com/projects/8994/tickets/3945#ticket-3...

  • José Valim

    José Valim August 24th, 2010 @ 07:38 PM

    Sorry, but this behavior won't be changed at this point. Is anyone willing to supply a patch with the solution proposed above or another solution that does not require helper :all to be removed?

  • Jesse Storimer
  • José Valim

    José Valim August 24th, 2010 @ 09:08 PM

    Jesse, I will try to discuss it with people at core, since I was not the one who added helper :all. Meanwhile, could you provide a solution that works as I said above?

  • Jesse Storimer

    Jesse Storimer August 24th, 2010 @ 09:30 PM

    I'm working on a patch along those lines.

    Jesse

  • Jesse Storimer

    Jesse Storimer August 26th, 2010 @ 04:13 AM

    As discussed offline the approach proposed turned out to be non-trivial and affected performance negatively. Attached is patch that takes a slightly different approach, allowing apps to do:

    class ApplicationController < ActionController::Base
      default_helpers_only!
    end
    

    With the introduction of the directive above we basically have the reverse of what we had in Rails 2.x. Rather than optionally including all helpers we are optionally disabling the inclusion of all helpers.

    The naming here isn't finalized, any suggestions?

    I realize that José said the behaviour won't change at this point, but, if it was open for debate, I'd still vote for a reversal of this behaviour to the way it was in Rails 2.x :)

    Firstly, due to the current implementation of helpers in Rails 3 there is no way to prevent all helpers from being included when a controller is initialized. This means that when you instantiate the example controller above all helper modules will be included, then subsequently discarded when this new directive is called.

    Secondly, the 2.x behaviour seems more in line with Rails' new slogan of 'Have it your way'. Forcing my controllers to load all helpers isn't my way. If I wanted to share a helper between two controllers I would put it in ApplicationController.

    Thirdly, it's simpler to include helpers after the fact than it is to uninclude them. The previous implementation was simpler and with less overhead. Start with the default helpers but include them all if you want to, rather than, start with all helpers included but toss them if you want to.

    OK, my gripings aside, any thoughts?

  • José Valim

    José Valim August 26th, 2010 @ 08:08 PM

    • State changed from “open” to “resolved”

    I just pushed clear_helpers that unloads all helpers, except the ones with the same name as your controller.

  • Jesse Storimer

    Jesse Storimer August 26th, 2010 @ 08:42 PM

    If my example below wasn't the intended use case for that patch then you can safely ignore the rest of this :)

    I'm not sure if this was the intended use case, but that patch doesn't allow for something like:

    class ApplicationController < ActionController::Base
      clear_helpers
    end
    
    class ProductsController < ApplicationController
      # this controller also needs clear_helpers in order to avoid helper :all
    end
    

    In the above case ApplicationController will only have ApplicationHelper. But when ProductsController inherits from ActionController::Base it will have helper :all applied.

    I attached a failing test case that demonstrates the case above.

  • José Valim

    José Valim August 26th, 2010 @ 10:21 PM

    Thanks for the failing tests. This issue has been fixed as well.

  • Jesse Storimer

    Jesse Storimer August 27th, 2010 @ 03:34 AM

    I found another issue with this implementation. Some of the modules that get included in ActionController::Base define helpers using the helper_method directive. eg. ActionController::Flash, ActionController::ResquestForgeryProtection both do this.

    If I then make the call to clear_helpers in my ApplicationController these base helpers disappear (and forgery protection no longer works).

    I attached a patch that deals with this.

  • Jesse Storimer

    Jesse Storimer August 27th, 2010 @ 12:17 PM

    Bah, I think my solution above was too naive. It didn't take into account that libraries might be mixing helper_methods into ActionController::Base. Using clear_helpers would clear those away too.

    I'm working on a test case for this. This is getting messy...

  • Jesse Storimer

    Jesse Storimer August 27th, 2010 @ 02:46 PM

    So I spoke too soon :P The patch I offered does cover the case above.

    Though I still have a feeling that this may come back to be a problem in the future. Discarding the ancestry tree of helpers seems like its bound to cause missing helpers at some point. Though it sounds like most people won't be using this clear_helpers feature.

    Regardless I added a test to my previous patch to cover this case.

  • José Valim

    José Valim August 28th, 2010 @ 02:46 PM

    Hey Jesse!

    Good solution to the problem, but the implementation could be a bit better. What you need to do is to define a class_attribute called :_helper_methods and every time you call helper_methods() you should append to this array.

    This way when you call clear_helpers, you will be able to iterate on this _helper_methods array and just define the proper ones.

    Can you work on this one ASAP so it can be included in the final release tomorrow? Thanks for your work mate!

  • Jesse Storimer

    Jesse Storimer August 28th, 2010 @ 03:52 PM

    Hey José,

    Thanks for the suggestion. Calling #ancestors directly felt pretty sketchy :P

    Here's a new patch using class_attribute :_helper_methods. I tried this out with my app and saw lots of .'s :)

    Good luck on the final release!

  • Repository

    Repository August 28th, 2010 @ 10:08 PM

    (from [730af4896358d9125dbd7b8384d66460ae839a45]) Ensure that inherited helper_methods are available after calling clear_helpers [#5348 state:resolved]

    Signed-off-by: José Valim jose.valim@gmail.com
    http://github.com/rails/rails/commit/730af4896358d9125dbd7b8384d664...

  • Repository

    Repository August 28th, 2010 @ 10:09 PM

    (from [ef01f8840b31971fe53881fe41cc24a9ba05e4f1]) Ensure that inherited helper_methods are available after calling clear_helpers [#5348 state:resolved]

    Signed-off-by: José Valim jose.valim@gmail.com
    http://github.com/rails/rails/commit/ef01f8840b31971fe53881fe41cc24...

  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:02 PM

    • Milestone set to 3.0.2

Create your profile

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

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages