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

Inherit from action controller base #1

Conversation

gsmendoza
Copy link
Contributor

Hello, appreciate if you can accept our pull request. It

  • Removes Gemfile.lock from the gem.
  • Lets ErrorPagesEngine inherit from ActionController::Base. This way, it doesn't call the filters and layout of ApplicationController.

@lazylester
Copy link
Owner

Thanks for the PR.

I think you're right... Gemfile.lock shouldn't be in the repo.

Not sure what the reason is for schema.rb... it's empty. Perhaps you can clarify?

But finally, regarding inheriting from ApplicationController, surely this is the whole point? We want our error pages to inherit the style of our application, vs. the ugly native Rails error pages.
Bear in mind I didn't do anything original here... just put the ideas that many others have documented into an engine, for testability and reusability. All the other examples I looked at are inheriting from ApplicationController. I can see that you would NOT want to inherit filters, but you WOULD want the application layouts, no?

@gsmendoza
Copy link
Contributor Author

About the schema.rb, I remember that it got generated when I ran rake db:migrate while testing the dummy app. Looks like there's no need to create a database to test the gem, so I'll remove it :)

About ApplicationController: I'm thinking that if an application layout throws an exception, that would mess up the 500 error page. That's why I thought it would be best to isolate the error pages from any external code that may cause them to fail.

I got the idea to inherit from ActionController::Base from sheerun/rails4-bootstrap@5c2df5a.

If a developer wants to reuse their application layout, he can write a error_pages_engine/errors layout and let it render the application layout.

Any other possible options?

@lazylester
Copy link
Owner

George,
The whole purpose of this engine is to inherit the layout of the site. If you just want a custom error page, you can replace the 404.html and other error page files in the public directory.
If you actually have a problem in the layout, as you have suggested, then there seems to be a bigger problem with your test suite. This is not within the scope of the engine approach of this gem.
I can see that you might not want to inherit filters from ApplicationController, so perhaps the appropriate change should be one that disables any filters. If you'd like to propose such a change, I'll look at it.
Les

@lazylester lazylester closed this Mar 17, 2014
@gsmendoza
Copy link
Contributor Author

No worries, Les. I'll see if we can just override error pages engine
controller on our end. Thanks!

On Tue, Mar 18, 2014 at 4:05 AM, lazylester notifications@github.comwrote:

Closed #1 #1.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1
.

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.

2 participants