Problem/Motivation
Our kernel exception handling was written as a quick'n'dirty "make it work for now" implementatoin. It's not extensible, it's barely comprehensible, it ties a bunch of different possible ways of handling exceptions into a single mega method, it's needlessly complex with potentially multiple levels of subrequest... really it has no redeeming value at all. (I can say that since I wrote it.)
Proposed resolution
Eliminate the ExceptionController concept entirely. Just wire all exception handling to the kernel exception event directly, breaking the various and sundry options down to different listeners. Each listener can decide for itself if it wants to handle or pass on the exception.
For the common case of handling HTTP exceptions, we provide a utility base class that simplifies HTTP handling.
This approach was discussed with the Alexes at Twin Cities DrupalCamp.
Remaining tasks
Commit it.
This should make #2063303: A Drupal 8 AjaxResponse that must return a 403, returns a 403 and prints "A fatal error occurred: " much easier to resolve, too.
User interface changes
None.
API changes
Contrib modules may now sanely add exception handling, especially for custom accept header formats. For details see the change notice draft.
Comment | File | Size | Author |
---|---|---|---|
#46 | interdiff.txt | 16.16 KB | dawehner |
#46 | exception-2323759-46.patch | 79.43 KB | dawehner |
#42 | exception-2323759-42.patch | 70.35 KB | dawehner |
#42 | interdiff.txt | 13.54 KB | dawehner |
#39 | interdiff.txt | 7.05 KB | Crell |
Comments
Comment #2
dawehnerComment #3
Crell CreditAttribution: Crell commentedFixed a lot of stupid errors, plus #2. It should actually install now. :-) Let's see what else fails.
Comment #5
Crell CreditAttribution: Crell commentedThis should get all of phpunit passing.
There's still an issue with simpletest. Somehow, all drupalPostForm() calls from simpletest are now 404ing. How that's happening I have no idea. Suggestions welcome.
Comment #7
Crell CreditAttribution: Crell commentedWell that was silly...
Comment #9
dawehnerProbably something like this.
Comment #11
Crell CreditAttribution: Crell commentedLet's see how this fares. Note to self: There are non-HTTP exceptions. :-) Also, we now log everything regardless of what type of exception it is.
Also with a nice big interdiff. And crap, I didn't see dawehner's patch before I wrote this. :-( Interdiff is against #7.
Comment #13
Crell CreditAttribution: Crell commentedWell let's try it with dawehner's priority change... (The other changes were already made by #11, effectively.)
Comment #15
Crell CreditAttribution: Crell commentedThis should get some of them. I think there's probably a few fails that are happening because we're throwing more accurate exceptions now and the tests can't handle that, but let's confirm.
Comment #17
dawehner...
Comment #18
dawehnerStarted to fix a couple of the failures.
Comment #20
Crell CreditAttribution: Crell commentedI think that was the wrong interdiff. :-)
Comment #21
dawehnerhere it is.
Comment #22
dawehnerSome further improvements.
Comment #24
Crell CreditAttribution: Crell commentedIs this something we're doing already but I forgot to keep in the conversion, or is this adding something to attributes? (Because I know Mark and catch will object to the latter.)
Comment #25
Crell CreditAttribution: Crell commentedYeesh! Found at least some of them. In both cases it was caused by existing shortcomings of the codebase that we tripped over. I've left big @todo and explanations for both of them.
Comment #27
dawehnerThis is done something which we have done earlier, see (ExceptionListener)
.
Note: There is a dedicated interface for the http exception.
Comment #28
dawehnerComment #30
Crell CreditAttribution: Crell commentedEr, debug code maybe?
Comment #31
dawehnerDamn you are right, let's try it again.
Comment #32
Crell CreditAttribution: Crell commenteddawhener++
The logger listener should have a very high priority so that it always fires. A negative priority means most custom/contrib listeners would implicitly bypass logging, which I doubt is desirable.
Shouldn't this also have a positive priority, so that it happens really early? That seems rather the point of the Fast404 listener.
Comment #33
Crell CreditAttribution: Crell commentedChange record drafted: https://www.drupal.org/node/2331613 (Just to get out in front for a change...)
Comment #34
chx CreditAttribution: chx commentedRegarding
CustomPageExceptionHtmlSubscriber
$this->systemSiteSettings = $config_factory->get('system.site');
in a constructor imo is a bit too much.$subrequest
should imo be$sub_request
but also I think it'd be cleaner + more visible if theRequest::create
arguments were broken out into variables and changed on POST. Why POST only? Do we not need to consider, I dunno, PUT and stuff?on403
andon404
into a helper? Do they differ beyond page.403 vs page.404 and HTTP_FORBIDDEN vs HTTP_NOT_FOUND ?Fast404ExceptionHtmlSubscriber
has -5 claiming it to be very high. Also, in ParamConversionEnhancer we change from 0 to 75. Undocumented magic numbers worry me especially when I don't understand the consequences. The doxygen onHttpExceptionSubscriberBase::getPriority
doesn't really help. What does it mean to have a "first attempt"...?\ No newline at end of file
tsk, tsk.+ if (in_array($format, ['drupal_modal', 'drupal_dialog', 'drupal_ajax', 'hal_json'])) {
there's no way hardwiring them like that is not going to backfire. Move it to a service argument or something.$this->logger->get('page not found')->warning(String::checkPlain($request->attributes->get('_system_path')));
That's odd. Watchdog had messages and variables separately and we didn't escape on logging. Why do we do that now?Comment #35
Crell CreditAttribution: Crell commentedDiscussing with chx and dawehner in IRC:
* We'll add notes about what the default/significant priorities are to the subscriber base class. For most cases, the 0 default should be fine.
* Point 2, that's existing code that's just being cut/pasted. It probably could be made more robust but I'd prefer to punt that to a later issue.
* Point 3 is specifically for the HTML handling. We'll have both methods share a utility method to reduce duplication there.
* Point 4: The subrequests in CustomPageExceptionHtmlSubscriber we can toss a try-catch around. If anything happens we turn everything into a bare 500 message, because we can't do much else.
* We can replace the if check above with something that examines the Accept header directly and just does a strpos for "json". Since this is the "catch of last resort" it's OK if that may have a few false positives in it. (We'll still check for core's 3 custom formats since they don't have json in their mime type.)
* And we still need to fix the priorities as noted in #32.
Comment #36
Crell CreditAttribution: Crell commentedI think this addresses all the points from #32 and #34. Thanks, chx.
Comment #38
dawehnerCrell, the king of introducing changes which he should know better due to earlier versions of the patch.
Didn't previous code logged the exception at least? (See ExceptionListener)
What happens if browser vendors decided at some point that they actually also accept json by default? At least we have to add a @todo which checks that we should use HTML in case it has a higher priority has json.
Sorry again, these changes are wrong! Subscribers do convert an application specific exception by default on priority 0 into a HTTP specific one. ALSO exception logging should for example not log a ResourceNotFoundException but a 403/404 one, which results from that.
Comment #39
Crell CreditAttribution: Crell commented*bows* Thank you, thank you!
1) Implicitly I suppose, but not explicitly. I've added some explicit logging.
2) Tweaked a bit. If you allow html and json, you get html, because I can't imagine anyone consciously doing that other than a browser with its shotgun approach.
3) We can't have a negative priority or we will skip logging any exception for a contrib module that uses the default priority for a custom format. That's no good. Let's try moving the others.
Also, I realized we didn't have an issue summary yet. Oopsies. Fixed that.
Comment #40
dawehnerOkay.
Comment #41
tim.plunkettThe actual substance of the changes seems good, just some questions about the periphery and then some coding standards nitpicks.
Usually we've named these logger.channel.[subsystem|module], what's the significance of "php" here?
Are we going to have to sprinkle this in everywhere? Hope this is just one...
Missing line before docblock, missing leading \
These are missing their one liners.
Missing blank line at end of file (in some) and before end of class (in most)
The one line description should be first, blank line, then @var
The extra lines in multiline @todos should be indented 2 spaces
Here's another one. Just those two? Why? Can we get a code comment why those two places need to check for an exception?
Comment #42
dawehnerOkay let's ensure that this issue doesn't start to sleep.
Well, we used to have watchdog('php') calls, 'logger.channel.php' is an exact replacement for that.
Well, they want to have special behavior on exception pages, it is as simple as that.
Let's fix those as well as some more issues.
Comment #43
Crell CreditAttribution: Crell commentedThanks, dawehner!
I can't RTBC this but I feel it is ready to be.
Comment #44
tim.plunkettI agree that it's ready. Thanks for responding to my comment, @dawehner!
Comment #45
alexpottThis looks like a really sensible clean up. ExceptionController and ExceptionListener were not mentioned in an existing CRs and I don't think we need one here.
Duplicate service keys? Missing json exception test coverage?
unconditionally is misspelt
Needs an issue and to finish the final word.
Utility misspelt
naturally misspelt
Not used.
Do we have equivalent test coverage already existing?
Comment #46
dawehnerReally good catch! Let's expand the test coverage.
Storm tells you that, right?
The first one is now covered by the new ExceptionHandlingTest, now also ExceptionListenerTest
Comment #47
Crell CreditAttribution: Crell commentedThis is technically not necessary as text/html is the default on Response if nothing is specified. It doesn't hurt anything, though. The rest of the interdiff seems fine to me, though, so back to Alex.
Comment #48
alexpottCommitted 51a27d1 and pushed to 8.0.x. Thanks!