Provide hook_oembed_providers_alter()

Updating version to 8.8.x-dev. Patch is coming.

Attached patch provides hook_oembed_providers_alter(). Tests have been updated.

I don't know if we want to have both #2999018: Expose oEmbed provider URL setting in the Media configuration form and this hook. If you can provide your own providers.json, do you really need this hook? The only "advantage" the hook provides is that you can alter the providers dynamically depending on various conditions -- sorta (they are strongly cached). But that goes against what the provider data is for; it's meant to be static database of oEmbed providers you want to support. An alter hook for this data does not, IMHO, make a lot of sense.

For now, I'll leave this issue open, but I'm in favor of closing this one out. If there is some other justification for adding this hook, I'd like a framework manager to weigh in on those potential pros and cons.

Re #8, the primary benefit of using hook_oembed_providers_alter() to add a provider is that it's a maintenance-free solution. If you provide your own providers.json file, then you are [probably] forking the authoritative source. Once you've done that, now you must periodically merge in any changes from upstream. With hook_oembed_providers_alter(), you never have to revisit it.

I agree with the alter hook is more flexible than #2999018: Expose oEmbed provider URL setting in the Media configuration form and it also avoids exposing a setting a lot of users might not understand. So +1 from me for closing #2999018: Expose oEmbed provider URL setting in the Media configuration form as a duplicate and use an alter hook instead.

The approach has been signed off by a subsystem maintainer, so removing that tag.

I'm happy to make whatever changes come up during code review :-)

The patch in #4 is working for us. I am able to add our instance of kaltura mediaspace to the list of approved providers.

We are using #4 as well for the University of Iowa's Kaltura/Mediaspace.

@joewhitsitt I'm having issues with pulling down a proper thumbnail from Kaltura/Mediaspace (see https://www.drupal.org/project/drupal/issues/3080666). Are you having similar issues? Did you figure out a work around?

Tests still pass on 8.9.x. Per #14 and #15, setting to RTBC.

Sorry to kick this back, but it looks like it still needs a change record before it can be committed. :(

Change record added: Added hook_oembed_providers_alter() to allow oEmbed provider list to be altered

Could the change record get a bit more detail about the hook? For example, we should state that it runs before providers are cached, and that it operates on raw provider information, before they are converted to Provider value objects. Also, maybe we want to mention that one could consult media.api.php for further information. While I'm at it, I'd also like to expand the doc comment in that file so that we state everything I just mentioned, and examples of why you'd want to implement this hook.

I'm really sorry to keep moving the goal posts on this one, but we're adding a whole new hook here and it's important that it be properly documented. There's a good chance committers would raise these concerns as well.

Updated patch with modified doc block per #21 (also caught a typo). Change record is updated.

I like that. Restoring RTBC.

Would it be possible to back port this patch to 8.8.x or has that ship sailed?

This is a feature request, so I don't think it will be backported.

@Chris Matthews - We're running 8.8.1 with patch #22. It's fairly self-contained in what it does, so the only risk, from our point of view, is that it needs rerolled at some point between 8.8.1 and 8.9.0.

Setting back to RTBC. That test failure is random. See #3103492: Random fail in WidgetUploadTest.

1. +++ b/core/modules/media/media.api.php
   @@ -37,6 +37,36 @@ function hook_oembed_resource_url_alter(array &$parsed_url, \Drupal\media\OEmbed
   + * The provider list is a pre-cache array of oEmbed providers. This hook fires
   + * before \Drupal\media\OEmbed\Provider objects are generated from the
   + * provider list.
   

   +1 on pointing out that altering occurs pre-cache. I think we should point out that implementations need to avoid anything dynamic.

   However given the need seems to be to add new providers I'm wondering whether an alter hook is really the best way forward here. There are a couple of ways we could go forward - we could implement some yaml parsing thing to add new providers or we could say that modules such as Peer Tube should be encouraged to decorate the service by doing something like:

     peertube.media.oembed.provider_repository:
       # Override the media.oembed.provider_repository to add peer tube.
       decorates: media.oembed.provider_repository
       class: Drupal\peertube\ProviderRepository
       public: false
       arguments: ['@peertube.media.oembed.provider_repository.inner']
   

   And then Drupal\peertube\ProviderRepository gets the info from the inner service and adds its own.

   Is there a (usual) case for altering the existing list of providers rather than just adding?

2. +++ b/core/modules/media/media.services.yml
   @@ -12,7 +12,7 @@ services:
   -    arguments: ['@http_client', '@config.factory', '@datetime.time', '@cache.default']
   
   +++ b/core/modules/media/src/OEmbed/ProviderRepository.php
   @@ -59,9 +69,10 @@ class ProviderRepository implements ProviderRepositoryInterface {
   -  public function __construct(ClientInterface $http_client, ConfigFactoryInterface $config_factory, TimeInterface $time, CacheBackendInterface $cache_backend = NULL, $max_age = 604800) {
   +  public function __construct(ClientInterface $http_client, ConfigFactoryInterface $config_factory, ModuleHandlerInterface $module_handler, TimeInterface $time, CacheBackendInterface $cache_backend = NULL, $max_age = 604800) {
   

   Adding an argument in the middle of a constructor is hard from an BC point of view.

We're using hook_oembed_providers_alter() to add a custom provider. We're not modifying providers. There may be a valid case for modifying a provider; however, someone else will need to jump in with that use case.

There are a couple of ways we could go forward - we could implement some yaml parsing thing to add new providers or we could say that modules such as Peer Tube should be encouraged to decorate the service

Re service decoration, this makes sense from a CS standpoint; however, this feature becomes accessible to only senior developers at that point.

Re parsing yaml, this doesn't seem like an unreasonable approach. It remains accessible to junior developers. Are you thinking something like 'my_module.media.oembed.yml'?

my_module.unl_media_hub: 
  provider_name: 'UNL MediaHub'
  provider_url: 'https://mediahub.unl.edu'
  endpoints: 
    - 
      schemes: 
        - 'https://mediahub.unl.edu/media/*'
        - 'http://mediahub.unl.edu/media/*'

      url: 'https://mediahub.unl.edu/oembed'
      discovery: 'false'

One of the benefits of hook_oembed_providers_alter() is that it would be easy for developers to find on api.drupal.org and the documentation is automatically handled.

New patch attached that uses the YAML approach. Because it's a change in direction, there's no interdiff.

If we go this direction, we'll need to rename the issue, update the issue summary, and modify the change record.

(Note: The patch inserts a new argument in the middle of ProviderRepository::_construct, which I know was objected to in the patch #22; however, adding the argument to the end violates Drupal code standards, so in the middle it went.)

I don't think we actually need the providers to be plugins -- the reason being, they don't provide any functionality at all. I suspect we can simply use \Drupal\Component\Discovery\YamlDiscovery to find providers in YAML files, without any need for plugin stuff.

This, I just realized, is something we could do with a contrib project. All we'd need to do is decorate the provider repository with a YamlDiscovery thing that will scan modules for MODULE.oembed_providers.yml. So that said, maybe we could do this more quickly/easily in contrib?

Plugin code is removed.

@phjou - Is my understanding correct that you need to dynamically add providers from user-provided config? If that's the case, can you confirm that the current approach (yaml) will not support your use case?

@phenaproxima - Quick and easy, yes.

If we’re just adding providers and aren’t altering them, then we’d be looking at a function something like hook_media_oembed_provider_add(). That is the type of procedural code we’ve moved away from. So then the challenge becomes how to register providers at runtime [from config]. I think we might be left with a contrib module that decorates the provider repository and provides a UI and defines config for providers. (I suppose it could also scan for YAML. There are multiple ways to register menu items. We could allow two ways to define providers - config and module YAML files.)

If we do go the contrib route, we could also add an option to disable the call to grab the JSON provider file. If you're only allowing custom providers, then there's no need to download and parse the provider JSON file. We would actually use that feature.

@phjou - It could provide an option to disable the downloading and parsing of providers.json. It sounds like in your case that you would not enable that option because you need one or more providers defined by providers.json. In the event that a site only used custom providers, then downloading and parsing providers.json would be unnecessary (thus a potential performance hit).

I had a requirement to add the custom providers to the existing providers' list and the solution provided here was just what I needed!
I applied the patch provided in #22. Even then when I try to add a URL of the custom provider in the remote-video type field, I am not able to embed it. On debugging I was able to find the following error.

Error: Call to a member function getEndpoints() on null in Drupal\media\OEmbed\UrlResolver->getResourceUrl() (line 161 of /app/web/core/modules/media/src/OEmbed/UrlResolver.php).

I am new to Drupal so facing all these difficulties. KIndly if someone could guide me here I would be grateful.

Thank You.

We've added the ability to define custom oEmbed providers in the oEmbed Providers module. They're defined as config entities. AFAIK, this solution addresses every use case described herein. If needed, adding hook_oembed_providers_alter() would be really easy. @phjou - Does this address your use case?

While https://www.drupal.org/project/drupal/issues/2999018 is closed as a duplicate of this, I see the point of the UI not WSODing out when the provider url is not available. Imho a media overview page should never crash because a 3rd-party service can't be reached. Status / warning messages are fine, shouldn't the WSOD be tackled as well?

I see the point of the UI not WSODing out when the provider url is not available. Imho a media overview page should never crash because a 3rd-party service can't be reached. Status / warning messages are fine, shouldn't the WSOD be tackled as well?

Yes, it should be, but it's out of scope for this issue. Would you be willing to open a separate issue to work on that?

I'm using the patch from #22 to alter oEmbed provider information fixing bugs before the oEmbed.com information is updated.

• Can you alter providers using the YAML method?
• Can you also alter or add multiple providers in the YAML file?

I like the idea of using the YAML file, but I need the ability to alter multiple providers.

Thanks all!

Can you alter providers using the YAML method?

No.

Can you also alter or add multiple providers in the YAML file?

Alter - no. Add multiple - yes.

If there were interest, we would add hook_oembed_providers_alter() to the oEmbed Providers module. It's basically a matter of lifting the code from #22. Part of the reason we wrote the module was to avoid running multiple core patches.

I wonder if we shouldn't wontfix this issue in core. It seems like the oEmbed Providers module fills the gap pretty well, and if we could implement the hook there, then I don't think we'd need to do this in core (although we could certainly revisit the idea if enough need arose).

Issue created: #3177117: Provide hook_oembed_providers_alter().

I wonder if we shouldn't wontfix this issue in core. - #52

FWIW, since Media already provides hook_oembed_resource_url_alter() it makes sense to me to provide hook_oembed_providers_alter() in Core. The oEmbed Providers contrib module does fill in the gap, but it seems like a pretty wide gap that "should" be handled in Core.

Well, sites went down yesterday. At the very least, the dependency to an external resource should not be a Core default as it serves like a ticking time bomb.

@randell - It's being discussed here: #3186184: Make oEmbed provider repository more fault-tolerant if provider database is unavailable.

I have rerolled the patch #3008119-22: Provide hook_oembed_providers_alter(). I can't create interdiff, because it is to much work to do taking into account it could be applied only for 9.1.*. Basically, it is absolutely the same. I have tested with the stable 9.3.0 version

@ranjith_kumar_k_u - What is the point to reupload basically the same patch without any changes. Why do you do this ?

The test bot in #60 says the patch failed to apply in core 9.4.x ( https://www.drupal.org/pift-ci-job/2284443 ), may be thats why a corrected patch is re-uploaded in #62 for 9.4.x?

Anyways, as suggested in #61 by @phjou, the oEmbed providers contributed module provides us the hook and additional cool features. I have used the module and it's great! :)

Have a great day everyone, cheers!

Agree with @ankithashetty in #65

The oembed_providers module works and solve this issue.

I think it's good to keep solution as a separate contrib module for now.

There was not much activity on this thread for over year so there is certainly not an urgent need to modify core.

Marking this issue as Postponed.

Linking to OEmbed Providers module again for the record.

In #52 Adam (phenaproxima) said,

I wonder if we shouldn't won't fix this issue in core. It seems like the oEmbed Providers module fills the gap pretty well, and if we could implement the hook there, then I don't think we'd need to do this in core (although we could certainly revisit the idea if enough need arose).

FWIW, I also think this feature request issue should be closed (won't fix) in favor of the contrib solution.

So the issue is that while the contrib module works it currently has to override and circumvent a lot of the logic in core because of core's missing flexiblity. This is definitely not ideal and it would be great if that situation could be improved so that the contrib module has to do less work, this issue being one fairly straightforward way to go about it. If the hook is not deemed appropriate by core maintainers it would be great if a different approach to allow the same or similar flexibility could be discussed before rejecting the issue altogether.

That's just my two cents and if the current state of the module is the best we can get that's also fine, I just personally think we could do better.

I have been re-reading this issue and looking at some of the changes rolled to core media since the it was started and I not sure about this issue. Perhaps it need rescoping. Would love to hear @Chris-Burge thoughts on what might make life easier for modules like oEmbed Providers (#52).

It would be nice if there were a way to call the decorated ->getAll() method inside the decorator ->getAll() method (like you would parent::method() with an extended class); however, the fact that we're using key-value store as a caching mechanism makes that unworkable. We would want to control caching in the decorator method and disable it in the decorated method. I think we're stuck with how we're doing things now.

That actually makes me think that the current implementation of hook_oembed_providers_alter() has a fairly major bug. The key-value store is set to store providers for 7 days (604800 seconds). There's no way to clear it. You just have to wait up to 7 days. So.. I would expect hook_oembed_providers_alter() would only run if the key-value value has expired, not every time ->getAll() is executed. Am I thinking about that correctly?

Seems as though there are 3 paths that could be taken ...

1. Won't fix issue and rely on oEmbed Providers, which already includes hook_oembed_providers_alter()
2. Implement hook_oembed_providers_alter() in Core per the original proposed resolution in the IS
3. Re-scope this issue to assist oEmbed Providers as Chris Burge detailed in #71

What is the will of the council?

@Chris Matthews - Thanks for reanimating this issue.

Re #71, the first part is still an issue, but it probably falls under the "would be nice" category. We could break the current ->getAll() method into two methods. The first [new] method would solely be responsible for fetching (no caching). The second method would be ->getAll(). It would call the first method and then cache the results in key/value before returning the providers list. From a BC standpoint, it would be 100% non-breaking. Then - the oembed_providers module could call the new fetch-only method instead of ->getAll(). Now that I think about it. It would be pretty easy to implement and to add test coverage. This isn't a heavy lift.

The second part was addressed in #3296300: Add UI to clear provider list from keyvalue. If hook_oembed_providers_alter() were implemented in core, then I think we would also want to address the caching issue in core - namely how to clear the values cached in key/value if someone doesn't want to wait 7 days. That might be a can of worms not worth opening.

What about this?

1. Add a new, fetch-only method to the ProviderRepository class in core.
2. Keep hook_oembed_providers_alter() in contrib.

1. Add a new, fetch-only method to the ProviderRepository class in core. 💯 ✅ 👍
2. Keep hook_oembed_providers_alter() in contrib. 💯 ✅ 👍

Both make sense to me.

I dove into ProviderRepository, and now that I'm in the code, I'm seeing it's going to be a bit more challenging to refactor ->getAll() into two methods. We'd also have to update ProviderRepositoryInterrface, which creates a BC issue. So - on second thought, let's not move forward on that.

In terms of this issue, I think we're good to change the status to 'Closed (won't fix)'.