Why does this code do all that dance? underscore, camelize, constantize, and require_dependency all at once? Could we simplify this to just constantize mod_name in both cases? Is raising a specific exception on LoadError really important besides being strictly backwards compat?

@fxn I kinda agree with you, but wasn't bold enough to go against that very specific comment stating the autoload exceptions should be wrapped.

I'll dig a bit more into this, to see if I can write it in a way that works with both autoloaders without an if check.

@fxn @rafaelfranca I think I found a way to stop using require_dependency in all cases.

Awesome, I'll have a look soon!

Quick comment from my phone. If a dependency of an app uses Zeitwerk, the constant is defined regardless of the app choice.

@fxn yes, but at the very least it's not defined when running action pack test suite.

Maybe that doesn't properly reflect reality? After some quick testing I think it does:

>> require 'action_pack'
=> true
>> defined? Zeitwerk
=> nil

👋 @fxn if you're still very busy it's fine, but I just wanted to make sure this didn't fall through the cracks 😉

@casperisfine thanks for the heads-up! I'll move it up.

I have read the patch and discussion, and I'd like to explore some small changes. I'll write back.

This is what I have in mind (draft, untested):

diff --git a/actionpack/lib/abstract_controller/helpers.rb b/actionpack/lib/abstract_controller/helpers.rb
index abb09456e0..ca66588bae 100644
--- a/actionpack/lib/abstract_controller/helpers.rb
+++ b/actionpack/lib/abstract_controller/helpers.rb
@@ -11,17 +11,20 @@ module Helpers
       class_attribute :_helper_methods, default: Array.new
     end
 
-    class MissingHelperError < LoadError
-      def initialize(error, path)
-        @error = error
-        @path  = "helpers/#{path}.rb"
+    class MissingHelperError < StandardError
+      def initialize(error, module_name)
+        @module_name = module_name
         set_backtrace error.backtrace
+      end
 
-        if /^#{path}(\.rb)?$/.match?(error.path)
-          super("Missing helper file helpers/%s.rb" % path)
-        else
-          raise error
-        end
+      def message
+        <<-MSG.strip_heredoc
+          Helper #{@module_name} not found. Please check:
+
+            * Does app/helpers/#{@module_name.underscore}.rb exist?
+
+            * If it does, does it define a module called #{@module_name}?
+        MSG
       end
     end
 
@@ -148,24 +151,17 @@ def clear_helpers
       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
+          when Symbol, String
+            arg = arg.to_s
+            module_name = arg >= "a" ? "#{arg}_helper".camelize : arg
             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
+              module_name.constantize
+            rescue NameError => error
+              if error.missing_name?(module_name)
+                raise MissingHelperError.new(error, module_name)
+              end
+              raise
           when Module
             arg
           else

Rationale:

• Then natural way to autoload things in an autoloading context is by referencing constants.

• So, we refactor in a way that is not based on file names, but on constant paths.

• We no longer raise LoadError, but we can still try our best at giving an exception message that talks about helper files.

Note that the code inflects with camelize, and autoloaders could inflect some of those helper files using different logic. But that gotcha was already present, no need to address it in this work.

What do you think?

(EDIT: I removed a recursive call for symbols, because I forgot the argument has to be an array and you get an array back. The code was uglier than a to_s call on Symbol, String.)

What do you think?

I wasn't sure what I thought, so I went ahead and made tests pass on your diff as f2b939790b837b53ec9af804d512cfca9c9b4ef0

• It's definitely makes modules_for_helpers much much cleaner
• The big downside is that both classic and zeitwerk need the proper AS::Inflectors.acronym defined, because the lookup is done by String#camelize. So this will definitely break some apps.

So overall I think Étienne's suggestion of conditionally rescuing Zeitwerk::NameError while a bit brittle is more pragmatic.

On one hand, I believe we have to depart from working with files as much as possible. For example, take this existing method:

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"
rescue NameError => e
  raise e unless e.missing_name? "#{module_name}Helper"
end

In my view, nowadays would be better written as (drafted inline, untested):

def default_helper_module!
  helper name.sub(/Controller$/, "Helper")
  # Error handling.
end

We could even encourage usage of constant names or module objects as helper arguments.

But I guess we have to continue supporting lowercase arguments, at least as long as include_all_helpers and helper :all exist, which go to the file system.

Right now, Action Pack inflects with camelize, and with the introduction of Zeitwerk that is no longer correct, because you need an inflector to behave like the user expects.

Quick idea I have not validated: What if we try to address this one using absolute paths in the appropriate spots while we are in the transition phase with both autoloaders? The inflector issue is still there (it was already there), but perhaps that would be an easier way to make this code compatible with add_autoload_paths_to_load_path now.

@fxn I rebased and update the PR using @etiennebarrie 's idea. The test suite is now green.

If you still think the code isn't good, I can try your absolute path idea, but I'm not super kin on it because as I understand it, it means doing a O(n) search into the helper load paths.

ACK!

Hey! I'll work on this one today or tomorrow.

I think this is all too complicated and I believe MissingHelperError tries to accomplish something that is not well-defined.

For starters, helpers do not need to be defined in app/helpers. That is true technically, but it is even documented explicitly:

The second form illustrates ... or other cases where the file containing the helper definition is not in one of Rails' standard load paths

so the error message appending helpers is dubious to me. You do not really expect the file to be defined in any particular place.

Then, we have more stuff to cover:

• Currently, the existing code in master catches LoadError and assumes the helper is missing if raised. That may or may not be the case, because any code can raise LoadError.
• Zeitwerk::NameError can similarly be triggered because of some other missing constant.
• We need an inflector to underscore, but in reality there is no reason to underscore if the argument is camel case.

We could perhaps refine introducing branches that check the error messages, but I don't like going that way. I would like to explore an approach in which we depart from MissingHelperError, simplify all this code, and mostly reraise whatever happens. At most, logging something that may be useful for beginners, who may forget the file name suffix or something if that is possible.

Let me explore that a little bit.

I would like to explore an approach in which we depart from MissingHelperError, simplify all this code, and mostly reraise whatever happens.

Yeah, I didn't try that because I though it would be some kind of API change, but if that's an acceptable change then yeah that would simply this code tremendously.

Just a heads up.

I have been working on this today, but need to understand the scope of the public API for file names to see how far we can push an implementation based on constantize. 100% backwards compatibility is not a goal per se if the trade-off is worthwhile and breakage is edge.

As an extreme use case, the test suite does this:

ActionController::Base.helpers_path = File.expand_path("../fixtures/helpers", __dir__)

and several controllers override helpers_path as they please.

@casperisfine do you have a monkey patch in Shopify to be able to disable config.add_autoload_paths_to_load_path? or does this issue block you to be able to?

do you have a monkey patch in Shopify to be able to disable config.add_autoload_paths_to_load_path?

I don't but I could. I'll probably add one in a few days to validates that this is indeed the last blocker.

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

I don't see a way to change helper to accomplish this in a way that is 100% backwards compatible.

The current implementation does not even require helpers to be in autoload paths, you can point helpers to arbitrary directories, as the test suite does. As long as require_dependency is able to resolve the file, you're good (and usage of require_dependency is documented in the API).

But I believe that implementation is not aligned with the direction Rails has taken, and in particular with config.add_autoload_paths_to_load_path. So I am considering breaking backwards compatibility in corner cases.

The solution I have in mind is something like this (untested):

def modules_for_helpers(args)
  args.flatten.map! do |arg|
    case arg
    when Module
      arg
    when String, Symbol
      arg = arg.to_s
      arg = arg.camelize unless arg.match?(/\A[A-Z]/)
      arg << "Helper"
      arg.constantize
    else
      raise ArgumentError, "helper must be a String, Symbol, or Module"
    end
  end
end

That camelizes instead of underscore. It bases loading the helpers on autoloading, which is the way to load code I'd like to push for. That is, you think in terms of constant paths, rather than file names, and you program as if all application classes and modules were in memory. That is the mindset.

In particular, if helper gets passed "FooBar" as argument, there is no manipulation to be done except for appendinng "Helper". That is the motivation for this rewrite too:

def default_helper_module!
  module_name = name.sub(/Controller\z/, "Helper")
  helper(module_name)
rescue LoadError => e
  raise e unless e.is_missing?("helpers/#{module_name.underscore}")
rescue NameError => e
  raise e unless e.missing_name?(module_name)
end

It would be even more aligned to accept only module objects and blocks as argumentsand deprecate the rest, but helper :all needs to camelize anyway, so I have abandoned that idea.

The cases in which this is backwards incompatible are going to be edge, I believe. We could do by documenting in the CHANGELOG and upgrade guide.

Meanwhile, I wonder if Rails could manually add helper folders to $LOAD_PATH if the flag is disabled.

The solution I have in mind is something like this

That's pretty much the monkey patch we're using to be able to set config.add_autoload_paths_to_load_path = false.

The cases in which this is backwards incompatible are going to be edge, I believe.

I agree.

It's in master, thanks as always for the patches and discussion @casperisfine. Please let me know if there's annything to refine once tested in your code base.

My pleasure. I'm currently updating Rails in our app to include this commit. I'll let you know if there's something wrong. I'm currently battling with an extremely weird error, but have no idea if it's a legit problem or if there's something weird in our code base.

But maybe you might have an idea, I reduced it down to:

$ bin/rails r 'p InventoryImport.name; p InventoryImport::MailerHelper'
"InventoryImport"
uninitialized constant #<Class:0x00007fe02a9f3190>::MailerHelper

So somehow InventoryImport exist, is an ActiveRecord model and has a proper name.

However when calling const_get on it, the NameError exception doesn't properly include its name.

Anyway, I'll keep digging, but it's super weird.

Ok, so I really don't get why the name isn't properly included in the message, but regardless I think this condition should be relaxed because the exception doesn't always include the strict constant name, example:

[11] pry(main)> Shop::DoesNotExist
NameError: uninitialized constant Shop (call 'Shop.connection' to establish a connection)::DoesNotExist

However when calling const_get on it, the NameError exception doesn't properly include its name.

Something that comes to mind is that we ignore overridden name methods. Don't know if that is the issue here, but would square.

I think I found the root cause. That NameError doesn't come from MRI, but from AS::Dependencies.load_missing_constant.

I'll try to figure a fix for that root cause.

Ok, so I have good reasons to believe that it's a problem specific to our app because we're not properly unhooking AS::Dependencies.

I'll keep digging, but sorry for the scare :)

Cool :).

Actually I'm able to reproduce it in a fairly vanilla Rails 6 app.

=> Shipit::Stack (call 'Shipit::Stack.connection' to establish a connection)
>> Shipit::Stack.const_get('FooHelper')
Traceback (most recent call last):
        2: from (irb):3
        1: from (irb):3:in `const_get'
NameError (uninitialized constant #<Class:0x00007ff96822ef20>::FooHelper)

What made me believe we didn't unhook AS::Dep properly was this minimal repro script:

require 'rails'
ActiveSupport::Dependencies.unhook! if ENV['UNHOOK']

class Foo
  class << self
    def name
      "#{super}(Lolilol)"
    end
  end
end

"Foo::Bar".constantize

$ ruby /tmp/name_error.rb 
Traceback (most recent call last):
	10: from /tmp/name_error.rb:12:in `<main>'
	 9: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/core_ext/string/inflections.rb:68:in `constantize'
	 8: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:280:in `constantize'
	 7: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:280:in `inject'
	 6: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:280:in `each'
	 5: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:284:in `block in constantize'
	 4: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:284:in `const_get'
	 3: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/dependencies.rb:214:in `const_missing'
	 2: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/dependencies.rb:529:in `load_missing_constant'
	 1: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/dependencies.rb:429:in `qualified_const_defined?'
/Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/dependencies.rb:429:in `const_defined?': wrong constant name Foo(Lolilol) (NameError)
$ UNHOOK=1 ruby /tmp/name_error.rb 
Traceback (most recent call last):
	6: from /tmp/name_error.rb:12:in `<main>'
	5: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/core_ext/string/inflections.rb:68:in `constantize'
	4: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:280:in `constantize'
	3: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:280:in `inject'
	2: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:280:in `each'
	1: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:284:in `block in constantize'
/Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:284:in `const_get': uninitialized constant Foo::Bar (NameError)

But it doesn't seem accurate.

Ok, I found a better repro script. It's not name that is redefined, but inspect:

require 'rails'
ActiveSupport::Dependencies.unhook! if ENV['UNHOOK']

class Foo
  class << self
    def inspect
      "#{super}(Lolilol)"
    end
  end
end

"Foo::Bar".constantize

byroot@jaures:ruby:(master)$ ruby /tmp/name_error.rb 
Traceback (most recent call last):
	6: from /tmp/name_error.rb:12:in `<main>'
	5: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/core_ext/string/inflections.rb:68:in `constantize'
	4: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:280:in `constantize'
	3: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:280:in `inject'
	2: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:280:in `each'
	1: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:284:in `block in constantize'
/Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:284:in `const_get': uninitialized constant Foo::Bar (NameError)
byroot@jaures:ruby:(master)$ UNHOOK=1 ruby /tmp/name_error.rb 
Traceback (most recent call last):
	6: from /tmp/name_error.rb:12:in `<main>'
	5: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/core_ext/string/inflections.rb:68:in `constantize'
	4: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:280:in `constantize'
	3: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:280:in `inject'
	2: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:280:in `each'
	1: from /Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:284:in `block in constantize'
/Users/byroot/.gem/ruby/2.7.1/gems/activesupport-6.0.2.2/lib/active_support/inflector/methods.rb:284:in `const_get': uninitialized constant Foo(Lolilol)::Bar (NameError)

This time it fails when AS::Dep is unhooked, just like what I experience.

I can also reduce it further without loading AS at all:

byroot@jaures:ruby:(master)$ ruby /tmp/name_error.rb 
Traceback (most recent call last):
/tmp/name_error.rb:12:in `<main>': uninitialized constant Foo(Lolilol)::Bar (NameError)

So it seems like AS::Dep was actually giving better error message, and missing_name? relies on it.

Interesting, looks like that is what Ruby does:

class Foo
  class << self
    def inspect
      "Yo"
    end
  end
end

Foo::X # => foo.rb:9:in `<main>': uninitialized constant Yo::X (NameError)

Yeah, I checked MRI's source and it's what it does. Might be worth a ticket upstream, because that's crap.

Anyway, I don't think it's much of a problem because NameError has #name and #receiver so we can get the real name much more accurately than by using a regexp (I need to check when it was added though, might be relatively recent).

I'm working on a PR, I should be opening it shortly, I'll be tagging you.