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

Added support for redirect #88

Closed
wants to merge 4 commits into from
Closed

Conversation

pinscript
Copy link
Contributor

As requested in #84 (I also wanted it for the P-R-G pattern). I think this is the most flexible solution.

We could offer a Redirect(location string) which defaults to 302 and a RedirectCode(location string, code int). What do you think? I think people should be aware of what status code they are returning so the implemented solution is enough for me.

@manucorporat
Copy link
Contributor

Sorry, I was in holidays.

I like it, but I would like to avoid the possibility to use:

c.Redirect("http://something", 200)

should we panic with non-3xx codes?

@pinscript
Copy link
Contributor Author

Sure. According to RFC2616 (http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html), the following status codes can send a Location header: 201, 300, 301, 302, 303, 305, 307, 406.

Should we allow only theese status codes?

I think a panic would be best.

@manucorporat
Copy link
Contributor

I guess so, since it's a high level API (called Redirect) we should not allow codes not specified by the standard.

@pinscript
Copy link
Contributor Author

Added range checks for status code. I probably misinterpreted RFC2616, no major browser seem to follow locations for 201 and 406 codes.

After giving it some thought, I think we should go with the Redirect(code, location) signature. This is in line with the other render-methods (JSON, XML, etc).

@manucorporat
Copy link
Contributor

Good job! Merged in develop: ceec1b6

Thank you so much ;)

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.

None yet

2 participants