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

Reference: https://www.drupal.org/core/beta-changes
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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Category: Feature request » Task

Seems reasonable to me. I would call this a minor DX improvement, not an API change. Refiling accordingly.

dawehner’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue summary: View changes
Issue tags: +DX (Developer Experience)

Given that this is just a small improvement I think this rather belongs ont 8.1

Crell’s picture

Version: 8.1.x-dev » 8.0.x-dev
Issue summary: View changes
Issue tags: +Novice

Disagree. This isn't a feature addition and its disruption should be practically zero. It should also be fairly simple I imagine.

Noe_’s picture

FileSize
1.02 KB

Giving a $event->getMessage() to the 404 and 403 pages of ExceptionJsonSubscriber.

Noe_’s picture

Status: Active » Needs review

And set to "Needs review" obviously.

Status: Needs review » Needs work

The last submitted patch, 4: 2372011-4.patch, failed testing.

Crell’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
@@ -39,7 +39,7 @@ protected static function getPriority() {
+    $response = new JsonResponse($event->getMessage(), Response::HTTP_FORBIDDEN);

I believe you need $event->getException()->getMessage().

Noe_’s picture

Status: Needs work » Needs review
FileSize
1.05 KB

Hopefully did a better job this time.

Status: Needs review » Needs work

The last submitted patch, 8: 2372011-8.patch, failed testing.

dawehner’s picture

Just 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.

richardcanoe’s picture

Status: Needs work » Needs review
FileSize
1.51 KB

Added message as array so as to create valid JSON. Tests to follow

Status: Needs review » Needs work

The last submitted patch, 11: NotFoundHttpException-2372011-11.patch, failed testing.

richardcanoe’s picture

Status: Needs work » Needs review
FileSize
1.51 KB

Updated failing tests due to changed response.

Status: Needs review » Needs work

The last submitted patch, 13: NotFoundHttpException-2372011-13.patch, failed testing.

richardcanoe’s picture

Status: Needs work » Needs review
FileSize
3.71 KB

Unlucky 13! Somehow missed the tests on the patch. Apologies.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

It is fairly simple, it seems. :-)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This 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.

Crell’s picture

Category: Task » Bug report
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Adding 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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs issue summary update

I 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.

  • alexpott committed 64332fe on 8.0.x
    Issue #2372011 by richardcanoe, Noe_: NotFoundHttpException handler...

Status: Fixed » Closed (fixed)

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