Conversation
339d1a0 to
6a0431d
Compare
jasonvarga
left a comment
There was a problem hiding this comment.
Nice split, and thanks for the write-up in the description — the stacking note made this easy to read.
This review covers the Cloudflare commit (6a0431d) only. I have separate, more serious findings against the Embed value object and the CoreModifiers rewiring, but those live in ca149d0 and belong on #15458, so I'll raise them there rather than here.
Two bugs in the new provider dropdown and two smaller things below.
| <div class="flex flex-col space-y-3 p-1.5 bg-gray-100 border border-gray-300 dark:bg-gray-900 dark:border-gray-700 rounded-xl"> | ||
| <ui-input-group> | ||
| <ui-combobox | ||
| :model-value="mode" |
There was a problem hiding this comment.
The description says "the stored value is the single source of truth for which input is shown, so the two can't disagree" — but they can, because which input renders is driven by isCloudflare (value-derived) while which option the dropdown displays is bound to mode, which is seeded once from meta.video.provider (server-derived).
Those disagree whenever the stored cloudflare: value is malformed: Embed::fromValue() reports provider unsupported for it, while the JS only checks the prefix. Mounting with value: 'cloudflare:ABC-123' and meta.video.provider: 'unsupported' gives:
isCloudflare→true, so the ID input renders showingABC-123mode→'url', so the dropdown reads URL
This is reachable — isInvalid is only a visual hint and there's no server-side validation, so a malformed ID saves fine and the field comes back in that contradictory state on reload.
Binding to the same source of truth fixes it in every case, since isCloudflare already falls back to mode when there's no value:
| :model-value="mode" | |
| :model-value="isCloudflare ? 'cloudflare' : 'url'" |
With that, mode goes back to being purely the empty-field seed, which is what the comment on it already claims.
| :options="meta.providers" | ||
| option-label="label" | ||
| option-value="value" | ||
| :aria-label="__('Video Provider')" | ||
| @update:model-value="changeMode" | ||
| /> |
There was a problem hiding this comment.
The combobox needs isReadOnly passed through. Both ui-inputs get it, this doesn't, and ui-combobox supports a readOnly prop (resources/js/components/ui/Combobox/Combobox.vue:61) — so nothing stops the user changing the provider on a read-only field, and changeMode() then calls this.update(null).
I confirmed it by mounting this component with config: { visibility: 'read_only' } and a populated YouTube value: isReadOnly is true, and selecting Cloudflare emits update:value → [null]. The field is now dirty and saving persists the wipe. Same applies to visibility: computed fields.
| :options="meta.providers" | |
| option-label="label" | |
| option-value="value" | |
| :aria-label="__('Video Provider')" | |
| @update:model-value="changeMode" | |
| /> | |
| :options="meta.providers" | |
| option-label="label" | |
| option-value="value" | |
| :read-only="isReadOnly" | |
| :aria-label="__('Video Provider')" | |
| @update:model-value="changeMode" | |
| /> |
| }, | ||
|
|
||
| isInvalid() { | ||
| if (this.isCloudflare) return !!this.videoId && !/^[a-zA-Z0-9]+$/.test(this.videoId); |
There was a problem hiding this comment.
Minor, but when this branch fails the message rendered below is statamic::validation.url (line 36), which is "Must be a valid URL." That shows up under the ID input and tells the user to enter a valid URL in a field that isn't one. Worth a dedicated string.
| { | ||
| const CLOUDFLARE = 'cloudflare'; | ||
| const CLOUDFLARE_EMBED_URL = 'https://iframe.cloudflarestream.com/'; | ||
| const CLOUDFLARE_ID_PATTERN = '/^[a-zA-Z0-9]+$/'; |
There was a problem hiding this comment.
PHP's $ matches before a trailing newline; JavaScript's (without the m flag) doesn't. So the fieldtype's isInvalid check rejects cloudflare:abc123\n, but this pattern accepts it and fromValue() then builds https://iframe.cloudflarestream.com/abc123\n — an embed URL with a newline in it.
I couldn't get that to escape the quoted attribute, so I don't think it's exploitable, but this is the validator that actually matters and it's looser than the UI's.
| const CLOUDFLARE_ID_PATTERN = '/^[a-zA-Z0-9]+$/'; | |
| const CLOUDFLARE_ID_PATTERN = '/^[a-zA-Z0-9]+\z/'; |
6a0431d to
f1f0421
Compare
Closes statamic/ideas#1336
The video fieldtype only understands URLs it can rewrite into an embed — YouTube and Vimeo. Cloudflare Stream videos are addressed by ID rather than a page URL, so there's no way to store one.
This adds a provider dropdown to the field. Picking Cloudflare Stream swaps the URL input for an ID input and stores the value as
cloudflare:<id>; the existing URL behaviour is untouched and stays the default. The stored value is the single source of truth for which input is shown, so the two can't disagree. Switching provider on a populated field clears the stored value rather than leaving the form showing something different from what would be saved.Cloudflare IDs are validated as alphanumeric before being interpolated into the iframe URL, and the embed URL is derived from the value rather than round-tripping through the server, so no lookup endpoint is needed.
Builds on #15458 (the
Videovalue object), which is where thecloudflare:parsing and augmentation live — please merge that one first. Split out of #11871; no new dependencies.