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

Show error stack traces in default_error_page #914

Closed
wants to merge 8 commits into from

Conversation

vlyon
Copy link
Contributor

@vlyon vlyon commented May 26, 2015

Fix for the missing stack trace on the default error page.

When show_errors is true, the default_error_page will now display a full stack trace (currently unused!) instead of the very useless basic message.

This is obviously the intended behaviour. See #769

If show_errors is false, a static page is shown if available, but if it's not available then the current default_error_page will actually show the error, ignoring the show_errors setting.
This is also fixed by this change.

@xsawyerx
Copy link
Member

👍

@vlyon
Copy link
Contributor Author

vlyon commented Jun 12, 2015

Hi @xsawyerx & @veryrusty.

Please could I ask you pull these fixes before we rename show_error to show_stacktrace as requested in #769 & #829.
The rename is a very good idea and gets a 👍 from me, but only after this fix.

Currently Dancer2 won't show any stack-trace _ever!_

These fixes are being used constantly on our dev system and have been very valuable and work well.
Any thoughts, concerns, issues, please let me know.

Thanx,
Vernon

@xsawyerx
Copy link
Member

When I think about it further, it would seem that there are two different things: showing errors (show_errors) and showing stack traces (show_stacktrace). One could control whether you see something at all (which it always has) and the other on how much you see.

@veryrusty @mickeyn @yanick @vlyon thoughts?

@vlyon
Copy link
Contributor Author

vlyon commented Jun 29, 2015

It is true, that you could add more options to control how much detail we see in errors, but is that really needed?

The changes in this PR only affect the default_error_page generation, which only happens after it's tried to use the template (views/500.tt) the static page (if show_errors is off) and the error_template which can be set in the config.
Only when all these efforts fail does it resort to the default_error_page. Do we really want to add extra options for controlling this last resort? (At this point I'm also pretty certain we're dealing with a development environment anyway.)

We could always add options to control the level of detail displayed in this handler later, if that is what is really desired. But it would be nice to improve at least this part of it now.

There is one way to somewhat implement what @xsawyerx is mentioning, which would result in behaviour slightly closer to what it is right now, but I'm not sure if it's what we want...
On lines 107 & 108 of Errors.pm we could alter the lines from this:

my $message = $self->show_errors ?
    $self->full_message : 'Wooops, something went wrong';

to this:

my $message = $self->show_errors ?
    $self->full_message : $self->message || 'Wooops, something went wrong';

Whattcha thunk?

@vlyon
Copy link
Contributor Author

vlyon commented Jul 13, 2015

Should I break this into smaller PRs?

@vlyon vlyon force-pushed the pr/error-stacktrace branch 2 times, most recently from a12e8b9 to 7ff4261 Compare July 21, 2015 08:43
@vlyon
Copy link
Contributor Author

vlyon commented Jul 27, 2015

C'mon guys, @veryrusty @mickeyn @yanick @xsawyerx
Which error page do you prefer?

Without
☝️ Not very useful now.
This is how this pull request will look. 👇

With

@veryrusty
Copy link
Member

@vlyon I promise to look through this sometime this week (i.e. before the end of July).

@vlyon
Copy link
Contributor Author

vlyon commented Jul 28, 2015

@veryrusty Awesome thanks.

I also have a small change to improve the looks of the Dumped data (remove padding on the left and add css overflow-x: auto to pre.content so that it displays perfectly).

I'll add it in a mo.

@veryrusty
Copy link
Member

😄 Its still July and I've had a look!

@vlyon++ Well done and a huge thanks for tackling this. Comments added on lines/commits as I think are appropriate ( only small code nits ). Do you have the tuits to clean these comments up?

@vlyon
Copy link
Contributor Author

vlyon commented Jul 30, 2015

@veryrusty Thanx.
I've fixed up the first commit, looking into the second one now.

@veryrusty
Copy link
Member

👍 I'll merge this in the next couple of days.

@veryrusty
Copy link
Member

Merged with commit 03cd334.

Thanks again @vlyon !

@veryrusty veryrusty closed this Aug 3, 2015
@vlyon
Copy link
Contributor Author

vlyon commented Aug 4, 2015

Thanx @veryrusty++

Now I can start work on producing a more useful stack trace... 👍

@vlyon vlyon deleted the pr/error-stacktrace branch August 4, 2015 10:25
xsawyerx added a commit that referenced this pull request Aug 28, 2015
    [ BUG FIXES ]
    * GH #947, #948: Escape file paths in regex patterns. (A. Sinan Unur)
    * GH #944: Setting response content in before hook when a serializer
      is set no longer triggers an error.
      (Russell Jenkins, Dmitrii Tcyganov)
    * GH #965: Remove non-existant role from Response::Delayed.
      (Vernon, Russell Jenkins)
    * GH #971: Route options matching no longer uses each iterator.
      (Tina Müller)
    * GH #959: Custom error template rendering fixed. (Russell Jenkins)
    * GH #961: Render custom error templates in before hooks. (Russell Jenkins)
    * GH #978: Tests - fix response regex after html_encode (Vernon)
    * GH #972: Exceptions thrown by serializers no longer masked.
      (Russell Jenkins)

    [ DOCUMENTATION ]
    * GH #967: Fix upload example. (Alberto Simões)
    * GH #881: Add cookie timeout example. (Andy Beverley)
    * GH #963: Document all available template tokens. (Sawyer X)

    [ ENHANCEMENTS ]
    * Optimize the s*#t out of basic routing. Faster than Dancer 1 now.
      (Sawyer X)
    * Only load HTTP::Server::PSGI when asked to start a development
      server not under Plack. (Sawyer X, Mickey Nasriachi)
    * GH #949: Produce cleaner, non-verbose test output (Vernon)
    * GH #950: Decode characters in param keys (Patrick Zimmermann)
    * GH #914: Include stack trace on default error page when
      show_errors is true. (Vernon)
    * GH #980, #981: halt keyword sets response content if provided,
      as Dancer 1 does. (Achilles Kars)
    * GH #909, #957, #983: HTML5 templates in generated apps and
      default error template (Gabor Szabo, Kadir, Vernon)
    * GH #972, #719, #969, #644, #647: Streamline serializer helpers.
      to_json/from_json now faster. (Russell Jenkins)
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

3 participants