Video prop shape: e2e test coverage for video from media library the component instance form + fix revealed bug in evaluating `StaticPropSource`s

"alpha blocker" was a typo. This is a blocker for beta.

This issue's title/summary is misleading: #3524130: Define JSON Schema $refs for linking/embedding videos and linking documents introduces not ONE "video" shape, but 2: one for local, one for remote.

The local one is about to land: [#3524130#21], the remote one won't land today for sure. oEmbed is hard.

As of #3524130-24: Define JSON Schema $refs for linking/embedding videos and linking documents, local video shape matching is working:

Note: the scope of this issue should be limited to local videos, i.e. we should not add support for OEmbed (Youtube / Vimeo) here.

"FE implementation" is ambiguous, so re-titling this issue to clarify its scope. #3534601: Add a Video prop type to the Code Component editor; requires adding `file` as default `StaticPropSource`, `FileWidget` support and various infrastructure improvements is a separate FE issue for the video prop that's a completely separate scope.

This MR looks pretty far along, and the new ui/tests/e2e/media-video-prop.cy.js is passing! 🥳

1. ✅ e2e tests run fine locally and fulfill the purpose. (The default media thumbnail is not loading, but that's fine — it does load on a "real" site, just tested that explicitly.)
2. One small change would make the new test module more widely useful: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
3. Question: what's the source for the 2 videos? Did you create them? Asking for licensing purposes: they're going to be licensed as GPL2+ upon commit (and in principle they already are by having them in the MR). Just hoping to confirm 🤞
4. ✅ I don't quite like xb_test_video_fixture_install(): much of that could be just exported config instead; which would make this usable in recipes, too (and which would then benefit Playwright tests in the future).

   But since the Media and File entities must be created this way, I think it's reasonable and pragmatic 👍

So: RTBC. If you can address 2+3 and get CI to green, let's merge 👍

WFM 👍

I think @larowlan would have some insight into #13. Seems like our handling of "use this example value that's already the prop shape but isn't a Media reference" for images should also work for videos, but apparently something about it isn't.

Got to the bottom of this and it was quite tricky

The difference between image and video is shown here in the shape matching \Drupal\experience_builder\Hook\ShapeMatchingHooks::getFieldTypeProps

return match ($media_source_class) {
      Image::class => [
        'src' => new ReferenceFieldTypePropExpression(
          new FieldTypePropExpression('entity_reference', 'entity'),
          new ReferenceFieldPropExpression(
            new FieldPropExpression(BetterEntityDataDefinition::create('media', $media_type_ids), $source_field_names, \NULL, 'entity'),
            new FieldPropExpression(BetterEntityDataDefinition::create('file'), 'uri', \NULL, 'url')
          )
        ),
        'alt' => new ReferenceFieldTypePropExpression(new FieldTypePropExpression('entity_reference', 'entity'), new FieldPropExpression(BetterEntityDataDefinition::create('media', $media_type_ids), $source_field_names, \NULL, 'alt')),
        'width' => new ReferenceFieldTypePropExpression(new FieldTypePropExpression('entity_reference', 'entity'), new FieldPropExpression(BetterEntityDataDefinition::create('media', $media_type_ids), $source_field_names, \NULL, 'width')),
        'height' => new ReferenceFieldTypePropExpression(new FieldTypePropExpression('entity_reference', 'entity'), new FieldPropExpression(BetterEntityDataDefinition::create('media', $media_type_ids), $source_field_names, \NULL, 'height')),
        // TRICKY: Additional computed property on image fields added by Experience Builder.
        // @see \Drupal\experience_builder\Plugin\Field\FieldTypeOverride\ImageItemOverride
        'srcSetCandidateTemplate' => new ReferenceFieldTypePropExpression(new FieldTypePropExpression('entity_reference', 'entity'), new FieldPropExpression(BetterEntityDataDefinition::create('media', $media_type_ids), $source_field_names, \NULL, 'srcset_candidate_uri_template')),
      ],
      VideoFile::class => [
        'src' => new ReferenceFieldTypePropExpression(
          new FieldTypePropExpression('entity_reference', 'entity'),
          new ReferenceFieldPropExpression(
            new FieldPropExpression(BetterEntityDataDefinition::create('media', $media_type_ids), $source_field_names, \NULL, 'entity'),
            new FieldPropExpression(BetterEntityDataDefinition::create('file'), 'uri', \NULL, 'url'),
            TRUE,
          )
        ),
      ],
      default => [],
    };

If you look at the src expression on both of them, you will see that its largely identical.
So why doesn't our fallback to default values work for video?

Well its purely by coincidence that the Image also has alt, width and height

When we try to evaluate these expressions in \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::clientModelToInput because there's no entity found, we end up in the catch block that falls back to the default value.

But this doesn't happen for Video. But why?

Because Image shape match contains three ReferenceFieldTypePropExpressions that wrap FieldPropExpressions but Video only contains one that wraps ReferenceFieldPropExpression

And if we look into the evaluator it has this code

if ($entity_or_field === NULL) {
      // Entity is optional for reference fields: the reference may point to
      // something or not.
      if ($expr instanceof ReferenceFieldPropExpression) {
        return NULL;
      }
      throw new \OutOfRangeException('No data provided to evaluate expression ' . (string) $expr);
    }

so the $expr instanceof ReferenceFieldPropExpression code here means that for Video, the OutOfRangeException is never thrown and hence the default value handling doesn't work.

To fix this, I added a new option to ReferenceFieldPropExpression, an isRequired flag. I would like @wim leers to confirm my approach here as I'm not totally sure about USV (although I'm not really making use of USV to track the required flag).

Once I add that and check for it, it works as expected. You can find that change in this commit

Apparently it is a deep back-end/shape matching bug that @bnjmnm's e2e test coverage uncovered here 🤯

While the purpose was ensuring the Redux-integrated field widget worked as expected, it actually largely did (which I already expected given how I didn't have to change a single thing to video-selection-from-Media-Library work — see #5), except that Shape matching infrastructure broke it. 👻

EDIT: cross-posted with @bnjmnm 😅

Found one more scenario that hits a bug that causes the sdc.experience_builder.video component to fail to render:

Twig\Error\RuntimeError occurred during rendering of component 8e5cdb12-a631-49b0-afa0-e69887dfdf86 in Content two (-), field field_xb_demo: An exception has been thrown during the rendering of a template ("[experience_builder:video/display_width] Must have a minimum value greater than or equal to 1. The provided value is: "0".") in "experience_builder:video" at line 1.

Steps to reproduce:

1. 🟢 Insert XB's Video component
2. ℹ️ Observe that the client-side returned by /xb/api/v0/config/component for this component contains:
   

       "propSources": {
         "video": {
           "required": true,
           "jsonSchema": {
             "title": "video",
             "type": "object",
             "required": [
               "src"
             ],
             "properties": {
               "src": {
                 "title": "Video URL",
                 "type": "string",
                 "format": "uri-reference",
                 "pattern": "^(/|https?://)?.*\\.([Mm][Pp]4)(\\?.*)?(#.*)?$"
               },
               "poster": {
                 "title": "Image URL",
                 "type": "string",
                 "format": "uri-reference",
                 "pattern": "^(/|https?://)?.*\\.([Pp][Nn][Gg]|[Gg][Ii][Ff]|[Jj][Pp][Gg]|[Jj][Pp][Ee][Gg]|[Ww][Ee][Bb][Pp]|[Aa][Vv][Ii][Ff])(\\?.*)?(#.*)?$"
               }
             }
           },
           "sourceType": "static:field_item:entity_reference",
           "expression": "ℹ︎entity_reference␟{src↝entity␜␜entity:media:video␝field_media_video_file␞␟entity␜*␜entity:file␝uri␞␟url}",
           "sourceTypeSettings": {
             "storage": {
               "target_type": "media"
             },
             "instance": {
               "handler": "default:media",
               "handler_settings": {
                 "target_bundles": {
                   "video": "video"
                 }
               }
             }
           },
           "default_values": {
             "source": [
               
             ],
             "resolved": {
               "src": "https://media.istockphoto.com/id/1340051874/video/aerial-top-down-view-of-a-container-cargo-ship.mp4?s=mp4-640x640-is&k=20&c=5qPpYI7TOJiOYzKq9V2myBvUno6Fq2XM3ITPGFE8Cd8=",
               "poster": "https://example.com/600x400.png"
             }
           }
         },
         "display_width": {
           "required": false,
           "jsonSchema": {
             "type": "integer",
             "minimum": 1
           },
           "sourceType": "static:field_item:integer",
           "expression": "ℹ︎integer␟value",
           "sourceTypeSettings": {
             "instance": {
               "min": 1,
               "max": null
             }
           }
         }
       },
       "transforms": [
         
       ],
   

3. 🟢 Select a video (here: the one with target_id=2) but do not enter a display width, the client will send:

   {
       "source": {
           "video": {
               "expression": "ℹ︎entity_reference␟{src↝entity␜␜entity:media:video␝field_media_video_file␞␟entity␜*␜entity:file␝uri␞␟url}",
               "sourceType": "static:field_item:entity_reference",
               "value": "2",
               "sourceTypeSettings": {
                   "storage": {
                       "target_type": "media"
                   },
                   "instance": {
                       "handler": "default:media",
                       "handler_settings": {
                           "target_bundles": {
                               "video": "video"
                           }
                       }
                   }
               }
           },
           "display_width": {
               "expression": "ℹ︎integer␟value",
               "sourceType": "static:field_item:integer",
               "value": [],
               "sourceTypeSettings": {
                   "instance": {
                       "min": 1,
                       "max": null
                   }
               }
           }
       },
       "resolved": {
           "video": "2",
           "display_width": []
       }
   }
   

   This renders fine.

4. 🔴 Then I remove the selected video, which causes:
   

   {
       "source": {
           "video": {
               "expression": "ℹ︎entity_reference␟{src↝entity␜␜entity:media:video␝field_media_video_file␞␟entity␜*␜entity:file␝uri␞␟url}",
               "sourceType": "static:field_item:entity_reference",
               "value": {
                   "src": "https://media.istockphoto.com/id/1340051874/video/aerial-top-down-view-of-a-container-cargo-ship.mp4?s=mp4-640x640-is&k=20&c=5qPpYI7TOJiOYzKq9V2myBvUno6Fq2XM3ITPGFE8Cd8=",
                   "poster": "https://example.com/600x400.png"
               },
               "sourceTypeSettings": {
                   "storage": {
                       "target_type": "media"
                   },
                   "instance": {
                       "handler": "default:media",
                       "handler_settings": {
                           "target_bundles": {
                               "video": "video"
                           }
                       }
                   }
               }
           },
           "display_width": {
               "expression": "ℹ︎integer␟value",
               "sourceType": "static:field_item:integer",
               "value": [
                   {
                       "value": ""
                   }
               ],
               "sourceTypeSettings": {
                   "instance": {
                       "min": 1,
                       "max": null
                   }
               }
           }
       },
       "resolved": {
           "video": {
               "src": "https://media.istockphoto.com/id/1340051874/video/aerial-top-down-view-of-a-container-cargo-ship.mp4?s=mp4-640x640-is&k=20&c=5qPpYI7TOJiOYzKq9V2myBvUno6Fq2XM3ITPGFE8Cd8=",
               "poster": "https://example.com/600x400.png"
           },
           "display_width": [
               {
                   "value": ""
               }
           ]
       }
   }
   

   Note how display_width's raw widget value is passed: value: "", which matches <input data-drupal-selector="edit-xb-component-props-189b4371-3fc7-4b4e-bb98-e5fe9a4c291d-display-width-0-value" data-form-id="component_inputs_form" type="number" id="edit-xb-component-props-189b4371-3fc7-4b4e-bb98-e5fe9a4c291d-display-width-0-value" name="xb_component_props[189b4371-3fc7-4b4e-bb98-e5fe9a4c291d][display_width][0][value]" step="1" min="1" placeholder="" class="_root_1or6m_1 form-number" value>

5. ❌ Which results in the dreaded Component failed to render, check logs for more detail. fallback.

… which looks an awful lot like client-side transforms aren't being applied. Which … they aren't, because they're absent from the API response in step 1 — for every component 😅

Ah … but … it's #3515629: FieldWidget's XB transforms must be bubbled by the Field Widget rendering to inform the client that made it so, and moved that to #attached['xb-transforms'] + respecting that in \Drupal\experience_builder\Render\MainContent\XBTemplateRenderer::renderResponse().

To be determined where the root cause of that lies, but this is unfortunately reproducibly broken. The e2e test can and should be expanded to remove the video, check that it renders, then re-add it and continue the remainder of the test (where it sets display width 190 etc).

From a https://en.wikipedia.org/wiki/Robustness_principle POV, the server-side should gracefully handle invalid values received by the client that the field type cannot make sense of (FieldItemListInterface::filterEmptyItems()), so … adding a work-around for the above problem, and triggering a deprecation error, to allow us to figure us out the root cause after beta1 👍

So after the crazy #20 detour, time to properly answer @larowlan's #17:

To fix this, I added a new option to ReferenceFieldPropExpression, an isRequired flag. I would like @wim leers to confirm my approach here as I'm not totally sure about USV (although I'm not really making use of USV to track the required flag).

1. This was missing the corresponding update to FieldObjectPropsExpression, which itself uses ReferenceFieldPropExpression. This is totally possible to make work: I got it to green.
2. To keep the expressions consistent, we should also update ReferenceFieldTypePropExpression, even though for data integrity reasons, it's already required that a StaticPropSource is non-empty (see assert($field_item_list->count() === 1); in \Drupal\experience_builder\PropSource\StaticPropSource::isMinimalRepresentation()). This will require widespread changes.
3. The
   

         // Entity is optional for reference fields: the reference may point to
         // something or not.
         if ($expr instanceof ReferenceFieldPropExpression) {
           return NULL;
         }
   

   code is positively ancient — I never expected it to survive to this day: it was a prototype to throw away/refactor! 😅 It dates back to >14 months ago, before we even were using the issue queue and MRs.

4. Finally: it's very easy to make a mistake somewhere along the way in the expression. HEAD contains a bug. Specifically, it's very easy to forget to make one reference in a chain required. Lee's original commit forgets this too, because it results in:
   

   ℹ︎entity_reference␟{src↝entity␜␜entity:media:baby_videos|vacation_videos␝field_media_video_file|field_media_video_file_1␞␟entity␜*␜entity:file␝uri␞␟url}
   

   and not
   ℹ︎entity_reference␟{src↝entity␜*␜entity:media:baby_videos|vacation_videos␝field_media_video_file|field_media_video_file_1␞␟entity␜*␜entity:file␝uri␞␟url}

So … trying a completely different approach: rather than modifying our StructuredDataPropExpressionInterfaces themselves, with not only LOTS of changes throughout the codebase, but also makes every hook_storage_prop_shape_alter() implementation more brittle. Furthermore … it'd require changing that hook, because it currently does not distinguish between required or not! That'd also mean potentially doubling every CandidateStorablePropShape: once for required, once for optional.

Turns out … that works too 😊 commit + update a few calls I missed = green, and I'll be able to revert lots of (most?) changes.

💡 All this led to the lightbulb moment that that code I quoted in point 3 above is truly positively ancient and should be generally applying, rather than special-casing ReferenceFieldPropExpression. Just did that, and then spent (too much) time figuring out the 2 failures: it was because of optional props with examples that need DefaultRelativeUrlPropSource … 😬 made that much more explicit.

That was green, so finally reverted all the prop expression changes: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

All green, more explicit, less change (if we'd have had to push this to completion, then all version hashes would change too, not to mention the last point), thoroughly manually tested.

I'm RTBC'ing and merging this as soon as it's green, because it A) makes @bnjmnm's new e2e test pass, B) it's totally fine for @larowlan to want to do a follow-up MR, C) it's totally fine for @bnjmnm to add the test coverage for #20 in a follow-up MR.

@larowlan in chat:

that sounds much cleaner, thanks

Yay, that answers B) it's totally fine for @larowlan to want to do a follow-up MR — would still be nice to see C) it's totally fine for @bnjmnm to add the test coverage for #20 in a follow-up MR. happen 😊

Tested changes on branch 0.x , following scenarios :

Scenario	Result	Status
Add Video component to page Add .mp4 media file Keep display width as 300 / 010 less than the page width Preview page Publish Page	On xb edit page video width is 300 /10 On preview of page video width is 300 / 10 After publish , on page video width is 300 / 10	PASS
Add Video component to page Add .mp4 media file Keep display width as 1600 , that is more the page width Preview page Publish Page	After publish , on page video width is 1570 (that is page width)	PASS
Add Video component to page Add .mp4 media file / edit existing video display width Keep display width as 0 (type 0 / backspace all on all the number in field and type 0 ) Preview page Publish Page	Warning message : Value must be greater than or equal to 1 Text box outline = red Error message : X data must be >= 1 On navigating away 0 value is removed and display width is empty on preview on page video width is 30 After publish , on page video width is empty , and set according to the page size (902)	FAIL
Set display width to a value that is less than 0 Manually type 0 , click on down arrow Manually type “-1“ type 1 , then move cursor before 1 and then add -	No change to the value present in text box Cannot add character “-“ to a number textbox it accepts “-1“ as value but on publish video width is 1	FAIL (for 3rd point)
Add video component to page , do not add any video src or display width , publish page Add video component , add media publish changes , navigate back to xb editor page and remove media and publish changes	On adding video component autosave is triggered , where video src is the default one as mentioned in .yml file and width is empty	PASS
To a page add two column component ,in column 1 add heading and in column 2 add video , add media ,set display width and publish		PASS

Thanks for the detailed report!

• Error message : X data must be >= 1
• On navigating away 0 value is removed and display width is empty
• on preview on page video width is 30
• After publish , on page video width is empty , and set according to the page size (902)

This is a known problem. The preview then does not match (width 30) the entered value (0), which then post-publish indeed results in something the author did not see in the preview. The issue to change that: #3530795: [needs design] Invalid prop errors should be detailed in Component preview; otherwise the preview LIES 😱.

Same for the other.

Removing the beta-blocker tag as discused with Alex.