Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• You flagged this item as spam.
  

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

  Ouch. Formatting fail.

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

• You flagged this item as spam.
  

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

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

  I can confirm this.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

  • helper-visibility.diff 1.4 KB

• You flagged this item as spam.
  

  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.

  • dont-load-all-helpers.diff 1.4 KB
• 

  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

• You flagged this item as spam.
  

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

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

• You flagged this item as spam.
  

  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?

  • no-helper-all.diff 2 KB

• You flagged this item as spam.
  

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

  There are other tickets regarding this:

  #3945 #4750

• 

  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.

• You flagged this item as spam.
  

  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 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?

• You flagged this item as spam.
  

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

  Would you consider accepting something along the lines of https://rails.lighthouseapp.com/projects/8994/tickets/4750 ?

• 

  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?

• You flagged this item as spam.
  

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

  I'm working on a patch along those lines.

  Jesse

• You flagged this item as spam.
  

  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?

  • default-helpers-only.diff 4.2 KB
• 

  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.

• You flagged this item as spam.
  

  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.

  • failing-helper-test.diff 2.2 KB
• 

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

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

• You flagged this item as spam.
  

  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.

  • base-helper-methods.diff 1.9 KB

• You flagged this item as spam.
  

  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...

• You flagged this item as spam.
  

  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.

  • base-helper-methods-with-lib-test.diff 2.4 KB
• 

  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!

• You flagged this item as spam.
  

  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!

  • inherited-helper-methods-class-attribute.diff 2.9 KB
• 

  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 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 October 15th, 2010 @ 11:02 PM

  • Milestone set to “3.0.2”

  [bulk edit]