Overview

See #3454173-45: Media Library integration (includes introducing a new main content renderer/`_wrapper_format`) for how #3450308: Create an Open API spec for the current mock HTTP responses actually caused the DX to regress rather than improve in some scenarios.

(In this case: an empty Ajax response was sent, with no visible error — the error itself was logged. This is very non-obvious 😬)

Proposed resolution

Ensure that

throw new ValidationFailed($message, $e->getCode(), $e);

… actually results in an appropriate error response being generated.

User interface changes

None … except in case of fatal errors, then you get an actually-useful-to-report error message:

CommentFileSizeAuthor
#14 Screenshot from 2024-09-20 15-03-44.png21.47 KBlongwave
#11 Screenshot from 2024-09-20 14-27-44.png132.92 KBlongwave
#2 Screenshot 2024-08-30 at 10.47.29 AM.png549.23 KBwim leers

Issue fork experience_builder-3470995

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: OpenAPI validation errors must be explicitly visible in the responses » OpenAPI validation errors must be provided as a JSON response
StatusFileSize
new Screenshot 2024-08-30 at 10.47.29 AM.png549.23 KB

If such errors are always JSON responses, then we can update the work @balintbrews introduced in #3461431: Improve client side error handling to surface a friendlier error than this:

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +blocker
Related issues: +#3470321: Surface verbose OpenAPI errors in the UI — for better bug reports and faster DX

Aha, for #2 we already have #3470321: Surface verbose OpenAPI errors in the UI — for better bug reports and faster DX! 👍

But without a structured error response from the server for the client to consume, fixing #2 is impossible.

IOW: this is a blocker for #3470321: Surface verbose OpenAPI errors in the UI — for better bug reports and faster DX.

Wim Leers credited effulgentsia.

Wim Leers credited traviscarden.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » traviscarden

Transfering relevant info + credit from #3470321: Surface verbose OpenAPI errors in the UI — for better bug reports and faster DX:

Any path that can result in an exception needs to return well-formed JSON that the UI can interpret as an error message. On the backend, that probably just means creating a new exception class (if one doesn't already exist). On the front-end, it may mean no more than you telling me the JSON format you need for error popups.

One thought I had on what's proposed is that debugging info in case of error responses from the API should be logged on the server side. E.g. a PHP stack trace surfaced in the browser is a security vulnerability.

That's a good point. We already have Drupal logging in place. Now that you point it out, I'm not sure what debugging info would be both safe and helpful in the browser console. Let's start with just the UI error message. Can you tell me how I need to format my JSON error message to be displayed nicely in the UI?

— @traviscarden, #3470321-4: Surface verbose OpenAPI errors in the UI — for better bug reports and faster DX

By the way, there's some prior art in core's jsonapi module for returning a stack trace in the json response if the site is configured to display verbose errors and the user has access to site reports.

So long as we do those two checks, it's safe. But, the question is whether in XB's case if seeing the stack trace in the browser would be helpful or not.

— @effulgentsia, #3470321-5: Surface verbose OpenAPI errors in the UI — for better bug reports and faster DX.

longwave made their first commit to this issue’s fork.

longwave opened merge request !321

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
Issue tags: +Needs tests
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

The implementation here:

  • tells Drupal which XB routes return JSON format by setting the _format requirement
  • intercepts exceptions for JSON format routes that are named experience_builder.*
  • returns the exception message as a simple JSON structure

This is enough to get started on displaying nicer errors on the client. I am not sure a stack trace would be useful to the client but it might be useful to send in the JSON data anyway, for debugging via the network inspector?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue summary: View changes
StatusFileSize
new Screenshot from 2024-09-20 14-27-44.png132.92 KB

A screenshot of this in action, exploiting the crash in #3469855: Emptying a required value through the UI crashes the app (empty <input>, selecting "None" option in <select> …) before that fix is merged:

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Simplified this further, we have precedent in XbRouteOptionsEventSubscriber for API routes already so I used that instead - no need to force _format: json (although perhaps we still should for correctness?)

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue tags: -Needs tests

It turns out that EntityFormControllerTest already tests an error that should be returned in JSON format, so I reworked the test case there to expect JSON instead - that should be enough coverage to start with?

I also ported the stack trace functionality from JSON:API module with the same conditions, and the test covers that too.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new Screenshot from 2024-09-20 15-03-44.png21.47 KB

With a one line change I've exposed the message in the UI if the preview endpoint fails:

traviscarden’s picture

traviscarden commented
Assigned: traviscarden » longwave
Status: Needs review » Needs work

I can RTBC this as soon as the (trivial) requested change is made.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Assigned: longwave » Unassigned

Reverted the UI change as it's not TypeScript safe, and should be done in #3470321: Surface verbose OpenAPI errors in the UI — for better bug reports and faster DX

traviscarden’s picture

traviscarden commented
Status: Needs review » Reviewed & tested by the community

Nice. This should immediately improve the experience for everyone.

I don't seem to have the ability to actually merge the MR, so I'll mark this RTBC until I can get it.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes

Adding @longwave's excellent screenshot from #14 to the issue summary.

traviscarden’s picture

traviscarden commented

I've been trying to get access to merge this MR all day, and despite apparently having all the necessary permissions, I still cannot. So if someone who can merge it kindly would, that would be great. You have my approval to do so.

hooroomoo made their first commit to this issue’s fork.

  • hooroomoo committed 1e3134ff on 0.x authored by longwave 
    Issue #3470995 by longwave, traviscarden, wim leers, effulgentsia:...
hooroomoo’s picture

hooroomoo
they/them
Location nyc🗽
commented
Status: Reviewed & tested by the community » Fixed

Merging on behalf of @traviscarden.

Status: Fixed » Closed (fixed)

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