Add "title" attribute to YouTube, Vimeo, and Playlist embeds

This is good, but can it be cached so we don't have to hit YouTube's servers every time we render the page?

When fetching data via HTTP, modules should be done with Guzzle. See #3062387: Do not use file_get_contents() do download stuff. Use Drupal's http client (Guzzle) to do that, for better error handling for an example patch.

Updated patch and some other cleanup.

Cleaner patch, with coding standard updates and the new member function in the test mock object.

Another run.

There is related effort happening here as well: https://www.drupal.org/project/video_embed_field/issues/3253820

Attached a patch that sets iframe title attribute with the help of the new methods.

The updated patch needs review.

The updated patch builds upon the above to add the title to both Vimeo and YouTube Playlists too.

Based on patch from #11, but I made few small improvements:
1. No new translation strings, since there's almost 80k installations and some/many of them might have made translations.
2. Removed unrelated coding standard fixes, so its easier to review and get committed.
3. Corrected the return type of oEmbedData()

Added the related issue #2913598: Accessibility enhancements for html output of iframe tag (title, id, name, inner A link). Patch on issue has some related/duplicate code, but it has two interesting features that might be left for follow-up:
- Transliterates and removes some unwanted characters from the title.
- Adds $instace_id argument to renderEmbedCode, so title's will be unique if there's multiple instances of same video on page.

And caching the response.

Added the tests and removed single undefined function (filterCharaters()) from patch on #13.

This fixes the tests other than ProviderUrlParseTest. That would need updating mocks. I'm willing to do that, if that increases the changes of commit.

Thanks for the fixes +1

-

And small improvement by handling the cases if returned json is not valid.

Watchout: #11 is the last patch supported on PHP:7.4.

All the more recent patches contributed afterwards (#12 to #18) use the null safe operator (?->) which is supported from PHP 8 onwards, therefore crashes on PHP 7.4, with the following error:

The website encountered an unexpected error. Please try again later.
ParseError: syntax error, unexpected '->' (T_OBJECT_OPERATOR) in Composer\Autoload\{closure}() (line 110 of modules/contrib/video_embed_field/src/Plugin/video_embed_field/Provider/YouTube.php).

It would be great if we could get a more recent version of the patch compatible with 7.4, but that's not necessarily a priority,

Otherwise, patch from #18 works great on PHP 8.1.

Thanks again to everyone for the great help with this ticket and contributions.

Small change to decrease severity of logging. This could be debated, but the reasoning is that it will produce noise to logs (eg. when forwarding only emergency/critical/error messages to log service) everytime private video is rendered on cold caches.

+++ b/src/ProviderPluginBase.php
@@ -103,6 +103,42 @@ abstract class ProviderPluginBase extends PluginBase implements ProviderPluginIn
+        $data = json_decode($body);

I think it would be best to use \Drupal\Component\Serialization\Json::decode

+++ b/src/Plugin/video_embed_field/Provider/Vimeo.php
@@ -48,11 +49,12 @@ class Vimeo extends ProviderPluginBase {
+    $url = sprintf("https://vimeo.com/api/oembed.json?url=%s", $this->getInput());

Shouldn't we use Url::fromUri(...)->toString(), so the url parameter is properly encoded?

Whoops, discard patch #22, I forgot point 2.
This implements both points.

wrong interdiff in #23...

I forgot to update Vimeo::getRemoteThumbnailUrl... This should work.
It also addresses #20 I believe (PHP 7.4 compatibility) but I have not tested that to confirm.
Attaching new interdiff, still from 21.

I have 2 more suggestions:

1. Normalize URLs before requesting the oembed endpoint
Most providers accept multiple URL patterns, but this isn't necessarily true when requesting the oembed data.
I noticed that many providers only accept some URL patterns when passed to the oembed endpoint.
And unlike core's Media oembed, video_embed_field doesn't validate that the given URL returns oembed data, before saving/storing data.
It is relevant for getting Youtube playlist title, e.g:
- Raw URL (we get the video title): https://www.youtube.com/oembed?url=https%3A%2F%2Fwww.youtube.com%2Fwatch...
- Normalized (we get the playlist title): https://www.youtube.com/oembed?url=https%3A%2F%2Fwww.youtube.com%2Fplayl...

Also, normalizing would also prevent unnecessary calls to the endpoint for minor/irrelevant URL changes (e.g. http/https, www/non-www, query strings changing, etc).
This is especially relevant when combined with other patches accepting more patterns, such as #3060201: Youtube Privacy Enhanced Mode (using -nocookie.com) (8.x-1.x).
I have added some more test cases.

2. Small change to title format
What about normalizing the titles to "@provider | @title" instead of "@provider Video (@title)".
The video title often contains parentheses, and some providers are not necessarily videos.

I've spun up a merge request for the most recent patch. However, I'm no fan of the title format, especially as it is forced even when no title could be retrieved from the oembed data - which can make it look something like 'YouTube | tgeXmUkMB9I'. Perhaps the title could be made configurable and/or skipped over if it wasn't retrieved from the oembed data?

OK - see my last commit; the format of the title is now configurable (which avoids adding a new translatable string too), and there's a configuration option to avoid falling back to using the video ID. So by default, titles will come out using that '@provider | @title' format, but those of us who might want something else can configure that to '@title' and untick the box to use a fallback at all.

Tests are finally passing via phpunit :)

Could we have an MR against the 3.0.x-dev branch?

As it is still in alpha version, we could merge it without taking too many risks.

Thanks! But I see from #3483205-5: Drupal 11 compatibility fixes for Video Embed Field that the video_embed_wysiwyg module has gone from the 3.0.x branch. Which direction would you like to head in?

1. Merge this MR for 8.x-2.x, with a view to merging 8.x-2.x into 3.0.x later (alongside other improvements currently in 8.x-2.x but not 3.0.x)
2. Postpone further work on this issue until video_embed_wysiwyg for 3.0.x is ready (😢 my least favourite option, personally!)
3. Make a version of the current MR from this issue which doesn't include the work for video_embed_wysiwyg, for merging into 3.0.x (and perhaps split out a follow-up for this issue's work for a later 3.0.x version of video_embed_wysiwyg)

I guess it depends on where plans for video_embed_wysiwyg, and your support for 8.x-2.x, are at. If video_embed_wysiwyg isn't going to make a return, we may as well just do option 3. Though you still have plenty of us using 8.x-2.x who might like this functionality in that branch too please!

Option 3 looks like a good start for me.

Ah I see some of the Vimeo videos used in tests were updated, so I've had to update the expected values too. And it appears that Vimeo only provides limited functionality for channels in the EU now (see https://www.reddit.com/r/vimeo/comments/1gga3gf/comment/luqd62h/), so I've removed the test for that as it fails in the GitLab pipeline.

MR !67 is now ready for 3.0x, with tests passing :)

Thanks for your great merge-request @james.williams!

Looks like some phpcs and phpstan warnings are still remaining.

And to avoid any BC breaking changes, could we try to make all the new method parameters optional if possible?

OK, all done, and the checks are now fully passing :)

Thanks!

Let's merge it and see if things break 😉 🤞

@james.williams looks like a rebase is needed.

Merged. Thanks @james.williams!

@james.williams what is probably missing is a little update to the README file.

Users will appreciate a bit of documentation about that important new a11y feature.

I don't see any readme file in the 3.0.x branch (or even the 8.x-2.x branch). It feels a bit beyond the scope of this ticket to start one?

Will try to initialize something.

@james.williams looks like we broke things finally 😉 #3513506: Youtube playlist error because of incorrect regex

That regex didn't change in this ticket's work, and I don't see where this work might have called it more, so I'm not sure that is related?

That's not about the regexp but the now broken video_embed_echo360 module which seems to override the renderEmbedCode method.

Not sure there is a way to fix that though. But potentially a lot of module inheriting ProviderPluginInterface::renderEmbedCode are probably broken now and will need an update.

We are on an alpha version of a new major version so we are expecting things to break.

But let's try to keep it as low as possible.

Makes sense. Shall we solve any further issues in a follow-up and close this one off now?

Let's close it, I don't think we will backport this on 2.x branch.

> I don't think we will backport this on 2.x branch.

A backport to the 2.x branch would be welcome. Should I open a new ticket to patch the 2.x branch or add a patch to this issue for 2.x?

@chrissnyder MR 42 is probably what you are looking for.