oEmbed URL resolution does not take multiple endpoints into account

I'm helping to review this while at the core contribution workshop at Drupalcon Seattle

Discussed this with @phjou at DrupalCon. I think, though I'm not certain, that the use case can be addressed by merging the endpoints from the two providers into a single provider, called PeerTube. The provider_url value is not really used by anything; it's just available to source plugins in case they need it. The endpoints are what actually define the availability and discoverability of oEmbed resources, and a single provider can define any number of those.

@phjou said he will try merging the endpoints together and will report back if it works.

@phjou and I walked through this and I think I now understand what the problem is. We were able to get his use case to work, but we did need to make some small modifications to the oEmbed system first.

The problem is that, in multiple places, the oEmbed system assumes that the first endpoint of any given provider is the one that should be used to generate the final resource URL. In a decentralized set-up like PeerTube, this assumption falls apart entirely.

So, our course of action here is that anywhere we do something like reset($endpoints), we should be doing something more like this:

foreach ($endpoints as $endpoint) {
  if ($endpoint->matchUrl($url)) {
    // This is the endpoint we're looking for
  }
}

This is technically a feature request, not a bug, so I'm adjusting the issue as such. I will also adjust the issue summary to more clearly explain what's going on.

I am experiencing the same problem. Facebook provides different endpoints for different schemes. The scheme for embedding videos is second on the list and is thus never called.

The schemes themselves are also patterns that can be on the same domain name, so I don't think the current approach is sufficient.

This should to do it. This is quite different to the previous patches, so if I've misunderstood the scope of this issue, I would be happy to file a new one instead.

Otherwise, let me know and I can update the issue summary with some concrete examples.

Helps to upload the patch too 😅

Hi @phjou, it looks like we both wrote very similar patches, with the same test failures. Any feedback on the test that has already been written? I think it makes sense to combine efforts at this stage.

+++ b/core/modules/media/src/OEmbed/Provider.php
@@ -97,4 +97,23 @@ public function getEndpoints() {
+  /**
+   * For the given media item URL find an endpoint with schemes that match.
+   *
+   * @param string $url
+   *   The media URL used to lookup the matching endpoint.
+   *
+   * @return \Drupal\media\OEmbed\Endpoint
+   *   An endpoint for the URL.
+   */
+  public function getEndpointMatchingUrl($url) {
+    $endpoints = $this->getEndpoints();
+    foreach ($endpoints as $endpoint) {
+      if ($endpoint->matchUrl($url)) {
+        return $endpoint;
+      }
+    }
+    return reset($endpoints);
+  }

I don't know if we should add another method to Provider. I think I'd rather have this logic in UrlResolver, to be honest.

That said, I think we might actually be able to reclassify this issue as a bug, not a feature request. The fact is, the oEmbed specification specifically says that a provider might provide multiple endpoints, and there is nothing I'm aware of that says each endpoint couldn't match different URL patterns. However, our oEmbed implementation utterly disregards this and assumes that the first endpoint is always the right one. From UrlResolver::getResourceUrl():

    $provider = $this->getProviderByUrl($url);
    $endpoints = $provider->getEndpoints();
    $endpoint = reset($endpoints);
    $resource_url = $endpoint->buildResourceUrl($url);

What makes it so certain that the first endpoint is the correct one? This should be doing something similar to what UrlResolver::getProviderByUrl() is doing:

      foreach ($provider_info->getEndpoints() as $endpoint) {
        if ($endpoint->matchUrl($url)) {
          return $provider_info;
        }
      }

So this, IMHO, buts up incredibly close to "bug" territory and should be considered a bug fix for 8.9.x and 9.0.x. If we're lucky, maybe committers would backport it to 8.8.x, too.

1. +++ b/core/modules/media/src/OEmbed/UrlResolver.php
   @@ -159,8 +159,12 @@ public function getResourceUrl($url, $max_width = NULL, $max_height = NULL) {
   +    foreach ($endpoints as $endpoint) {
   +      if ($endpoint->matchUrl($url)) {
   +        $resource_url = $endpoint->buildResourceUrl($url);
   +      }
   +    }
   

   This may accidentally change the behavior of getResourceUrl(). With this, the last matching endpoint will win, not the first. To keep things as backwards-compatible as possible. I think we should move this to a utility function of UrlResolver (maybe a new public static method?) which returns the result of buildResourceUrl() on the first matching endpoint, not the last.

2. +++ b/core/modules/media/src/Plugin/Validation/Constraint/OEmbedResourceConstraintValidator.php
   @@ -110,6 +110,11 @@ public function validate($value, Constraint $constraint) {
   +      foreach ($endpoints as $endpoint) {
   +        if ($endpoint->matchUrl($url)) {
   +          $resource_url = $endpoint->buildResourceUrl($url);
   +        }
   +      }
   

   If we add that utility function, we can call it here to avoid repeating ourselves.

3. +++ b/core/modules/media/tests/src/Unit/ProviderTest.php
   @@ -0,0 +1,40 @@
   +  public function testGetEndpointMatchingUrl() {
   +    $provider = new Provider('Sample provider', 'https://sample', [
   +      [
   +        'schemes' => [
   +          'https://foo/*',
   +          'https://bar/*',
   +        ],
   +        'url' => 'https://first-endpoint',
   +      ],
   +      [
   +        'schemes' => [
   +          'https://baz/*',
   +        ],
   +        'url' => 'https://second-endpoint',
   +      ],
   +    ]);
   +
   +    $this->assertNull($provider->getEndpointMatchingUrl('https://invalid-url'));
   +    $this->assertEquals('https://first-endpoint', $provider->getEndpointMatchingUrl('https://foo/1')->getUrl());
   +    $this->assertEquals('https://first-endpoint', $provider->getEndpointMatchingUrl('https://bar/2')->getUrl());
   +    $this->assertEquals('https://second-endpoint', $provider->getEndpointMatchingUrl('https://baz/3')->getUrl());
   

   I think this will fail now, because we no longer have a getMatchingEndpointUrl() method. But if we have a new utility method of UrlResolver, we can add a test of that to UrlResolverTest.

I believe this addresses the main issues from #27.

...or it would if the patch worked. Take 2, but with a less broken file this time!

(Hopefully) fixed up the failing test. Should actually pass now.
Also worth noting that I didn't add any tests for the change to OEmbedResourceConstraintValidator, but there weren't any tests for it in the first place, and I didn't feel comfortable writing a full new test suite.

Again with the failing patches. I even checked first that time!

P.S. Sorry for any accidental spamming. This is my first real contribution and I'm still getting used to the process.

Add utility function in UrlResolver to avoid repeating code.

Rerolled and made getEndpointMatchingUrl protected, since it's now only used in the UrlResolver.

This is working as expected. Thanks!

OK this makes plenty of sense (although the patch was easier to follow than the issue) - updated the issue summary myself in the end.

Committed 600da9f and pushed to 9.2.x. Also cherry-picked to 9.1.x. Thanks!