Distinguish missing controller exceptions from unrelated NameError #37834
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
casperisfine
force-pushed
the
handle-unrelated-name-error-in-router
branch
from
3f9045e
to
3cee27b
Compare
rafaelfranca
reviewed
Dec 4, 2019
| @@ -529,7 +529,7 @@ GEM | |||
| websocket-extensions (0.1.4) | |||
| xpath (3.2.0) | |||
| nokogiri (~> 1.8) | |||
| zeitwerk (2.2.0) | |||
| zeitwerk (2.2.2) | |||
There was a problem hiding this comment.
Should we change the minimum version in the gemspec?
There was a problem hiding this comment.
Good point. I'll do it.
casperisfine
force-pushed
the
handle-unrelated-name-error-in-router
branch
from
3cee27b
to
11f5fb8
Compare
casperisfine
force-pushed
the
handle-unrelated-name-error-in-router
branch
from
11f5fb8
to
6d0285a
Compare
Fix: rails#37650 The classic autoloader used to totally unregister any constant that failed midway. Which mean `"SomeConst".constantize` was idempotent. However Zeitwerk rely on normal `Kernel#require` behavior, which mean that if an exception is raised during a class/module definition, it will be left incompletely defined. For instance: ```ruby class FooController ::DoesNotExist def index end end ``` Will leave `FooController` defined, but without its `index` method. Because of this, when silencing a NameError, it's important to make sure the missing constant is really the one we were trying to load.
casperisfine
force-pushed
the
handle-unrelated-name-error-in-router
branch
from
6d0285a
to
54878cd
Compare
eugeneius
pushed a commit
to eugeneius/rails
that referenced
this pull request
Dec 21, 2019
…or-in-router Distinguish missing controller exceptions from unrelated NameError
This was referenced Dec 21, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix: #37650
The classic autoloader used to totally unregister any constant that
failed midway. Which mean
"SomeConst".constantizewas idempotent.However Zeitwerk rely on normal
Kernel#requirebehavior, which meanthat if an exception is raised during a class/module definition,
it will be left incompletely defined. For instance:
Will leave
FooControllerdefined, but without itsindexmethod.Because of this, when silencing a NameError, it's important
to make sure the missing constant is really the one we were trying
to load.
This patch
I tried writing a test case for it in
railties/test/application/middleware/exceptions_test.rbbut without success. If you have an idea of a proper way to test this case, I'm all ears.Also this patch won't fully work without fxn/zeitwerk#99, because
Zeitwerk::NameError#nameisn't set as ofzeitwerk 2.2.1.@rafaelfranca @fxn thoughts?
cc @etiennebarrie @Edouard-chin