logankoester / errship

Errship is a Rails 3.1 engine for rendering error pages inside your layout. It supports i18n, custom exceptions, and Airbrake error tracking.
http://blog.logankoester.com/errship.html
MIT License
26 stars 5 forks source link

Http status is always 200 #11

Closed marioizquierdo closed 12 years ago

marioizquierdo commented 12 years ago

The rescuers are not using the right http status, for example:

def render_404_error(exception = nil, errship_scope = false)
  render :template => '/errship/standard', :locals => {
    :status_code => 404, :errship_scope => errship_scope }
end

will return HTTP status 200. I think it should be:

def render_404_error(exception = nil, errship_scope = false)
  render :template => '/errship/standard', :locals => {
    :status_code => 404, :errship_scope => errship_scope }, :status => 404
end

This is confusing me in some specs of my application; the next example should be ok and it fails like this:

description "When looking for an unexisting record" do
  before { get :show, :id => "nonsense" }
  it "should return a 404 status" do
    it { should respond_with :not_found }
  end
end

#=> Failure/Error: should respond with not found
#=> Expected response to be a <:not_found>, but was <200>

The only way to check (in rspec) if the error page was raised is with:

it { should render_template 'errship/standard' }

But this is not very semantic, and it's coupled to the errship implementation.

Was it intentional? Otherwise I can supply a pull request.

Thanks, Mario Izquierdo.

czarneckid commented 12 years ago

I think this is a bug. Pull request away :)

alexfrydl commented 12 years ago

This is actually not a bug. Our nginx configs have proxy_intercept_errors on, so if you return status 404 nginx will ignore what you send and render whatever is set to error_page 404. So the only way for errship to control it is to return status 200. It's not semantic, no, but since it's just an error page for users and not for REST APIs, it's not a big deal.

marioizquierdo commented 12 years ago

Hmm, its not beautiful but probably does not make sense to complicate things more. Having that in mind, I think the only thing we need is improve the documentation.

I will add a pull request with some comments on it.

Thanks.

On Tue, Nov 1, 2011 at 5:08 PM, Brennan Frydl < reply@reply.github.com>wrote:

This is actually not a bug. Our nginx configs have proxy_intercept_errors on, so if you return status 404 nginx will ignore what you send and render whatever is set to error_page 404. So the only way for errship to control it is to return status 200. It's not semantic, no, but since it's just an error page for users and not for REST APIs, it's not a big deal.

Reply to this email directly or view it on GitHub: https://github.com/agoragames/errship/issues/11#issuecomment-2595947

logankoester commented 12 years ago

Thanks Brennan - I knew we'd done it that way for a reason, but I couldn't recall what it was.

czarneckid commented 12 years ago

Agree with Mario that we should just make a note in our documentation to reflect our reasoning.

fschwahn commented 12 years ago

Maybe this should be configurable? I'd prefer it if the error pages return the correct http code. The default behavior could stay the same. I can supply a patch if you think it makes sense.

alexfrydl commented 12 years ago

For our internal use we don't need it, but we'd accept a pull request or patch for it.

logankoester commented 12 years ago

I have merged @fschwahn's pull request and released as Errship 2.2.0. See README for details.