Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make rescue_from Page not found errors configurable #661

Closed
wants to merge 1 commit into from

Conversation

januszm
Copy link
Contributor

@januszm januszm commented Jun 25, 2015

Since Rails 4 'Exceptions raised inside exception handlers are not propagated up'.
Therefore, ActionController::RoutingError raised in the Comfy::Cms::ContentController cannot be rescued from the application controllers. We can make the call to rescue_from inside ContentController conditional (NOTICE: this workaround won't work if we simply pass if: condition as an option to rescue_from, it just can't be called at all).

Let me know your thoughts and if you agree to go this way I'll prepare some tests.

@GBH
Copy link
Member

GBH commented Jun 25, 2015

What a bizzare thing. Add a test for this and let's merge this in.

@januszm
Copy link
Contributor Author

januszm commented Jun 25, 2015

Any ideas how to refactor this? I don't want to spoil your gem so we can
always treat it as an application concern (and force devs to implement
something like an exceptions_app) or work more on CMS and:

  • remove rescue_from call from ContentController
  • refactor find page/raise errors
    25 cze 2015 16:41 "Oleg" notifications@github.com napisał(a):

What a bizzare thing. Add a test for this and let's merge this in.


Reply to this email directly or view it on GitHub
#661 (comment)
.

Since Rails 4 'Exceptions raised inside exception handlers are not propagated up'.
Therefore, ActionController::RoutingError raised in the ContentController
cannot be rescued from the application controllers.
@januszm januszm force-pushed the configurable_rescue_from_404 branch from 736c21b to 7104a33 Compare June 29, 2015 11:50
@januszm
Copy link
Contributor Author

januszm commented Jun 29, 2015

UPDATE: added integration test, updated configuration_test.

I had to modify the ContentController a bit more to make tests running (also, see the workaround in the integration test, setup and teardown). Without this workaround this new feature is not testable because once you run 'rake test' the app is loaded and the ContentController is already parsed (and rescue_from called).

Let me know if you're going to merge this. Alternatively, for someone who doesn't want to use rescue_from 404 provided by the CMS the solution is to unregister the exception handler somewhere in his app with the following in his controller:

.rescue_handlers.delete(["ActiveRecord::RecordNotFound", :page_not_found])

@januszm
Copy link
Contributor Author

januszm commented Aug 17, 2015

FYI: I ended up just using the workaround in the initializer:

+++ b/config/initializers/comfortable_mexican_sofa.rb
 #     return true
 #   end
 # end
+
+# Unset the exception handler for 404
+Comfy::Cms::ContentController.rescue_handlers.delete(["ActiveRecord::RecordNotFound", :page_not_found])

@rience
Copy link

rience commented Aug 29, 2016

@GBH can this be merged and released?

@GBH
Copy link
Member

GBH commented Nov 19, 2017

Content controller is not relying on exception handling to render 404s anymore.

@GBH GBH closed this Nov 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants