- Issue created by @bnjmnm
- 🇺🇸United States effulgentsia
"alpha blocker" was a typo. This is a blocker for beta.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
As of #3524130-24: Define JSON Schema $refs for linking/embedding videos and linking documents → , local video shape matching is working:
- 🇫🇮Finland lauriii Finland
Note: the scope of this issue should be limited to local videos, i.e. we should not add support for OEmbed (Youtube / Vimeo) here.
- 🇺🇸United States effulgentsia
"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.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
This MR looks pretty far along, and the new
ui/tests/e2e/media-video-prop.cy.js
is passing! 🥳- ✅ 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.)
- One small change would make the new test module more widely useful: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
- 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 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
andFile
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 👍
- 🇺🇸United States bnjmnm Ann Arbor, MI
#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.
- 🇺🇸United States bnjmnm Ann Arbor, MI
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)
- We get the
- 🇺🇸United States effulgentsia
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.
- First commit to issue fork.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
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 hasalt
,width
andheight
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 thecatch
block that falls back to the default value.But this doesn't happen for Video. But why?
Because Image shape match contains three
ReferenceFieldTypePropExpression
s that wrapFieldPropExpression
s but Video only contains one that wrapsReferenceFieldPropExpression
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, theOutOfRangeException
is never thrown and hence the default value handling doesn't work.To fix this, I added a new option to
ReferenceFieldPropExpression
, anisRequired
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
- 🇺🇸United States bnjmnm Ann Arbor, MI
👏 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.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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 😅
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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:
- 🟢 Insert XB's
Video
component - ℹ️ 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": [ ],
- 🟢 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.
- 🔴 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 afterbeta1
👍 - 🟢 Insert XB's
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
So after the crazy #20 detour, time to properly answer @larowlan's #17:
To fix this, I added a new option to
ReferenceFieldPropExpression
, anisRequired
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).- This was missing the corresponding update to
FieldObjectPropsExpression
, which itself usesReferenceFieldPropExpression
. This is totally possible to make work: I got it to green. - To keep the expressions consistent, we should also update
ReferenceFieldTypePropExpression
, even though for data integrity reasons, it's already required that aStaticPropSource
is non-empty (seeassert($field_item_list->count() === 1);
in\Drupal\experience_builder\PropSource\StaticPropSource::isMinimalRepresentation()
). This will require widespread changes. - 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.
- 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
StructuredDataPropExpressionInterface
s themselves, with not only LOTS of changes throughout the codebase, but also makes everyhook_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 everyCandidateStorablePropShape
: 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 needDefaultRelativeUrlPropSource
… 😬 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.
- This was missing the corresponding update to
-
wim leers →
committed fd0f5886 on 0.x authored by
bnjmnm →
Issue #3528396 by wim leers, bnjmnm, larowlan: Video prop shape: e2e...
-
wim leers →
committed fd0f5886 on 0.x authored by
bnjmnm →
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@larowlan in chat:
that sounds much cleaner, thanks
Yay, that answers — would still be nice to see happen 😊
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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.