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

Created on 24 February 2021, over 3 years ago
Updated 21 February 2023, almost 2 years ago

Vimeo links return the title when getName() is called in drupal/modules/contrib/video_embed_field/src/Plugin/video_embed_field/Provider/Vimeo.php

Would like to add that to YouTube and YouTubePlaylists.

๐Ÿ“Œ Task
Status

Needs review

Version

2.0

Component

Video Embed Field (base module)

Created by

Live updates comments and jobs are added and updated live.
  • Accessibility

    It affects the ability of people with disabilities or special needs (such as blindness or color-blindness) to use Drupal.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland sokru

    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 ๐Ÿ“Œ Accessibility enhancements for html output of iframe tag (title, id, name, inner A link) Needs review . 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.

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland sokru

    And caching the response.

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland sokru

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

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland sokru

    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.

  • Status changed to RTBC over 1 year ago
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada RobLoach Earth

    Thanks for the fixes +1

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland sokru

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

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland sokru

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

  • Status changed to Needs review over 1 year ago
  • ๐Ÿ‡ซ๐Ÿ‡ทFrance dydave

    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.

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland sokru

    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.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium herved
    +++ 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?

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium herved

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

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium herved

    wrong interdiff in #23...

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium herved

    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.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium herved

    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.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom james.williams

    james.williams โ†’ made their first commit to this issueโ€™s fork.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom james.williams

    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 'Vimeo | 12345'. Perhaps the title could be made configurable and/or skipped over if it wasn't retrieved from the oembed data?

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom james.williams

    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.

  • Pipeline finished with Failed
    22 days ago
    Total: 157s
    #326920
  • Pipeline finished with Failed
    22 days ago
    Total: 185s
    #326933
  • Pipeline finished with Failed
    22 days ago
    Total: 171s
    #326958
  • Pipeline finished with Failed
    22 days ago
    Total: 167s
    #326992
  • Pipeline finished with Failed
    22 days ago
    Total: 160s
    #327038
  • Pipeline finished with Failed
    22 days ago
    Total: 167s
    #327041
  • Pipeline finished with Failed
    22 days ago
    Total: 160s
    #327054
  • Pipeline finished with Failed
    22 days ago
    Total: 169s
    #327058
  • Pipeline finished with Failed
    22 days ago
    Total: 170s
    #327068
  • Pipeline finished with Failed
    11 days ago
    Total: 157s
    #336188
  • Pipeline finished with Failed
    11 days ago
    Total: 296s
    #336278
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom james.williams

    Tests are finally passing via phpunit :)

Production build 0.71.5 2024