[PP] FE implementation of Video Prop

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

This issue's title/summary is misleading: ✨ Define JSON Schema $refs for linking/embedding videos and linking documents Active 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. ✨ Add a Video prop type to the Code Component editor Active 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 👍

#9.2

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

I grabbed these videos from Pexels, which specifies they are free to use (although I should point out no specific license is mentioned). Also note I reduced the dimensions and frame rate of the videos before adding to provide a smaller file size.

WFM 👍

Not 100% sure on this, but it seems like the mechanism that accounts for an image not yet having a value hasn't been applied to video. I added an intentionally failing e2e test that reproduces the steps.

To Reproduce: Add video component to layout, then update the Display Width field before adding a video
(This doesn't have to be the display width field - it can be any field in a component instance form with a video prop)

• We get the Component failed to render, check logs for more detail. in the preview
• The logged error is
  

  An exception has  been thrown during the rendering of a template 
      ("[experience_builder:video/video.src] NULL value found, but a  
      string is required. This may be because the property is empty    
     instead of having data present.

• Here's the model being sent in the bad request

There's one scenario where this won't happen: If the prop is optional and a video has been added then removed. In this instance, the empty video will not result in an error because of the way removed media items are handled on optional props. The request no longer attempts to send the default value and instead sends empty ones

(The above is with a temporarily altered Video component so the video prop is not required)

Unassigning myself so a B.E. dev can have a look at #13

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 change 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

👏 to @larowlan for tracking that down. Assigning to @wim-leers here to reflect the existing assignment in the MR to get a +1 on that backend solution.

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

While the purpose was ensuring the 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 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>

… 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 🐛 Component transforms need to be per sourceType, not per component prop Active 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 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 — would still be nice to see happen 😊

Tested changes on branch 0.x , following scenarios :

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: 📌 Invalid prop errors should be detailed in Component preview Active .

Same for the other.