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

Generate a bare 500 response when errors:display is false. This commi… #1089

Closed
wants to merge 2 commits into from
Closed

Generate a bare 500 response when errors:display is false. This commi… #1089

wants to merge 2 commits into from

Conversation

Perlkonig
Copy link
Contributor

…t does not remove the Resources folder at this time, though they could conceivably be deleted if this approach was accepted.

Related to Issue #1064

…t does not remove the Resources folder at this time, though they could conceivably be deleted if this approach was accepted.
@Perlkonig
Copy link
Contributor Author

Another option is to leverage the Errors plugin in the handle() function, but I didn't have time right this second to test how that might work. I'll try tomorrow. Ideally it would check for a customized 500 page in the Errors plugin and return a bare Handler::QUIT otherwise.

@Perlkonig
Copy link
Contributor Author

Perlkonig commented Oct 3, 2016

The latest commit cleans up the Resources folder and minimizes the SimplePageHandler.php file as much as works. My admittedly basic tests seemed to work fine.

  • The principle here is that there is no reason for Grav to waste resources on this. If system.errors.display is truly set to false, then don't do anything but emit an empty 500.
  • Most (all?) modern web servers already offer a server-level option for customizing error pages.
  • I tried a couple ways of just getting rid of SimplePageHandler.php entirely, but couldn't find one. I'm not at all familiar with Whoops, so that didn't help 😄
  • I tried numerous methods for leveraging the Errors plugin or some other mechanism to allow in-Grav customization, but I couldn't come up with something clean. Again, if system.errors.display is false, it's probably best to keep it simple and just return the 500. Let the server handle it.

Anyway, for your consideration. Thanks for your time!

@rhukster
Copy link
Member

rhukster commented Oct 3, 2016

I think this change would cause a lot of upset people. The custom error page is not just for 500 errors, it's for anything that Grav would throw an error at. I think most webserver provided 500 errors are very ugly, and don't provide any information, causing lots of extra detective work. So i'm really not interested in simply removing that.

If you can find a way to add toggle an optional "let webserver handle" errors, then sure I'll consider that, but not simply removing functionality that is there already. Changes need to be upgrade-safe and not change fundamental behavior. They can add, even modify (to a degree) current behavior, but not simply remove it.

@rhukster rhukster closed this Oct 3, 2016
@Perlkonig
Copy link
Contributor Author

Perlkonig commented Oct 4, 2016

But I think the point of setting system.errors.display to false is exactly to not "provide any information." The site owner can always set the display to true and get a full stack trace.

Can you provide some more information about what sorts of errors this generic page is catching? All I've ever seen is 500s. If there are some other scenarios, it would help to understand them to create an ideal solution.

If you're open a new system setting, then I can do that. Ideally I'd turn system.errors.display to a three-option switch, but that's not backwards compatible. It just feels hackish to have a system.errors.display.no_really setting 😄

@Perlkonig Perlkonig deleted the feature/bare500 branch October 4, 2016 00:19
@flaviocopes
Copy link
Contributor

Agree, we cannot remove such functionality, which I consider useful.
An alternative way would be letting the theme provide a custom error page. Or have the ability to change the strings that provide the error message (which now is hardcoded to english - that way one could remove the error message if desired).

@Perlkonig
Copy link
Contributor Author

I tried finding a way to customize, but apparently the handler needed a standalone HTML page. When the error is called, it means you can't rely on Grav to generate anything. Something blew up. New PR on its way.

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.

3 participants