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.
| /** | ||
| * Turn a link that's direct to a video's page into its embeddable equivalent. | ||
| */ | ||
| public static function embedUrl(string $url): string |
There was a problem hiding this comment.
string $url here (and on isEmbeddable() below) makes the modifiers fatal on a null value.
An empty video field augments to null, so {{ video | embed_url }} on a blank field throws:
TypeError: Statamic\Fieldtypes\Video\Embed::embedUrl(): Argument #1 ($url) must be of type string, null given
This one isn't recoverable: Modify::modify() catches Exception, and TypeError is an Error, so it escapes as a 500 rather than a ModifierException. On 6.x both calls returned quietly (null and false respectively).
It isn't limited to video fields either — {{ some_null_var | embed_url }} was previously a harmless no-op and now fatals too.
Making both params ?string with an early return, or guarding in the modifier before delegating, covers it.
|
|
||
| public function toBool(): bool | ||
| { | ||
| return $this->isSupported(); |
There was a problem hiding this comment.
This flips {{ if video }} to false for any URL the class doesn't recognise.
Boolable is honoured by Antlers conditions (PathDataManager.php:969, Environment.php:358), so a field holding a perfectly valid stored value that isn't YouTube/Vimeo/a known file extension/cloudflare: — a self-hosted player page, a Loom or Wistia link, an unusual extension — goes from truthy to falsey. {{ if video }}<iframe …>{{ /if }} silently stops rendering on upgrade, with nothing in the logs.
That's the one I'd most want changed, because it's invisible.
ArrayableString::toBool() — the convention the description cites — is (bool) $this->value, i.e. "is there a value", not "do I recognise it". Matching that and leaving recognition to isSupported() / {{ video:provider }} keeps both questions answerable.
| : static::unsupported($value); | ||
| } | ||
|
|
||
| if (static::isVideoFile($value)) { |
There was a problem hiding this comment.
isVideoFile() running before oembedProvider() misclassifies Vimeo's direct-playback URLs.
They end in .mp4, so they land on provider: file with embedUrl set to the untouched URL — which means the augmented value silently loses ?dnt=1.
This isn't hypothetical: EmbedUrlTest::it_transforms_vimeo_file_links asserts ?dnt=1 gets added to exactly this URL shape, and vimeoEmbedUrl() below carries a dedicated progressive_redirect guard for it. That test still passes here only because it feeds a raw string.
Swapping the order so oembedProvider() is checked first fixes it.
| } | ||
|
|
||
| #[\ReturnTypeWillChange] | ||
| public function jsonSerialize() |
There was a problem hiding this comment.
Worth flagging explicitly since it's easy to miss from the diff: because Value::jsonSerialize() hands the Embed straight through, this changes a video field in the REST API from "https://vimeo.com/123" to {"embed_url":…,"id":…,"provider":…,"url":…}.
Consistent with ArrayableString/ArrayableLink, and a major is the right time — it just needs to be a deliberate, documented change rather than a side effect. Either an upgrade guide entry, or return (string) $this here and keep the structured form to toArray().
|
|
||
| return $url; | ||
| if ($url instanceof Embed) { | ||
| return $url->embedUrl; |
There was a problem hiding this comment.
embedUrl is null for the unsupported provider, so this returns empty where the modifier used to pass the URL through untouched.
EmbedUrlTest::it_leaves_urls_from_unknown_providers_untouched asserts that contract, and passes here only because it feeds a raw string rather than an augmented value.
| return $url->embedUrl; | |
| return $url->embedUrl ?? $url->url; |
| if ($url instanceof Embed) { | ||
| return $url->embedUrl; | ||
| } |
There was a problem hiding this comment.
This makes trackable_embed_url return the non-trackable URL.
$url->embedUrl is the privacy-preserving form — youtube-nocookie.com, and Vimeo with ?dnt=1 — which is precisely what this modifier exists to avoid. The implementation directly below deliberately keeps youtube.com and adds no dnt, so for any augmented field the modifier now does the opposite of its name:
YouTube: https://www.youtube.com/embed/… → https://www.youtube-nocookie.com/embed/…
Vimeo: https://player.vimeo.com/video/… → https://player.vimeo.com/video/…?dnt=1
Running the existing logic below against $url->url instead of short-circuiting gives the right result.
| if ($url instanceof Embed) { | ||
| return $url->isSupported(); | ||
| } |
There was a problem hiding this comment.
isSupported() is true for provider: file, so is_embeddable now returns true for direct video files that previously returned false.
The practical effect is that {{ if video | is_embeddable }}<iframe src="{{ video | embed_url }}"> starts emitting an iframe pointing at a raw .mp4 for self-hosted files.
"Supported" and "embeddable" are different questions — file is the former but not the latter. Checking the provider against the embeddable set, or adding an isEmbeddable() to the value object distinct from isSupported(), keeps them apart.
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.