Include TagHelper but don't explicitly require it

[sferik]

Allow autoloading to work as intended and avoid multiple requires.

[jeremy]

Looks like the concern and sanitize helper requires are needed. Why capture and autoload?

[rwz]

Awesome pull request. +1

[sferik]

@jeremy Without require 'active_support/dependencies/autoload', I get the error NameError: uninitialized constant ActiveSupport::Autoload upon require 'action_view/helpers/sanitize_helper'. Perhaps the autoload dependency belongs in the sanitize helper?

[sferik]

Okay, it should be ready to pull. I can squash these into a single commit, if you'd prefer.

[pixeltrix]

The NameError for ActiveSupport::Autoload is actually coming from html-scanner, however I'm not sure that we support requiring of individual helpers since they are all marked as autoload in lib/action_view/helpers.rb and then immediately included. If it is meant to be supported then there's more places where they fail to load so we should explicitly test them.

/cc @josevalim

[josevalim]

We should support manual requires BUT in order for all manual requires to work in Rails, you MUST require the parent library. So be sure you required "action_view" first. In this case, you should probably require "action_view/helpers" as well. Those files are requiring shared AS dependencies and setting up autoload only. If a require is still missing after requiring those two files, a Pull Request is welcome.

[pixeltrix]

If we're requiring 'action_view' then that's all that is needed since everything is autoloaded, e.g:

require 'action_view'

class MyClass
  include ActionView::Helpers::TextHelper

  def my_method
    simple_format("Your text here")
  end
end

If this is the case we probably don't want explicit requires for other helpers within a helper as it may prevent proper autoloading if they get required before 'action_view/helpers'

[josevalim]

Exactly. It could even lead to double requires.

[sferik]

Do I really need to require all of action_view just to use a single action_view/helpers/text_helper? Ideally, every file would stand on its own, so it could be required and tested independently.

[josevalim]

requiring "action_view" doesn't actually load anything from action view. it just sets up autoloads and requires common dependencies.

https://github.com/rails/rails/blob/master/actionpack/lib/action_view.rb

Yes, you need to do that. On all frameworks (active_model, action_controller, etc).

[sferik]

I still get errors when I require 'action_view' before I require 'action_view/helpers/text_helper'.

[josevalim]

Yeah, so that's a bug. :) Reopened.

[sferik]

@josevalim Okay, so what needs to be changed in this pull request before it can be merged?

[pixeltrix]

Don't require 'action_view/helpers/text_helper' just include ActionView::Helpers::TextHelper in your class/module, otherwise you're circumventing the autoload of the ActionView::Helpers constant. This constant includes all of the helpers but you still should be able to include individual helpers so you don't have to mix in everything. The reason for all these autoloads is so that they can be eager loaded when running in threadsafe mode since require is inherently unsafe when using threads.

In this case there is a bug for TextHelper - it uses the tag method so it should include TagHelper and not require it like it does at the moment. Also the SanitizeHelper requires the TagHelper but neither includes it or uses it.

[sferik]

@pixeltrix If I've interpreted what you've said correctly, my pull request now implements your suggestion. However, this does not fix my original problem:

irb(main):001:0> require 'action_view'
=> true
irb(main):002:0> require 'action_view/helpers/text_helper'
NameError: uninitialized constant ActionView::Helpers::FormTagHelper::TextHelper
    from /Users/erik/Projects/rails/actionpack/lib/action_view/helpers/form_tag_helper.rb:18:in `<module:FormTagHelper>'
    from /Users/erik/Projects/rails/actionpack/lib/action_view/helpers/form_tag_helper.rb:14:in `<module:Helpers>'
    from /Users/erik/Projects/rails/actionpack/lib/action_view/helpers/form_tag_helper.rb:8:in `<module:ActionView>'
    from /Users/erik/Projects/rails/actionpack/lib/action_view/helpers/form_tag_helper.rb:6:in `<top (required)>'
    from /Users/erik/Projects/rails/actionpack/lib/action_view/helpers/form_helper.rb:4:in `require'
    from /Users/erik/Projects/rails/actionpack/lib/action_view/helpers/form_helper.rb:4:in `<top (required)>'
    from /Users/erik/Projects/rails/actionpack/lib/action_view/helpers/active_model_helper.rb:1:in `require'
    from /Users/erik/Projects/rails/actionpack/lib/action_view/helpers/active_model_helper.rb:1:in `<top (required)>'
    from /Users/erik/Projects/rails/actionpack/lib/action_view/helpers.rb:37:in `<module:Helpers>'
    from /Users/erik/Projects/rails/actionpack/lib/action_view/helpers.rb:4:in `<module:ActionView>'
    from /Users/erik/Projects/rails/actionpack/lib/action_view/helpers.rb:3:in `<top (required)>'
    from /Users/erik/Projects/rails/actionpack/lib/action_view/helpers/text_helper.rb:6:in `<module:ActionView>'
    from /Users/erik/Projects/rails/actionpack/lib/action_view/helpers/text_helper.rb:4:in `<top (required)>'
    from (irb):2:in `require'
    from (irb):2
    from /Users/erik/.rbenv/versions/1.9.2-p290/bin/irb:12:in `<main>'

[sferik]

@jeremy @josevalim @pixeltrix I've given you all commit access to my fork of the rails repo, in case you want to push to the add_explicit_requires branch on sferik/rails

[stve]

I think what @pixeltrix meant was that you don't need to require the text_helper explicitly since it'll auto-load when you require action_view:

1.9.2p290 :001 > require 'action_view'
 => true 
1.9.2p290 :002 > class Foo; include ActionView::Helpers::TextHelper; end
 => Foo 
1.9.2p290 :003 > Foo.new.strip_tags('<a href="#bar">ohai</a>')
 => "ohai"

[pixeltrix]

@sferik don't require, use include:

>> require 'action_view'
=> true
>> include ActionView::Helpers::TagHelper
=> Object
>> include ActionView::Helpers::TextHelper
=> Object
>> simple_format("Hello World")
=> "<p>Hello World</p>"

[sferik]

Gotcha. Is this badboy ready to merge or should we cleanup all the explicit requires of Action View helpers?

[josevalim]

It is fine to merge imho, but could you please squash? <3

[sferik]

Squashed!

[pixeltrix]

@josevalim is it worthwhile to migrate the individual helper tests from ActionView::TestCase to ActiveSupport::TestCase so that we can test this? ActionView::TestCase includes ActionView::Helpers so problems like this one aren't detected.

[josevalim]

We could do this move if it will basically be a class name change. If it requires rewriting many of the tests, I am -1.

[pixeltrix]

Certainly the intent would be to not touch the actual tests, just change the class and add any necessary setup.