Raise exception if nested file will never be found #19537
Conversation
| @@ -6,23 +6,19 @@ def require_nested(name) | |||
| filename = "#{self}::#{name}".underscore | |||
| filename = name.to_s.underscore if self == Object | |||
| if Rails.application.config.cache_classes | |||
| raise LoadError, "No such file to load -- #{filename}" unless ActiveSupport::Dependencies.search_for_file(filename) | |||
There was a problem hiding this comment.
This is the main part of the PR.
There was a problem hiding this comment.
do we want to do this in development and test only?
to not incur the hit to the disk?
Or do we consider the fstat calls across the load path minor?
There was a problem hiding this comment.
I think that's the current issue @kbrock, right now we're setting cache_classes = true in test and prod which is causing this issue to not be found in test, but simultaneously I would think we want test to be as close to prod as possible in this regard.
FWIW the direct call to require_dependency in the else branch will fail in the case addressed by this PR.
There was a problem hiding this comment.
@carbonin aah, so this check will never be run in development. thanks (not what I wanted, but more clear)
Fryguy
force-pushed
the
require_nested_fix
branch
from
6ba46ce
to
5f968fc
Compare
|
Had to rebase due to #19538 |
Fryguy
force-pushed
the
require_nested_fix
branch
from
5f968fc
to
f7c9bca
Compare
It is possible to give autoload an invalid path, and that issue will never be found until the associated constant is vivified. This causes issues where people forget to remove the require_nested calls when they remove the files. Now, it's not possible to forget, because an exception will be raised as soon as the parent file is loaded.
Fryguy
force-pushed
the
require_nested_fix
branch
from
f7c9bca
to
2a116ec
Compare
|
Checked commits Fryguy/manageiq@01d3dbc~...2a116ec with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 lib/extensions/require_nested.rb
|
It is possible to give autoload an invalid path, and that issue will
never be found until the associated constant is vivified. This causes
issues where people forget to remove the require_nested calls when they
remove the files. Now, it's not possible to forget, because an
exception will be raised as soon as the parent file is loaded.
This PR is built upon some commits to clarify the code and remove dead code, which came up as part of my research on how this method works.
See also #5270 , where the commented code was added. Basically, Rails has trouble with matching nested and top-level const names, and the code in question gets around that by loading both the nested and top-level at the same time.
@carbonin Please review.