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

Padrino halt with message error #686

Closed
hiddenhippo opened this Issue Sep 28, 2011 · 4 comments

Comments

Projects
None yet
4 participants
@hiddenhippo

hiddenhippo commented Sep 28, 2011

a.k.a codebyday

Noticed the following issue:

In sinatra:
halt 404, "go away"
sends http code 404 and the string "go away" to the client

In padrino
halt 404, "go away"
sends http code 200 to the client!
however:
halt 404
sends http code 404 to the client

@ghost ghost assigned joshbuddy Sep 28, 2011

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 28, 2011

Member

@DAddYE @joshbuddy @skade What do you guys think?

This test seems to confirm it:

  should "support halting with 404 and message" do
    mock_app do
      controller do
        get :index do
          halt 404, "not found"
        end
      end
    end

    get "/"
    assert_equal "not found", body
    assert_equal 404, status
  end

test_0072_support_halting_with_404_and_message(Routing)
Expected: 404
Actual: 200

As mentioned halt 404 with no message makes it pass and halt 401, "go away" also works as expected (returns 401 status). Failing test added here:

c9c6b8d

Member

nesquena commented Sep 28, 2011

@DAddYE @joshbuddy @skade What do you guys think?

This test seems to confirm it:

  should "support halting with 404 and message" do
    mock_app do
      controller do
        get :index do
          halt 404, "not found"
        end
      end
    end

    get "/"
    assert_equal "not found", body
    assert_equal 404, status
  end

test_0072_support_halting_with_404_and_message(Routing)
Expected: 404
Actual: 200

As mentioned halt 404 with no message makes it pass and halt 401, "go away" also works as expected (returns 401 status). Failing test added here:

c9c6b8d

@joshbuddy

This comment has been minimized.

Show comment
Hide comment
@joshbuddy

joshbuddy Sep 28, 2011

Contributor

Thank you! I'll take a look. I'm trying to re-vamp all this routing shit right now to be much closer to sinatra .. and less monkey patching.

Contributor

joshbuddy commented Sep 28, 2011

Thank you! I'll take a look. I'm trying to re-vamp all this routing shit right now to be much closer to sinatra .. and less monkey patching.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 28, 2011

Member

Thanks Josh, looking forward to the next version of http_router.

Member

nesquena commented Sep 28, 2011

Thanks Josh, looking forward to the next version of http_router.

@DAddYE DAddYE closed this in f67eefd Oct 2, 2011

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Oct 2, 2011

Member

Here the correct fix: 0fec162

Member

DAddYE commented Oct 2, 2011

Here the correct fix: 0fec162

danishkhan pushed a commit to danishkhan/padrino-framework that referenced this issue Oct 28, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment