Use `RenderSafeComponentContainer` also when normalizing `Component` config entities (was: "SDC using an invalid filter in Twig somehow bypasses the robust error handling")

This is surprising. #3517966: Failing kernel tests for all ways a component source can fail to render on the server side added test coverage for exactly this and #3485878: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log) was supposed to protect against this.

See tests/modules/xb_test_sdc/components/crash/crash.twig. That is handled correctly. But this evidently is not. So let's at minimum expand our existing test coverage with this, and then go from there.

Epic find! 👏

I've found the issue. The problem is that the Renderer in Core can throw an exception and we're not handling it in XB.
It breaks in the ClientSideRepresentation. Here is the diff to explain a bit:

diff --git a/components/my-hero/my-hero.twig b/components/my-hero/my-hero.twig
index 5b39e616..ce9c5c02 100644
--- a/components/my-hero/my-hero.twig
+++ b/components/my-hero/my-hero.twig
@@ -1,4 +1,4 @@
-<div {{ attributes }} class="my-hero__container">
+<div {{ attributes|invalidFilter }} class="my-hero__container">
   <h1 class="my-hero__heading">{{ heading }}</h1>
   <p class="my-hero__subheading">{{ subheading }}</p>
   <div class="my-hero__actions">
diff --git a/src/ClientSideRepresentation.php b/src/ClientSideRepresentation.php
index 4637364f..baa6368f 100644
--- a/src/ClientSideRepresentation.php
+++ b/src/ClientSideRepresentation.php
@@ -11,6 +11,7 @@ use Drupal\Core\Cache\RefinableCacheableDependencyTrait;
 use Drupal\Core\Render\BubbleableMetadata;
 use Drupal\Core\Render\RendererInterface;
 use Drupal\experience_builder\Render\ImportMapResponseAttachmentsProcessor;
+use Twig\Error\SyntaxError;
 
 /**
  * @see \Drupal\jsonapi\Normalizer\Value\CacheableNormalization
@@ -49,7 +50,11 @@ final class ClientSideRepresentation implements RefinableCacheableDependencyInte
     }
 
     $build = $this->preview;
-    $default_markup = $renderer->renderInIsolation($build);
+    try {
+      $default_markup = $renderer->renderInIsolation($build);
+    } catch (SyntaxError $e) {
+            $default_markup = '<div>Render error: ' . htmlspecialchars($e->getMessage()) . '</div>';
+    }
     $assets = AttachedAssets::createFromRenderArray($build);
     $import_map = ImportMapResponseAttachmentsProcessor::buildHtmlTagForAttachedImportMaps(BubbleableMetadata::createFromRenderArray($build)) ?? [];

That's why it's throwing an exception and we're not catching it, and with this, the result is... better.

The problem is that in the Renderer, when it detects the exceptions, it catch it,set it to renderingRoot to false and throw again the exception.

    try {
      return $this->doRender($elements, $is_root_call);
    }
    catch (\Exception $e) {
      // Mark the ::rootRender() call finished due to this exception & re-throw.
      $this->isRenderingRoot = FALSE;
      throw $e;
    }

My question is: Should it be treated as an `IneligibleComponent? My guess is NO, because is an error in runtime. How can we detect an invalid filter (or something else) in the code without rendering it first? Maybe render with the example values inthe setCachedDefinitions? I guess is too killer.

My guess is that we should do something like I have in the previous code and show the user that the component in terms of schema is ok, that's why it's showing there, but there is/are errors in the runtime.

I'll work on the test with this invalid filter.

TIL Twig throws \Twig\Error\SyntaxError and Drupal core's renderer service does not catch it 🤯 I didn't know core's abstractions were so leaky 😬 I'd argue this is a bug in the SDC subsystem in Drupal core 😇 → adding to #3462705: [META] Missing features in SDC needed for XB.

Thanks for the detailed analysis and write-up in #6, that allowed me to review this in ~5 mins instead of >30! 🙏👏

My question is: Should it be treated as an `IneligibleComponent? My guess is NO, because is an error in runtime. How can we detect an invalid filter (or something else) in the code without rendering it first?

Great question!

• Yes, it should be treated as an ineligible component.
• Yes, we can detect this: \Twig\Parser::parse() would throw it.

Maybe render with the example values inthe setCachedDefinitions? I guess is too killer.

That seems very reasonable to me. But AFAICT the above would be sufficient.

How can we detect an invalid filter (or something else) in the code without rendering it first?

We can: Parser::parse() 🤓

Thanks to @isholgueras' analysis, this is also something that falls in the #3520484: [META] Production-ready ComponentSource plugins bucket, not #3517941: [META] Robust component instance error handling during hydration+rendering.

Actually, it's both! So keeping as-is. I'd indeed have expected #3517941 to have prevented this, even if the SDC discovery logic should've detected and prevented this. Because it's possible an SDC is modified and then starts failing.

See https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... for the test coverage that should be updated to prove that \Drupal\experience_builder\Element\RenderSafeComponentContainer::renderComponent() SHOULD catch this and degrade gracefully. We'll need to debug why that's not the case.

Reworked to re-use the existing test to prove that this bug has nothing to do with what that test is testing, which is the preview component.
Instead the error is with the ::normalizeForClientSide.
Added changes to the existing test to demonstrate the bug and then added the fix, which is to use the render safe component container.

I'm relieved that the fear/surprise I expressed in #10 has been disproven :)

This IMHO stresses the importance of #3521041: [META] Refactor the organically grown Component metadata mess in PHP (`::getClientSideInfo()`) and JS (`BaseComponent`, `DynamicComponent`, `PropSourceComponent` etc.) — we shouldn't have to manually use RenderSafeComponentContainer all over the place?

We could still do the discovery bits suggested by @isholgueras in #6 and confirmed by me in #9.

But that’s going very far in babysitting broken code. But most importantly: an SDC’s Twig template could change any second! (In that sense, this is similar to #3470422: Handle components provided by ComponentSources IMPLICITLY disappearing: gracefulness when developing SDCs: SDCs may appear/disappear from one request to the next.) Then the discovery wouldn’t help. But the RenderSafeComponentContainer will.

I did express in #10 my surprise at RenderSafeComponentContainer not handling this — both @isholgueras and I missed that we simply were not using it in this one spot, but thankfully @larowlan spotted that!

So this is the simplest possible solution that avoids broken SDCs being able to break XB 😊