Conversation
29f66c6 to
e05f376
Compare
e05f376 to
9e595ca
Compare
jasonvarga
left a comment
There was a problem hiding this comment.
Just on the new commit here — the other points I have are against #15458 and #15459 and I've left them there.
CI is red on this one, and the cause is the new dependency's version constraint rather than anything in the logic. Details inline on composer.json.
One thing to be aware of when you re-run: of the ~50 matrix jobs in the last run, only 6 completed and 2 failed genuinely — the other ~40 were cancelled by fail-fast before they finished. So the failure set below may not be the whole story; worth a clean run once the constraint is bumped.
| "league/glide": "^3.0 || ^4.0", | ||
| "maennchen/zipstream-php": "^3.1", | ||
| "michelf/php-smartypants": "^1.8.1", | ||
| "mpratt/embera": "^2.0", |
There was a problem hiding this comment.
^2.0 resolves as low as 2.0.0, and the offline-resolution behaviour this feature is built on doesn't exist down there.
Embed::fromOembed() leans on Embera's getFakeResponse() rules via Embera::ONLY_FAKE_RESPONSES. I checked that method across the tags: Embera\Provider\Vimeo doesn't have it in 2.0.0, 2.0.5, 2.0.10, 2.0.15, 2.0.20, 2.0.25 or 2.0.30 — it first appears somewhere between 2.0.30 and 2.0.32. Youtube is the same. The constant itself exists all the way back to 2.0.0, so nothing throws; the fake-response path just quietly resolves nothing and falls through to a live request.
That's what P8.3 - L12.* - prefer-lowest - shard 3/4 is tripping over:
Failed asserting that two strings are identical.
-'Vimeo'
+'unsupported'
at tests/Fieldtypes/VideoTest.php:27
It's not only a test-environment problem, though. The PR description's core argument is that YouTube, Vimeo, DailyMotion, Loom, Streamable, Rumble, Coub and Ted cost zero HTTP requests — but that's only true on the newer end of what ^2.0 permits. A site whose lockfile lands on an older 2.0.x pays a network round-trip on every YouTube and Vimeo embed, which is exactly the cost the dependency was justified as avoiding.
| "mpratt/embera": "^2.0", | |
| "mpratt/embera": "^2.0.32", |
Worth confirming the floor per-provider rather than taking 2.0.32 from me — I only bisected Vimeo and Youtube, and all eight of the "offline" providers need getFakeResponse() at whatever minimum you settle on.
|
|
||
| protected static function fromOembed(string $url): self | ||
| { | ||
| if ($video = static::lookup($url, Embera::ONLY_FAKE_RESPONSES)) { |
There was a problem hiding this comment.
Flagging this line as the reason the constraint matters — this is the call that silently degrades to a network request when the installed Embera is too old to have a getFakeResponse() for the provider. No error, no signal, just a slower path and a different result.
Since the zero-request property is load-bearing for the whole dependency argument, it might be worth having it_does_not_make_http_requests_for_offline_providers cover all eight providers rather than just YouTube and Vimeo. That'd turn a future upstream regression into a failing test instead of a silent perf change.
The video fieldtype only understands YouTube and Vimeo, because those are the two providers
CoreModifiers::embedUrl()knows how to rewrite by hand. This adds oEmbed lookup viampratt/emberaso Loom, Wistia, TikTok, Streamable, Rumble, Vidyard, SproutVideo, DailyMotion, Coub, Ted and Bunny work too, resolved through a new CP endpoint as you type.On the dependency, since that was the open question. Embera can resolve a URL to an embed URL from rules built into the library rather than calling the provider's API, so YouTube, Vimeo, DailyMotion, Loom, Streamable, Rumble, Coub and Ted make zero HTTP requests — there's a test asserting that under
Http::preventStrayRequests(). Only Wistia, TikTok, Vidyard and SproutVideo need a request, and those are cached for an hour. So the runtime cost for the common case is nothing; the dependency earns its keep for the long tail.Embera's own HTTP client sets
CURLOPT_SSL_VERIFYPEER => 0and no timeout, so it isn't used —Statamic\Fieldtypes\Video\HttpClientimplements Embera'sHttpClientInterfaceover Laravel'sHttpclient instead, with verification on and 5s connect / 10s total timeouts. Embera's oEmbedhtmlis never rendered: thesrcis extracted, validated as anhttpsURL, and the iframe is built in Vue, so no provider-supplied markup reaches the CP. Lookups are debounced, aborted on change, and discarded if they resolve after the value has moved on.