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

Error handlers are hard to blend in with the rest of the site #222

Closed
LaylBongers opened this issue Mar 10, 2017 · 7 comments
Closed

Error handlers are hard to blend in with the rest of the site #222

LaylBongers opened this issue Mar 10, 2017 · 7 comments
Labels
question A question (converts to discussion)

Comments

@LaylBongers
Copy link

From the docs:

Unlike request handlers, error handlers can only take 0, 1, or 2 parameters of types Request and/or Error.

This is problematic if your site includes for example a navigation bar with a signed in section. These kinds of sections in the site's layout shared between all pages still often need to touch the DB (or cached data) and cookies. Currently the error handler doesn't allow this being done easily. I feel like it would be best to treat error handlers as any other handler, except that they can also take Request and Error as parameters.

An alternative solution is to use a redirect, but that just works around the underlying issue.

@SergioBenitez
Copy link
Member

Error catchers cannot fail. They are meant to catch, not handle errors. That is, they are a sink for errors that aren't handled elsewhere. If you need custom error handling, you can use forwarding and/or Result return types to handle this.

@SergioBenitez SergioBenitez added the question A question (converts to discussion) label Mar 10, 2017
@echochamber
Copy link

@SergioBenitez I think this confusion could be avoided in the future by a docs improvement. The fact that the section on Error Catchers in the docs does not link to the section on Errors (and vice versa) is what I think lead to this confusion.

From the docs on Error Catchers

When Rocket wants to return an error page to the client, Rocket invokes the catcher for that error

Is not entirely clear on what "wants to return an error page to the client" means from this page alone (the Errors doc page clarifies this). I think a link the Errors section or a short sentence elaborating on what "wants to return an error page to the client" means would make the docs easier to understand and tie the related concepts together.

Thoughts? If 👍 I can open a PR for it later today.

@LaylBongers
Copy link
Author

I see the reasoning behind this now, but what should I do in the case of 404 errors? Forwarding feels a bit like a hack and I don't know if it's going to have an effect on SEO/accessibility/etc.

@SergioBenitez
Copy link
Member

SergioBenitez commented Mar 11, 2017

@echochamber You're totally right. The docs aren't in any repository yet, but feel free to write something up and submit it as a PR here; it'll force me to finally put the docs up somewhere.

@LaylConway Most 404 pages don't make use of anything in the request except maybe the URL. As a result, error catchers should work well there. Nonetheless, you can invoke request guards directly from the error handler if needed. See, for instance, how to invoke the State request guard. You'll need to manage errors yourself here.

But it really all depends on why/when you're getting a 404. The simplest/most correct thing to do is is to return a Result<T, status::Custom<E>>, where maybe T and E are both Template, or something else entirely. Then you can set the status code and emit whatever custom response you'd like.

@LaylBongers
Copy link
Author

Thank you for the answers, I'm leaving this open for the docs issue, feel free to close it if needed.

@SergioBenitez
Copy link
Member

Closing this out. @echochamber Feel free to open a PR when you're ready. If you want to chat about it, visit the #rocket IRC/Matrix channel.

@echochamber
Copy link

@SergioBenitez Apologies, I forgot about this over the weekend. Just remembered I offered to write some docs here. I'll have a PR for you tonight or tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question A question (converts to discussion)
Projects
None yet
Development

No branches or pull requests

3 participants