Problem/Motivation

See #2825347-33: 'Accept' header still is used for determining response format of error responses, ?_format= has no effect AND serialization module's exception subscriber does not support status code 500 error responses:

Note that by now, \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber really always does the same thing: it always ends up calling onHtml() for fatal errors. In other words: it's finally living up to its description:

/**
 * Last-chance handler for exceptions.
 *
 * This handler will catch any exceptions not caught elsewhere and report
 * them as an error page.
 */

So I think we should rename the class (to FatalErrorExceptionSubscriber) and simplify the logic (get rid of the current onException() and make the current onHtml() the new onException(), then get rid of everything else).

But that… is for another issue.

This is that issue.

Another consequence is that this "last chance" exception handler also then treats all unhandled exceptions the same way: it will print a backtrace if verbose error logging is enabled for both HTML requests and REST requests, and … anything else. Currently, it only does that for HTML requests.

Proposed resolution

  1. always do the same, regardless of format
  2. renames DefaultExceptionSubscriber to FinalExceptionSubscriber
  3. renames the service from exception.default to exception.final
  4. adds helpful documentation explaining how this exception subscriber compares to the others

Remaining tasks

None.

User interface changes

For non-HTML requests with a fatal PHP error or another kind of unhandled exception: a backtrace if verbose error logging is enabled.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#19 interdiff.txt1.44 KBwim leers
#19 2853300-19.patch14.38 KBwim leers
#12 2853300-12.patch13.01 KBwim leers
#12 interdiff.txt983 byteswim leers
#11 2853300-11.patch12.99 KBwim leers
#11 interdiff.txt4.67 KBwim leers
#10 2853300-10.patch9.88 KBwim leers
#10 interdiff.txt5.06 KBwim leers
#9 2853300-9.patch5.8 KBwim leers
#9 interdiff.txt5.12 KBwim leers
#3 2853300-3.patch1.7 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Rename + simplify \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber » [PP-1] Rename + simplify \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber
Status: Active » Postponed
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 2853300-3.patch1.7 KB

This is a start. This should be further simplified step-by-step, to prove that it works.

But for that to make sense (i.e. for tests to be able to pass), we need #2825347: 'Accept' header still is used for determining response format of error responses, ?_format= has no effect AND serialization module's exception subscriber does not support status code 500 error responses to land first.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] Rename + simplify \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber » Rename + simplify \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber
Status: Postponed » Needs review

#2825347: 'Accept' header still is used for determining response format of error responses, ?_format= has no effect AND serialization module's exception subscriber does not support status code 500 error responses landed, this is now unblocked!

Status: Needs review » Needs work

The last submitted patch, 3: 2853300-3.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Reviewed & tested by the community

Nice simplification!

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review
Issue tags: -maintainability +Needs change record

This patch has one less hunk than I would expect given the issue title (namely, the hunk with the new name). I also don't understand how it passes tests if the title is correct. :) Edit: Or how the patch implements the title at all, actually.

Also, I think that initial patch was just intended as an initial step and more work is outlined in the summary? The "simplify" part. I.e. what Wim said:

This is a start. This should be further simplified step-by-step, to prove that it works.

Finally, the thing in the title sounds like it would need a change record.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
Status: Needs review » Needs work

Oh yes this was definitely not yet ready! I said This is a start :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
Issue tags: +DX (Developer Experience), +API-First Initiative
StatusFileSize
new interdiff.txt5.12 KB
new 2853300-9.patch5.8 KB

This:

  1. deletes onFormatUnknown()
  2. renames onHtml() to onException()
  3. deletes the prior implementation of onException()

It also updates CommentResourceTestBase to now assert the more helpful error response that you now get in REST when a fatal error occurs: you get a stack trace there now too, if you have verbose error logging enabled!

The end result is that this Last-chance handler for exceptions. as the current docblock says has far less complexity: rather than a very complex and brittle format-based redirection, it now always does the same. It's a last-chance thing, after all.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt5.06 KB
new 2853300-10.patch9.88 KB

With #9, \Drupal\Tests\Core\EventSubscriber\DefaultExceptionSubscriberTest fails. Because it relies on:

  1. error_displayable() (defined in errors.inc)
  2. DRUPAL_ROOT (defined in bootstrap.inc)
  3. ERROR_REPORTING_DISPLAY_VERBOSE (defined in bootstrap.inc)

So, I had to make a few changes to wrap those in methods I can override for testing purposes.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt4.67 KB
new 2853300-11.patch12.99 KB

This:

  1. renames DefaultExceptionSubscriber to FinalExceptionSubscriber
  2. renames the service from exception.default to exception.final
  3. adds helpful documentation explaining how this exception subscriber compares to the others
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt983 bytes
new 2853300-12.patch13.01 KB

Oops, I made two mistakes in #10. It's a good thing we have functional tests :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Rename + simplify \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber » Standardize fatal error/exception handling: backtrace for all formats, not just HTML
Issue summary: View changes
Issue tags: -Needs change record

Title + issue summary updated. CR created: https://www.drupal.org/node/2856738.

dawehner’s picture

dawehner
Primary language German
commented

Looks super good for me!

The last submitted patch, 9: 2853300-9.patch, failed testing.

The last submitted patch, 10: 2853300-10.patch, failed testing.

The last submitted patch, 11: 2853300-11.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: 2853300-12.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 2853300-19.patch14.38 KB
new interdiff.txt1.44 KB
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
@@ -281,7 +281,7 @@ public function testPostDxWithoutCriticalBaseFields() {
-    $this->assertSame('Internal Server Error', (string) $response->getBody());
+    $this->assertStringStartsWith('The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Symfony\Component\HttpKernel\Exception\HttpException</em>: Internal Server Error in <em class="placeholder">Drupal\rest\Plugin\rest\resource\EntityResource-&gt;post()</em>', (string) $response->getBody());

<3

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

:)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2853300-19.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

Sigh. After being RTBC for a month, and it being the oldest RTBC issue for a very long time, this random infrastructure fail causes it to be the youngest RTBC issue. Note that https://www.drupal.org/pift-ci-job/634975 does not even list any failing test! :/

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed a0dcc7f and pushed to 8.4.x. Thanks!

Backtraces for REST requests - nice!

  • alexpott committed a0dcc7f on 8.4.x 
    Issue #2853300 by Wim Leers: Standardize fatal error/exception handling...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

:)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +8.4.0 release notes
adamps’s picture

adamps commented
Related issues: +#2924860: “Uncaught exception” page contains HTML but has content-type text/plain

I have found a bug #2924860: “Uncaught exception” page contains HTML but has content-type text/plain which might have been caused by or be related to this issue. I would be grateful if one of the experts here could have a look.