Invalid prop errors should be detailed in Component preview

wim leers → credited neha_bawankar → .

See the 2 "FAIL" test cases in the report by @neha_bawankar at #3528396-26: Video prop shape: e2e test coverage for video from media library the component instance form + fix revealed bug in evaluating `StaticPropSource`s → . I explained in #3528396-27: Video prop shape: e2e test coverage for video from media library the component instance form + fix revealed bug in evaluating `StaticPropSource`s → why those are both symptoms of this issue not being solved.

wim leers → credited effulgentsia → .

This is something @effulgentsia, @bnjmnm, I and possibly others discussed before this issue was created.

@lauriii wasn't around then. It came up again a few days ago. Thanks to @neha_bawankar the very confusing impact for content authors is now much clearer.

Time for Lauri to chime in and to work with designers to articulate a way forward here.

Per #3529788-32: Required `type: string` (without other constraints, so "prose strings") props should fall back to empty string (even though invalid) while Content Author is typing, to allow matching live preview in most cases → we also need to take into account when zend.assertions are set to 1 as that will also cause a render fail from \Drupal\Core\Template\ComponentsTwigExtension::validateProps which is dynamically injected into each component's twig template via \Drupal\Core\Template\ComponentNodeVisitor

Even if XB doesn't validate component input when creating the preview, that code will when assert is enabled.

We could either:

• Detect asserts are on and swap out the validator to do nothing on preview routes, OR
• Adapt the 'component failed to render' message to include assert is enabled messaging

I would vote for the first option as having to disable assert during development could be silently muting other errors

If the issue is the

          $component = Component::load($component_instance['component']);
          assert($component instanceof Component);

Could we change only this assert with a regular if if we're in preview? Something like this draft code:

          $component = Component::load($component_instance['component']);
          // Avoid asserts in preview with zend.assertions=1 to improve message to users.
          if (!$isPreview) {
            assert($component instanceof Component);
          }
          elseif (!$component instanceof Component) {
            // Or anything else.
            throw new ComponentNotFoundException($component_instance['component'] . ' doesn\'t exist.');
          }

I'm probably missing something but that's the only assert we need to sort, right?

#6: Ah, of course, that is what we missed in 📌 [PP-1] Failing kernel tests for all ways a component source can fail to render on the server side Active and 📌 Improve server-side error handling Active 😬

+1 for the proposal in #6 … except that that will then result in everything looking while to the content author while editing, but will result in a component instance failing to render on the live site? 😅 Or what am I missing?

It will depend on the twig file.

Something like a missing title here

<h1>{{ title }}</h1>

Would be white, but in other cases, there might be a fatal

{{ some_invalid_thing|t }}

We won't get to live site, because validation will prevent publishing

Oh, right. Great, then #6 is a solid plan! 👍 Thanks :)

The 3530795-validation-render branch has an approach that looks like this:

The screenshot was taken before we added support to gracefully handle empty required fields - mentally replace that message with any of the many possible render-crashing errors that can't be handled so smoothly. The design is just rough, the main things to note is the unrenderable component has a clear message in its place, and the guilty prop input is correctly highlighted as invalid.

Thanks, Ben!

Discussed in meeting with @callumharrod, @lauriii and @bnjmnm. There's some more clarity now, but still plenty left to be clarified. @callumharrod is working on expanded designs to demonstrate how this is expected to work:

1. when an invalid prop value is auto-saved for a component instance being edited currently (optimistic rendering whenever possible — for now only one case — see 🐛 Hero component failed to render after removing text Active + 📌 Narrow the conditions under which we allow a prop to be an empty string Active — potentially more in the future)
2. when an invalid prop value is auto-saved for a component instance NOT being edited currently, only being previewed (never optimistically rendered — visually similar to @bnjmnm's #12, but with different styling, and most importantly: with dimensions retrieved from the component's default representation based on example values)
3. showcasing the workflow when opening an auto-saved Page with many component instances of which e.g. 3 have invalid values auto-saved for >=1 prop, including whether the preview canvas should auto-scroll the first invalid component instance into view
4. (not discussed but just realized: the design should visualize what to do when multiple props have invalid values auto-saved)

Is the "preview LIES" part of this issue title still accurate? The way I read the comment linked in #3 is that there are situations where you can publish something while it still have invalid values. If those cases still exist, shouldn't those be separate issues? Meanwhile, if you can't publish with invalid values, then what do we mean by the preview lying?