Conversation
62b7e0b to
ca149d0
Compare
jasonvarga
left a comment
There was a problem hiding this comment.
Nice direction — a provider discriminator and a first-class embed_url is the right shape, and {{ video:provider }} / {{ video:embed_url }} work well. The move of the URL rewriting onto the value object is a good de-duplication too.
The problem is that augment() now returns the object for every value, and several modifier and template paths behave differently for an actual video field than they do for the equivalent raw string. The new tests only ever exercise the string path, so CI is green while the field itself regresses.
I verified this by running a probe against this branch, then reverting the three source files to the merge base (ffbecaa9) and running the identical probe:
| Template | 6.x | This PR |
|---|---|---|
{{ empty_video | embed_url }} |
null |
uncaught TypeError |
{{ empty_video | is_embeddable }} |
false |
uncaught TypeError |
{{ if video }} where value is https://example.com/nope |
TRUE |
FALSE |
{{ video | embed_url }}, unsupported provider |
https://example.com/nope |
'' |
{{ video | trackable_embed_url }}, YouTube |
https://www.youtube.com/embed/… |
https://www.youtube-nocookie.com/embed/… |
{{ video | trackable_embed_url }}, Vimeo |
https://player.vimeo.com/video/… |
…?dnt=1 |
{{ video | embed_url }}, Vimeo progressive_redirect .mp4 |
…?dnt=1&loc=… |
…?loc=…, no dnt |
{{ video | is_embeddable }}, direct .mp4 |
false |
true |
{{ video }}, {{ if video == "…" }} and the GraphQL output are all unchanged, so the __toString strategy is holding up — it's the modifier short-circuits and toBool() that need another look. Details inline.
One thing not covered inline: Value::jsonSerialize() returns the Embed, so a video field in the REST API changes from "https://vimeo.com/123" to {"embed_url":…,"id":…,"provider":…,"url":…}. There's precedent for that (ArrayableString, ArrayableLink both do it) and a major is the right place for it, but it'll need an upgrade guide entry since the description only discusses the template surface.
On the open question in the description — returning the object only for cloudflare: values wouldn't help much. The null, unsupported and file paths below are the sharp edges, and they'd all still be there.
Also, the description refers to the value object as Statamic\Fieldtypes\Video\Video; it's …\Video\Embed.
ca149d0 to
cdbcc04
Compare
jasonvarga
left a comment
There was a problem hiding this comment.
Thanks — that's a thorough set of fixes, and all seven are confirmed. I re-ran the same before/after probe against 81c399a:
| 6.x | 81c399a | |
|---|---|---|
{{ empty_video | embed_url }} |
null |
null ✅ |
{{ empty_video | is_embeddable }} |
false |
false ✅ |
{{ if video }}, unrecognised URL |
TRUE |
TRUE ✅ |
{{ video | embed_url }}, unsupported |
the URL | the URL ✅ |
{{ video | trackable_embed_url }} |
trackable | trackable ✅ |
{{ video | embed_url }}, Vimeo progressive_redirect |
…?dnt=1 |
…?dnt=1 ✅ |
{{ video | is_embeddable }}, direct .mp4 |
false |
false ✅ |
| API value | string | string ✅ |
ParityTest is the right answer to how these got through in the first place — asserting the augmented value agrees with the raw string across every modifier and every value shape closes the gap structurally, rather than one case at a time. Separating isEmbeddable() from isSupported() came out well too.
One new thing, in the length fix. Details inline — it's a one-line narrowing, and there's a tidier version of it worth considering.
Locally: 99/99 on this PR's suites, and 2302 passing across tests/Modifiers, tests/Fieldtypes and tests/Antlers.
| // Value objects like ArrayableString are both Arrayable and Stringable. | ||
| // They stand in for a string, so measure the string, not the array. | ||
| if ($value instanceof Arrayable && ! $value instanceof Stringable) { |
There was a problem hiding this comment.
The comment scopes this to value objects, but Stringable is auto-implemented by PHP 8 on anything declaring __toString() — so the guard catches a good deal more than the value objects it's aimed at.
In src/, the non-Countable classes that are both Arrayable and Stringable are ArrayableString, LabeledValue, ArrayableLink and Embed — plus Asset, Entries\Collection, Taxonomy, AssetContainer and Blueprint.
Measured on the last of those:
{{ … | length }} |
6.x | this PR |
|---|---|---|
Asset |
21 (field count) |
17 (strlen of url()) |
Collection |
2 |
4 (strlen of handle) |
AssetContainer |
7 |
4 (strlen of handle) |
For the value objects the change is right — {{ code_field \| length }} measuring the code beats counting ['value', …]. For assets and collections it's neither intended nor an improvement: the character count of an asset URL is no more meaningful than the field count was, and it changes silently. LengthTest only covers the ArrayableString case, so nothing catches it.
Narrowing the guard to the value-object family fixes it:
| // Value objects like ArrayableString are both Arrayable and Stringable. | |
| // They stand in for a string, so measure the string, not the array. | |
| if ($value instanceof Arrayable && ! $value instanceof Stringable) { | |
| // Value objects like ArrayableString are both Arrayable and Stringable. | |
| // They stand in for a string, so measure the string, not the array. | |
| if ($value instanceof Arrayable && ! $value instanceof ArrayableString && ! $value instanceof Embed) { |
Though that second instanceof hints at the tidier fix: if Embed extended ArrayableString — the convention the description cites anyway — the guard would just be ! $value instanceof ArrayableString, and you'd get value(), the ArrayAccess implementation and Statamic\View\Blade\value() unwrapping for free instead of hand-rolling them. Up to you whether that's in scope here, but it's what this guard is working around.
The
videofieldtype doesn't implementaugment(), so the raw string is all a template ever gets. There's no provider discriminator, and no first-class way to get an embed URL —{{ video_field | embed_url }}works only becauseCoreModifiers::embedUrl()happens to know how to rewrite YouTube and Vimeo URLs by hand.This augments to a
Statamic\Fieldtypes\Video\Videovalue object exposingprovider,id,urlandembed_url, following theArrayableString/ArrayableLinkconvention:__toString()returns the original value, so{{ video_field }}is unchanged, and it implementsArrayable,ArrayAccess,BoolableandJsonSerializable. It recognises YouTube, Vimeo, direct video files (viaFileTypes::video()) andcloudflare:<id>values, falling back to anunsupportedprovider.The YouTube/Vimeo URL→embed logic is moved out of
CoreModifiers::embedUrl()onto the value object, and the modifier now delegates to it — one implementation rather than two.embedUrl,trackableEmbedUrlandisEmbeddablealso accept the object directly. The existing modifier tests pass unchanged, which is the regression proof for the move.Worth a look:
augment()returns the object for all values, including plain URLs.__toString()covers direct output, modifiers and GraphQL (the existing GraphQL test still passes unchanged), but a strictis_string()check downstream would now see an object. Happy to keep plain URLs as strings and only return the object forcloudflare:values if you'd rather not change that surface in a major.Split out of #11871 so the augmentation contract can be settled on its own, ahead of the Cloudflare Stream and oEmbed work. No new dependencies.