Show exceptions refactor: controller should be responsible for choice to show exceptions #3717

Merged
merged 4 commits into from Nov 22, 2011

Projects

None yet

2 participants

@lest
Contributor
lest commented Nov 22, 2011

Original issue #3323

I've introduced ActionController::Metal#show_detailed_exceptions?. Middleware only checks env['action_dispatch.show_detailed_exceptions']. Logic was moved to controller layer and it can be overriden.

/cc @jeremy

@josevalim josevalim commented on an outdated diff Nov 22, 2011
actionpack/lib/action_controller/metal.rb
@@ -196,10 +196,15 @@ module ActionController
@_request = request
@_env = request.env
@_env['action_controller.instance'] = self
+ @_env['action_dispatch.show_detailed_exceptions'] = show_detailed_exceptions?
@josevalim
josevalim Nov 22, 2011 Member

We shouldn't add this logic directly to metal. It should be moved to its own module.

@josevalim
josevalim Nov 22, 2011 Member

Maybe it could even be added to the existing Rescue module.

@josevalim josevalim commented on an outdated diff Nov 22, 2011
actionpack/lib/action_controller/metal.rb
process(name)
to_a
end
+ def show_detailed_exceptions?
+ defined?(Rails.application) && Rails.application.config.consider_all_requests_local || request.local?
@josevalim
josevalim Nov 22, 2011 Member

Instead of checking for Rails.application we should actually copy Rails.application.config.consider_all_request_local to config.action_controller and access it directly.

In fact, defined?(Rails.application) is always a smell. If someone wants to contribute to Rails, a good starting point would be converting all defined?(Rails.application) checks into an option.

@josevalim
josevalim Nov 22, 2011 Member

Another issue here is that we are always calculating request.local?. This is bad because request.local? triggers the ip calculation which is unnecessary in almost all requests, except when you have an exception (which should be, well, exceptional). I would suggest allowing "action_dispatch.show_detailed_exceptions" to be a proc/lambda.

@josevalim josevalim commented on the diff Nov 22, 2011
...ack/lib/action_dispatch/middleware/show_exceptions.rb
@@ -38,9 +38,8 @@ module ActionDispatch
"application's log file and/or the web server's log file to find out what " <<
"went wrong.</body></html>"]]
- def initialize(app, consider_all_requests_local = false)
@josevalim
josevalim Nov 22, 2011 Member

We would need to deprecate the second option.

@lest
lest Nov 22, 2011 Contributor

@josevalim Should we handle somehow this option? Or simply show deprecation message and ignore it?

@josevalim
josevalim Nov 22, 2011 Member

Yeah, just ignore it. Show a deprecation warnings saying it no longer works.

@josevalim
Member

Very nice pull request. I have submitted some comments regarding its implementation. Thanks!

@lest
Contributor
lest commented Nov 22, 2011

@josevalim I moved code to Rescue module.
It allows not to call show_detailed_exceptions on everty request, only if exception occures. So I think there is no more need of lambda/proc.
I removed use of Rails.application.config in favor of copying and accessing it directly.

There is only question about deprecation #3717 (comment)

@josevalim
Member

Loved the changes you made. Have you checked if railties tests still pass? You don't need to run them all, but at least the following is required:

cd railties
bundle exec rake test TEST_DIR=application

Thanks for your work!

@lest
Contributor
lest commented Nov 22, 2011

@josevalim I've run

cd railties
bundle exec rake test

And all of them passed.

@josevalim
Member

One last thing: can we have a CHANGELOG entry? :)

@lest
Contributor
lest commented Nov 22, 2011

@josevalim Ok :-) I'll add deprecation warning and changelog entry in several hours.

@lest
Contributor
lest commented Nov 22, 2011

@josevalim deprecation warning and changelog entry added

@josevalim josevalim merged commit 39ecbfd into rails:master Nov 22, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment