Problem/Motivation
This is a followup issue for #1986330: When Batch ID doesn't exist, Drupal should emit a 404
NotFoundHttpException accepts reason message in its constructor, but this message is never used
Proposed resolution
Modify ExceptionJsonSubscriber to use $exception->getMessage() for both 403 and 404 pages.
Modify the default 403 and 404 controllers to use the exception message. (This may take a little more work to pass through.)
Remaining tasks
Do that.
User interface changes
404 page should include reason message optionally passed in Exception constructor (that will default to the current error text if no message is provided)
API changes
None.
Beta phase evaluation
Issue category | Bug, because the current error message for JSON-based errors is completely generic and largely useless, even though we have a useful error message available. We're just not using it. That is poor DX. |
---|---|
Issue priority | Normal, because nothing is actually broken per se, just annoying to work with. |
Unfrozen changes | Unfrozen because it only changes an output string for error messages. There is no functionality change, just improved messaging for the developer. |
Disruption | There should be no disruption for any existing APIs or modules. |
Comment | File | Size | Author |
---|---|---|---|
#15 | NotFoundHttpException-2372011-15.patch | 3.71 KB | richardcanoe |
Comments
Comment #1
Crell CreditAttribution: Crell commentedSeems reasonable to me. I would call this a minor DX improvement, not an API change. Refiling accordingly.
Comment #2
dawehnerGiven that this is just a small improvement I think this rather belongs ont 8.1
Comment #3
Crell CreditAttribution: Crell commentedDisagree. This isn't a feature addition and its disruption should be practically zero. It should also be fairly simple I imagine.
Comment #4
Noe_ CreditAttribution: Noe_ commentedGiving a $event->getMessage() to the 404 and 403 pages of ExceptionJsonSubscriber.
Comment #5
Noe_ CreditAttribution: Noe_ commentedAnd set to "Needs review" obviously.
Comment #7
Crell CreditAttribution: Crell commentedI believe you need $event->getException()->getMessage().
Comment #8
Noe_ CreditAttribution: Noe_ commentedHopefully did a better job this time.
Comment #10
dawehnerJust doing that would be bad, because just the exception message would be not valid JSON. What about using
['message' => $exception->getMessage()]
It would be also great to return [] in case the message is empty.
When you have done that, we also have to adapt the existing tests, as they expect a different message.
Comment #11
richardcanoe CreditAttribution: richardcanoe commentedAdded message as array so as to create valid JSON. Tests to follow
Comment #13
richardcanoe CreditAttribution: richardcanoe commentedUpdated failing tests due to changed response.
Comment #15
richardcanoe CreditAttribution: richardcanoe commentedUnlucky 13! Somehow missed the tests on the patch. Apologies.
Comment #16
Crell CreditAttribution: Crell commentedIt is fairly simple, it seems. :-)
Comment #18
alexpottThis issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
Comment #19
Crell CreditAttribution: Crell at Palantir.net commentedAdding table. Also reclassifying as a bug. Back to you, Alex.
Edit: We also would not need a change notice here as no APIs are changing.
Comment #20
alexpottI agree with the re-classification as a bug. Committed 64332fe and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.