Add the ability to surface more helpful error details for display in the UI

Assigning to @balintbrews to flesh out any front-end details and issue metadata.

📌 Also validate request bodies against the OpenAPI spec Downport should land before we can work on this. I also recommended in that issue 📌 Also validate request bodies against the OpenAPI spec Downport that we implement the backend side as part of that scope, then we can focus on the required client-side changes in this issue. We need to know the shape of the response in case of errors, then this will come down to making ErrorBoundary.tsx smarter about what kind of error types and shapes it can handle, and consolidating how user facing messages are constructed.

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. Though I'm curious what debugging info you had in mind — there might be interesting use cases I'm not thinking about.

📌 Also validate request bodies against the OpenAPI spec Downport should land before we can work on this.

I actually don't think it's a prerequisite because it's not really specific to that issue. 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?

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.

#5++

On the backend, that means adding a new exception that takes additional details, i.e., the user-facing error message and the console debugging details, and returns it in the form of JSON.

I created 🐛 OpenAPI validation errors must be provided as a JSON response RTBC for that yesterday, independently of this issue. (Another issue/MR was stuck on getting tests to work, and if the error response had been more explicit, it'd have been trivial.)

So let's keep this issue scoped to only:

On the front-end, that means translating the JSON into the error modal and console log.

Moved #4 + #5 to #3470995-6: OpenAPI validation errors must be provided as a JSON response → . Let's keep this focused on the client side, and hence assigned to @balintbrews.

I agree with @traviscarden in #4 that this isn't blocked on 📌 Also validate request bodies against the OpenAPI spec Downport , but it is now blocked on 🐛 OpenAPI validation errors must be provided as a JSON response RTBC .

Marked 🐛 Blocks screen when error encountered Closed: duplicate as a duplicate of this.

🐛 OpenAPI validation errors must be provided as a JSON response RTBC is RTBC so this can be worked on now to expose JSON error messages to the user where necessary.

Server-side support aka 🐛 OpenAPI validation errors must be provided as a JSON response RTBC landed. Which means this is unblocked.

This is critical to improve the productivity of people working full-time on XB, but also to improve the quality of bug reports.

This would've saved me at least an hour, and probably would've saved @f.mazeikis multiple hours on this single issue alone: ✨ [PP-1] Integrate saving sections with the backend Postponed 😬

This should happen ASAP, and should probably do something like this:

diff --git a/src/EventSubscriber/ApiExceptionSubscriber.php b/src/EventSubscriber/ApiExceptionSubscriber.php
index deb79ca6..1a56583e 100644
--- a/src/EventSubscriber/ApiExceptionSubscriber.php
+++ b/src/EventSubscriber/ApiExceptionSubscriber.php
@@ -10,6 +10,7 @@ use Drupal\Core\ParamConverter\ParamNotConvertedException;
 use Drupal\Core\Routing\RouteMatchInterface;
 use Drupal\Core\Session\AccountInterface;
 use Drupal\experience_builder\Exception\ConstraintViolationException;
+use League\OpenAPIValidation\PSR7\Exception\Validation\AddressValidationFailed;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 use Symfony\Component\HttpFoundation\JsonResponse;
 use Symfony\Component\HttpFoundation\Response;
@@ -75,6 +76,10 @@ final class ApiExceptionSubscriber implements EventSubscriberInterface {
       // Generate a JSON response containing details when the status is 500, if
       // the current user has access to it.
       if ($status === 500) {
+        if ($exception instanceof AddressValidationFailed) {
+          // @see \Drupal\experience_builder\EventSubscriber\ApiMessageValidatorBase::logFailure
+          $response['message'] = $exception->getVerboseMessage();
+        }
         // The stack trace may contain sensitive information. Only show it to
         // authorized users.
         // @see \Drupal\jsonapi\Normalizer\HttpExceptionNormalizer::buildErrorObjects()

How about ^ this, @wim leers? It takes your proposal with two small changes:

1. Instead of testing for a specific exception class, it just checks for a ::getVerboseMessage() method and uses it if it's there.
2. It extracts the conditional local to a helper class.

So nice to see this moving forward! 🤩🥳 May it save us all a lot of time! 😄

I'm not convinced a utility class is worth it, because I doubt (m)any other code paths will end up calling this. But … that's really negligible subjective commentary, and could very well change.

I can't RTBC this because:

1. PHPStan is not passing
2. PHPUnit tests are not passing
3. I could fix those two, but the #1 thing this should be getting us, is the more helpful exception message appearing in the dialog shown in the issue summary. So I modified the return value for \Drupal\experience_builder\Controller\ApiLayoutController::__invoke() by renaming one of its 4 key-value pairs to be a wrong key. This triggers an OpenAPI error as expected, but that doesn't appear in the dialog:

Then again, I shouldn't have been surprised, because @balintbrews' issue summary clearly states:

On the front-end, that means translating the JSON into the error modal and console log.

But I'm not quite sure how an error message is supposed to show up, given that even

diff --git a/ui/src/components/error/ErrorBoundary.tsx b/ui/src/components/error/ErrorBoundary.tsx
index 5f649468..0e98257a 100644
--- a/ui/src/components/error/ErrorBoundary.tsx
+++ b/ui/src/components/error/ErrorBoundary.tsx
@@ -60,7 +60,7 @@ export default ErrorBoundary;
 
 const getRouteErrorMessage = (error: unknown): string => {
   if (isRouteErrorResponse(error)) {
-    return `${error.status} ${error.statusText}`;
+    return 'yo';
   } else if (error instanceof Error) {
     return error.message;
   } else if (typeof error === 'string') {

results in nothing appearing. So marking only for the back-end changes; once those are done, please assign to @balintbrews so he can tackle the front-end bits in a fraction of the time it'd take me 🙏😊

Well, I got PHPStan passing, but I'm getting completely different PHPUnit failures locally from CI. I'll assign this to @balintbrews until I come back to look at it again tomorrow in case he can make any progress on the frontend part in the meantime.

I can reproduce the same error using Drupal 11.1.x — which is also what CI is using. What version of Drupal core are you using?

I bet this is PHPUnit trying to generate a helpful failure message, but it fails in trying that because a prophecy (which uses reflection) is part of trying to use that.

More importantly: prophecies are appropriate in unit tests, not in kernel tests. See \Drupal\Tests\experience_builder\Unit\ApiLogControllerTest::testApiLogController() for an example of a controller being tested using a unit test 😊

Thanks, @wim leers. Quick status update:

1. Thanks for pointing out the pattern in `ApiLogControllerTest::testApiLogController()`. If Prophecy was a problem, it apparently wasn't the whole one, because I'm still getting the same "Serialization of 'ReflectionClass' is not allowed" error on CI. I don't see any indication of which test is causing it--unfortunately, because the new tests are passing for me locally.
2. I'm also using Drupal 11.1.x, yet other tests are failing even on 0.x--not the same ones as on CI.

Ted and I are going to look at this together this afternoon. Hopefully I'll be able to report progress then.

That's better. Thanks for helping me figure out the object serialization bit, @tedbow. The tests are passing now. On to @balintbrews for the frontend part.

wim leers → credited tedbow → .

While it'd be great to surface this in the UI, the updated expectations in XbConfigEntityHttpApiTest will be super valuable while working on ✨ HTTP API for code component config entities Active !

So: let's first land the back-end parts of this issue. 😄 (We can totally do >1 MR/issue.)

Back to for a new MR that'll tackle the client-side bits!