Remove require_dependency usage in helper [Closes #37632]

Motivation is twofold:

  * We are gradually removing `require_dependency` from the framework.

  * Let `helper` work if `config.add_autoload_paths_to_load_path` is
    disabled.

Co-authored-by: Jean Boussier <jean.boussier@gmail.com>

actionpack/CHANGELOG.md

@@ -1,3 +1,8 @@
+*   The +helper+ class method for controllers loads helper modules specified as
+    strings/symbols with `String#constantize` instead of `require_dependency`.
+
+    *Xavier Noria*, *Jean Boussier*
+
 *   Correctly identify the entire localhost IPv4 range as trusted proxy.
 
     *Nick Soracco*

actionpack/lib/abstract_controller/helpers.rb

@@ -74,39 +74,56 @@ module AbstractController
         end
       end
 
-      # The +helper+ class method can take a series of helper module names, a block, or both.
+      # Includes the given modules in the template class.
       #
-      # ==== Options
-      # * <tt>*args</tt> - Module, Symbol, String
-      # * <tt>block</tt> - A block defining helper methods
+      # Modules can be specified in different ways. All of the following calls
+      # include +FooHelper+:
       #
-      # When the argument is a module it will be included directly in the template class.
-      #   helper FooHelper # => includes FooHelper
+      #   # Module, recommended.
+      #   helper FooHelper
       #
-      # When the argument is a string or symbol, the method will provide the "_helper" suffix, require the file
-      # and include the module in the template class. The second form illustrates how to include custom helpers
-      # when working with namespaced controllers, or other cases where the file containing the helper definition is not
-      # in one of Rails' standard load paths:
-      #   helper :foo             # => requires 'foo_helper' and includes FooHelper
-      #   helper 'resources/foo'  # => requires 'resources/foo_helper' and includes Resources::FooHelper
+      #   # String/symbol without the "helper" suffix, camel or snake case.
+      #   helper "Foo"
+      #   helper :Foo
+      #   helper "foo"
+      #   helper :foo
       #
-      # Additionally, the +helper+ class method can receive and evaluate a block, making the methods defined available
-      # to the template.
+      # The last two assume that <tt>"foo".camelize</tt> returns "Foo".
       #
-      #   # One line
-      #   helper { def hello() "Hello, world!" end }
+      # When strings or symbols are passed, the method finds the actual module
+      # object using +String#constantize+. Therefore, if the module has not been
+      # yet loaded, it has to be autoloadable, which is normally the case.
+      #
+      # Namespaces are supported. The following calls include +Foo::BarHelper+:
+      #
+      #   # Module, recommended.
+      #   helper Foo::BarHelper
+      #
+      #   # String/symbol without the "helper" suffix, camel or snake case.
+      #   helper "Foo::Bar"
+      #   helper :"Foo::Bar"
+      #   helper "foo/bar"
+      #   helper :"foo/bar"
+      #
+      # The last two assume that <tt>"foo/bar".camelize</tt> returns "Foo::Bar".
+      #
+      # The method accepts a block too. If present, the block is evaluated in
+      # the context of the controller helper module. This simple call makes the
+      # +wadus+ method available in templates of the enclosing controller:
       #
-      #   # Multi-line
       #   helper do
-      #     def foo(bar)
-      #       "#{bar} is the very best"
+      #     def wadus
+      #       "wadus"
       #     end
       #   end
       #
-      # Finally, all the above styles can be mixed together, and the +helper+ method can be invoked with a mix of
-      # +symbols+, +strings+, +modules+ and blocks.
+      # Furthermore, all the above styles can be mixed together:
       #
-      #   helper(:three, BlindHelper) { def mice() 'mice' end }
+      #   helper FooHelper, "woo", "bar/baz" do
+      #     def wadus
+      #       "wadus"
+      #     end
+      #   end
       #
       def helper(*args, &block)
         modules_for_helpers(args).each do |mod|
@@ -127,46 +144,17 @@ module AbstractController
         default_helper_module! unless anonymous?
       end
 
-      # Returns a list of modules, normalized from the acceptable kinds of
-      # helpers with the following behavior:
-      #
-      # String or Symbol:: :FooBar or "FooBar" becomes "foo_bar_helper",
-      # and "foo_bar_helper.rb" is loaded using require_dependency.
-      #
-      # Module:: No further processing
-      #
-      # After loading the appropriate files, the corresponding modules
-      # are returned.
-      #
-      # ==== Parameters
-      # * <tt>args</tt> - An array of helpers
-      #
-      # ==== Returns
-      # * <tt>Array</tt> - A normalized list of modules for the list of
-      #   helpers provided.
-      def modules_for_helpers(args)
-        args.flatten.map! do |arg|
-          case arg
-          when String, Symbol
-            file_name = "#{arg.to_s.underscore}_helper"
-            begin
-              require_dependency(file_name)
-            rescue LoadError => e
-              raise AbstractController::Helpers::MissingHelperError.new(e, file_name)
-            end
-
-            mod_name = file_name.camelize
-            begin
-              mod_name.constantize
-            rescue LoadError
-              # dependencies.rb gives a similar error message but its wording is
-              # not as clear because it mentions autoloading. To the user all it
-              # matters is that a helper module couldn't be loaded, autoloading
-              # is an internal mechanism that should not leak.
-              raise NameError, "Couldn't find #{mod_name}, expected it to be defined in helpers/#{file_name}.rb"
-            end
+      # Given an array of values like the ones accepted by +helper+, this method
+      # returns an array with the corresponding modules, in the same order.
+      def modules_for_helpers(modules_or_helper_prefixes)
+        modules_or_helper_prefixes.flatten.map! do |module_or_helper_prefix|
+          case module_or_helper_prefix
           when Module
-            arg
+            module_or_helper_prefix
+          when String, Symbol
+            helper_prefix = module_or_helper_prefix.to_s
+            helper_prefix = helper_prefix.camelize unless helper_prefix.start_with?(/[A-Z]/)
+            "#{helper_prefix}Helper".constantize
           else
             raise ArgumentError, "helper must be a String, Symbol, or Module"
           end
@@ -186,13 +174,10 @@ module AbstractController
         end
 
         def default_helper_module!
-          module_name = name.sub(/Controller$/, "")
-          module_path = module_name.underscore
-          helper module_path
-        rescue LoadError => e
-          raise e unless e.is_missing? "helpers/#{module_path}_helper"
+          helper_prefix = name.delete_suffix("Controller")
+          helper(helper_prefix)
         rescue NameError => e
-          raise e unless e.missing_name? "#{module_name}Helper"
+          raise unless e.missing_name?("#{helper_prefix}Helper")
         end
     end
   end

actionpack/test/abstract_unit.rb

@@ -1,8 +1,6 @@
 # frozen_string_literal: true
 
 $:.unshift File.expand_path("lib", __dir__)
-$:.unshift File.expand_path("fixtures/helpers", __dir__)
-$:.unshift File.expand_path("fixtures/alternate_helpers", __dir__)
 
 require "active_support/core_ext/kernel/reporting"
 
@@ -35,6 +33,19 @@ module Rails
   end
 end
 
+module ActionPackTesSuitetUtils
+  def self.require_helpers(helpers_dirs)
+    Array(helpers_dirs).each do |helpers_dir|
+      Dir.glob("#{helpers_dir}/**/*_helper.rb") do |helper_file|
+        require helper_file
+      end
+    end
+  end
+end
+
+ActionPackTesSuitetUtils.require_helpers("#{__dir__}/fixtures/helpers")
+ActionPackTesSuitetUtils.require_helpers("#{__dir__}/fixtures/alternate_helpers")
+
 ActiveSupport::Dependencies.hook!
 
 Thread.abort_on_exception = true

actionpack/test/controller/helper_test.rb

@@ -52,9 +52,10 @@ class HelpersPathsController < ActionController::Base
   paths = ["helpers2_pack", "helpers1_pack"].map do |path|
     File.join(File.expand_path("../fixtures", __dir__), path)
   end
-  $:.unshift(*paths)
 
   self.helpers_path = paths
+  ActionPackTesSuitetUtils.require_helpers(helpers_path)
+
   helper :all
 
   def index
@@ -63,9 +64,8 @@ class HelpersPathsController < ActionController::Base
 end
 
 class HelpersTypoController < ActionController::Base
-  path = File.expand_path("../fixtures/helpers_typo", __dir__)
-  $:.unshift(path)
-  self.helpers_path = path
+  self.helpers_path = File.expand_path("../fixtures/helpers_typo", __dir__)
+  ActionPackTesSuitetUtils.require_helpers(helpers_path)
 end
 
 module LocalAbcHelper
@@ -85,18 +85,10 @@ class HelperPathsTest < ActiveSupport::TestCase
 end
 
 class HelpersTypoControllerTest < ActiveSupport::TestCase
-  def setup
-    @autoload_paths = ActiveSupport::Dependencies.autoload_paths
-    ActiveSupport::Dependencies.autoload_paths = Array(HelpersTypoController.helpers_path)
-  end
-
   def test_helper_typo_error_message
     e = assert_raise(NameError) { HelpersTypoController.helper "admin/users" }
-    assert_equal "Couldn't find Admin::UsersHelper, expected it to be defined in helpers/admin/users_helper.rb", e.message
-  end
-
-  def teardown
-    ActiveSupport::Dependencies.autoload_paths = @autoload_paths
+    # This message is better if autoloading.
+    assert_equal "uninitialized constant Admin::UsersHelper", e.message
   end
 end
 
@@ -204,6 +196,7 @@ class HelperTest < ActiveSupport::TestCase
 
   def test_all_helpers_with_alternate_helper_dir
     @controller_class.helpers_path = File.expand_path("../fixtures/alternate_helpers", __dir__)
+    ActionPackTesSuitetUtils.require_helpers(@controller_class.helpers_path)
 
     # Reload helpers
     @controller_class._helpers = Module.new

guides/source/upgrading_ruby_on_rails.md

@@ -147,6 +147,33 @@ Example:
   end
 ```
 
+### The `helper` class method in controllers uses `String#constantize`
+
+Conceptually, before Rails 6.1
+
+```ruby
+helper "foo/bar"
+```
+
+resulted in
+
+```ruby
+require_dependency "foo/bar_helper"
+module_name = "foo/bar_helper".camelize
+module_name.constantize
+```
+
+Now it does this instead:
+
+```ruby
+prefix = "foo/bar".camelize
+"#{prefix}Helper".constantize
+```
+
+This change is backwards compatible for the majority of applications, in which case you do not need to do anything.
+
+Technically, however, controllers could configure `helpers_path` to point to a directoy in `$LOAD_PATH` that was not in the autoload paths. That use case is no longer supported out of the box. If the helper module is not autoloadable, the application is responsible for loading it before calling `helper`.
+
 Upgrading from Rails 5.2 to Rails 6.0
 -------------------------------------