Visibility of helpers seems all wrong

Ouch. Formatting fail.

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

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.

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.

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

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?

There are other tickets regarding this:

#3945 helper :all on by default #4750 Add feature, turning off autoloading helpers

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.

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.

#3945 helper :all on by default

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?

Would you consider accepting something along the lines of #4750 Add feature, turning off autoloading helpers ?

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?

I'm working on a patch along those lines.

Jesse

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?

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.

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

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.

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

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.

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!

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!

(from [730af4896358d9125dbd7b8384d66460ae839a45]) Ensure that inherited helper_methods are available after calling clear_helpers [#5348 Visibility of helpers seems all wrong state:resolved]

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

(from [ef01f8840b31971fe53881fe41cc24a9ba05e4f1]) Ensure that inherited helper_methods are available after calling clear_helpers [#5348 Visibility of helpers seems all wrong state:resolved]

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