-
Notifications
You must be signed in to change notification settings - Fork 10.6k
fix #12516 by cleaning up Hosting error page #12545
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
Conversation
| @@ -0,0 +1,14 @@ | |||
| # Error Page | |||
|
|
|||
| This folder is shared among multiple projects. The `ErrorPage.Designer.cs` and `ErrorPageModel.cs` files are used to render this view. The `ErrorPage.Designer.cs` file is generated by rendering `src/Views/ErrorPage.cshtml` using the [RazorPageGenerator](https://github.com/aspnet/AspNetCore-Tooling/tree/master/src/Razor/src/RazorPageGenerator) tool. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Always nice to make the grass greener for the next person to work with the error page!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Even on platforms without powershell, you can look at the script to see how the generator is supposed to be invoked.
| @@ -31,12 +31,11 @@ | |||
| @foreach (var errorDetail in Model.ErrorDetails) | |||
| { | |||
| <div class="titleerror">@errorDetail.Error.GetType().Name: @{ Output.Write(HtmlEncodeAndReplaceLineBreaks(errorDetail.Error.Message)); }</div> | |||
| @{ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't valid Razor syntax, it spit out errors generating the code. I think Razor is nice and decides to work around the error but it's not actually correct.
The </div> ended the markup block so you're back in the body of the foreach which is a code context, and @{ isn't needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did it give an error when you compiled it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. It throws an error but still generates the output. But errors are bad :).
| // The startup hook is only present when detailed errors are allowed, so | ||
| // we can turn on all the details. | ||
| model.ErrorDetails = exceptionDetailProvider.GetDetails(exception); | ||
| model.ShowRuntimeDetails = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary since the property defaults to false. I thought about defaulting it to true but I think it's better for users of the page to opt-in to detailed information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be one more place we do this. IIRC we have a hosting startup error page and a detailed errors error page. Can you check these and make sure we set ShowRuntimeDetails appropriately:
- https://github.com/aspnet/AspNetCore/blob/cc1f23c5f8afb7d2a00405f19811d2372c4fcec2/src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddleware.cs
- https://github.com/aspnet/AspNetCore/blob/f3f9a1cdbcd06b298035b523732b9f45b1408461/src/Hosting/Hosting/src/GenericHost/GenericWebHostedService.cs
- https://github.com/aspnet/AspNetCore/blob/f3f9a1cdbcd06b298035b523732b9f45b1408461/src/Hosting/Hosting/src/Internal/WebHost.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the diagnostics one. It's a little deceptive because it's actually a different error page because it's even more detailed (query strings, headers, etc.) and the middleware is only enabled in Development mode.
I did miss GenericWebHostedService. Looking at that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it was the other way around :). I missed the WebHost one. Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, clarify why you don't need to updat ethe GenericWebHostedService model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Ping. Would like re-review after some changes. |
| @@ -0,0 +1,45 @@ | |||
| { | |||
| "solution": { | |||
| "path": "C:\\Code\\aspnet\\AspNetCore\\src\\Middleware\\Middleware.sln", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make relative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unfortunate that the default VS experience for this doesn't use relative paths, even though they are supported. I may file a VS feedback item for that ;).
When detailed errors is off (because Environment is not
Development) and you have a startup error and.CaptureStartupErrorsis one, this is what you get now:I checked IIS and the Diagnostics error page. They look good already. IIS shares the page but only ever renders it if detailed errors are enabled (via Environment or the environment variable override).
Fixes #12516