Expose oEmbed provider URL setting in the Media configuration form

The workaround could be contrib module to alter this param but it needs kinda "mirroring" for that file

I confirm this

I have this problem!

The world is so sad....

So we will expose the config setting in the UI. Until we have that, you can change it by drush:

drush cset media.settings oembed_providers_url https://my-oembed-provider-url.com/providers.json

You just need a proxy server that's not banned.

Re-tagging, and marking for issue summary update since it seems that we agree to expose the config option in the UI. Let's also re-title this issue while we're at it.

That's super cool.
It's not an issue, it's a feature and a very good improvement.
And I'm not kidding here

This setting needs UI (as #10) and proper error handling when list of providers is not accessible

Marking as a feature request.

Nice examples from WP about adding custom providers https://generatewp.com/introducing-oembed-provider-generator/

Another big database of providers https://github.com/itteco/iframely

Surely this list should be configurable

@wengerk and I are working on addding tests on this feature.

Here is the patch with some tests coverage (thanks for wengerk the help and review).

I have to change the request send in the MediaSettingsForm::validationForm to use request instead of get. get is a magic call which was not mockable.

We also had to test the form via a KernelTest because we need to mock the httpClient (Browser Test was not an option in this case). If you know a better way to achieve this, I'm open to suggestion. Testing Drupal Form with a KernelTest was not that easy because of all the dependencies ^^.

Now that there is tests on the proposed changes in #17, let's share some thoughts:

1. Checking the availability of the provider URL on submit is nice but I'm not sure if this is a real requirements.
2. [Security] Maybe we need to display a message to user to warn them to only put trusted provider URL here. I'm curious what the security team think about it ?
3. [UX]As this feature is more like an overriding of a default setting, I'm not sure if the current behavior is correct. What about leaving the field empty (=default provider URL) and just allow user to override it if they like. One problem I could see is if user override the default provider URL they currently can't easily found what was the default setting.

Thanks all for your review & feedback.

Indeed we try our best but let's discuss our implementation, we maybe could find a better approach.

Add the missing@group media to the tests ... sorry guys

Looks real good! Removing the "needs tests" tag, since if there is one thing this patch is not lacking, it's tests!

1. +++ b/core/modules/media/src/Form/MediaSettingsForm.php
   @@ -64,6 +77,8 @@ protected function getEditableConfigNames() {
   +    $providers_url = $this->config('media.settings')
   +      ->get('oembed_providers_url');
   

   Nit: This does not need to be its own variable. Let's just use '#default_value' => $this->config('media.settings')->get('oembed_providers_url').

2. +++ b/core/modules/media/src/Form/MediaSettingsForm.php
   @@ -91,15 +106,62 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t('Enter a different URL from which to serve oEmbed providers information, including the <em>http://</em> or <em>https://</em> prefix.'),
   

   This needs to be rephrased a bit. How about: "Enter the URL from which to fetch information about all available oEmbed providers, including the http:// or https:// prefix. Generally, this should only be changed if the default URL is blocked or otherwise inaccessible."

3. +++ b/core/modules/media/src/Form/MediaSettingsForm.php
   @@ -91,15 +106,62 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    // Trying to access URL. If something is wrong, the URL is incorrect, or
   

   "Trying" --> "Try"

4. +++ b/core/modules/media/src/Form/MediaSettingsForm.php
   @@ -91,15 +106,62 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    try {
   +      $request = $this->httpClient->request('GET', $providers_url);
   +      $status_code = $request->getStatusCode();
   +
   +      if ($status_code != 200) {
   +        $message = $this->t("Error trying to access provided oEmbed Providers URL. Request ended with code @request_code and message @request_message.", [
   +          '@request_code' => $request->getStatusCode(),
   +          '@request_message' => $request->getBody()->getContents(),
   +        ]);
   +
   +        $form_state->setErrorByName('oembed_providers_url', $message);
   +      }
   

   So here is the thing -- this kind of leaks the abstraction that the media.oembed.provider_repository service provides.

   I would prefer if we added a new method to ProviderRepositoryInterface, called setProvidersUrl(), which can change the providers URL (permanently) and validate that it is, in fact, accessible. The implementation would look something like:

   public function setProvidersUrl($url, $validate = TRUE) {
     $old_url = $this->providersUrl;
   
     $this->providersUrl = $url;
   
     if ($validate) {
       try {
         $this->getAll();
       }
       catch (ProviderException $e) {
         // Restore the previous URL and re-throw the caught exception,
         // preserving the stack trace.
         $this->providersUrl = $old_url;
         throw new ProviderException($e->getMessage(), NULL, $e);
       }
     }
     $this->configFactory->get('media.settings')->set('oembed_providers_url', $url);
   }
   

   This validateForm() method would then set an error if it catches a ProviderException. All of this would keep the provider URL abstraction well-encapsulated in the ProviderRepository class.

   The only real disadvantage is that, if we do this, we'll need framework manager review on this issue since we're adding a method to ProviderRepositoryInterface. However, we should be okay in the eyes of the backwards compatibility policy under the 1:1 interface:implementation rule.

5. +++ b/core/modules/media/tests/src/Kernel/MediaSettingsFormTest.php
   @@ -0,0 +1,153 @@
   +    $this->assertEquals(
   

   This should be assertSame().

Thanks! Feel free to set the issue to "Needs review" when you post a new patch; that way the testbot will run tests automatically :)

Nice work! Only a few more points, and then I can review the tests. Also tagging for framework manager review, since we're altering ProviderRepositoryInterface.

Additionally, could you post interdiffs along with the patches? It will make things a lot easier to review :) Instructions are at https://www.drupal.org/documentation/git/interdiff. Thanks!

1. +++ b/core/modules/media/src/Form/MediaSettingsForm.php
   @@ -23,16 +25,36 @@ class MediaSettingsForm extends ConfigFormBase {
   +   * The HTTP client.
   +   *
   +   * @var \GuzzleHttp\ClientInterface
   +   */
   +  protected $httpClient;
   

   We no longer need the HTTP client in this form, so let's remove it.

2. +++ b/core/modules/media/src/Form/MediaSettingsForm.php
   @@ -91,6 +115,30 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form['providers']['oembed_providers_url_validate'] = [
   +      '#type' => 'checkbox',
   +      '#title' => $this->t('Validate URL'),
   +      '#default_value' => TRUE,
   +      '#description' => $this->t('If checked, the URL will be tested to accessibility from the site.'),
   +    ];
   

   We should remove this. I cannot think of any circumstances under which we do *not* want to validate the given URL. If it's invalid, the entire oEmbed system will flat-out break, so we should never allow people to bypass validation in the UI.

3. +++ b/core/modules/media/src/OEmbed/ProviderRepository.php
   @@ -119,4 +128,26 @@ public function get($provider_name) {
   +    $this->configFactory->getEditable('media.settings')->set('oembed_providers_url', $url);
   

   Nit: This line is longer than 80 characters.

4. +++ b/core/modules/media/src/OEmbed/ProviderRepositoryInterface.php
   @@ -37,4 +37,18 @@ public function getAll();
   +   * Set new oEmbed providers URL.
   

   "Set" should be "Sets".

5. +++ b/core/modules/media/src/OEmbed/ProviderRepositoryInterface.php
   @@ -37,4 +37,18 @@ public function getAll();
   +   * @param bool $validate
   +   *   If set to TRUE, then provided URL will be check for accessibility,
   +   *   otherwise it will be set without any tests.
   

   The description should begin with "(optional)", and we should mention that it defaults to TRUE.

We'll need framework manager review on this issue since we're adding a method to ProviderRepositoryInterface.

The tests need to be rerolled following all changes from #28 to #33.

Here is a rewrite of tests to match the changes from #28 to #33.

Let's test it !

wengerk, can you check tests?

Here is the new coverage.
Let's try it on testbot.

Short story:
I had to replace the whole Functional/ProviderRepositoryTest and move it as a KernelTest (Kernel/ProviderRepositoryTest) which allows me to proper mock the http_client.
It's basically the same tests as previously but using a Kernel tests instead of Browser one.

Thanks for your review !

Here the two last tests should be fixed (ResourceFetcherTest & UrlResolverTest).

Here a reroll since MediaSettingsForm has changed a little bit & make this patch not applicable.

I was unable to generate the interdiff between the two patch (41 & 43) if someone can help it's would be a pleasure.

[deleted]

Thanks for the #17 patch. It works great on the d8.6.10.

Looks like this doesn't apply any more, tagging for reroll.

I can apply #43 patch on 8.7.x and 8.8.x

Applies to 8.7.0-beta2 and I see the setting on /admin/config/media/media-settings. Marking as RTBC!

I'm not 100% sure I feel okay with this going in as-is; kicking back to Needs Review to actually, like, review this from a standpoint of "do the new API additions make sense?"

I have same error if i don't have internet ))) or if not have curl localhost (cURL error 60: SSL certificate problem: unable to get local issuer certificate)

Maybe add posibility add local providers.json?

For Patch have error "Bad Request"
to solve this add in patch method GET

try {
      $response = $this->httpClient->request('GET', $this->providersUrl);
    }

Review of #43

1. +++ b/core/modules/media/src/Form/MediaSettingsForm.php
   @@ -7,6 +7,7 @@
   +use Drupal\media\OEmbed\ProviderRepositoryInterface;
    use Symfony\Component\DependencyInjection\ContainerInterface;
    
    /**
   @@ -30,6 +31,13 @@ class MediaSettingsForm extends ConfigFormBase {
   
   @@ -30,6 +31,13 @@ class MediaSettingsForm extends ConfigFormBase {
       */
      protected $entityTypeManager;
    
   +  /**
   +   * The oEmbed provider repository.
   +   *
   +   * @var \Drupal\media\OEmbed\ProviderRepositoryInterface
   +   */
   +  protected $providerRepository;
   +
      /**
       * MediaSettingsForm constructor.
       *
   @@ -39,11 +47,14 @@ class MediaSettingsForm extends ConfigFormBase {
   
   @@ -39,11 +47,14 @@ class MediaSettingsForm extends ConfigFormBase {
       *   The iFrame URL helper service.
       * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
       *   The entity type manager.
   +   * @param \Drupal\media\OEmbed\ProviderRepositoryInterface $provider_repository
   +   *   The oEmbed provider repository.
       */
   -  public function __construct(ConfigFactoryInterface $config_factory, IFrameUrlHelper $iframe_url_helper, EntityTypeManagerInterface $entity_type_manager) {
   +  public function __construct(ConfigFactoryInterface $config_factory, IFrameUrlHelper $iframe_url_helper, EntityTypeManagerInterface $entity_type_manager, ProviderRepositoryInterface $provider_repository) {
        parent::__construct($config_factory);
        $this->iFrameUrlHelper = $iframe_url_helper;
        $this->entityTypeManager = $entity_type_manager;
   +    $this->providerRepository = $provider_repository;
      }
    
      /**
   @@ -53,7 +64,8 @@ public static function create(ContainerInterface $container) {
   
   @@ -53,7 +64,8 @@ public static function create(ContainerInterface $container) {
        return new static(
          $container->get('config.factory'),
          $container->get('media.oembed.iframe_url_helper'),
   -      $container->get('entity_type.manager')
   +      $container->get('entity_type.manager'),
   +      $container->get('media.oembed.provider_repository')
   

   +1 for using DI

2. +++ b/core/modules/media/src/Form/MediaSettingsForm.php
   @@ -110,6 +122,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form['providers'] = [
   +      '#type' => 'details',
   +      '#title' => $this->t('oEmbed providers'),
   +      '#open' => TRUE,
   +    ];
   +
   +    $form['providers']['oembed_providers_url'] = [
   +      '#type' => 'url',
   +      '#title' => $this->t('oEmbed providers URL'),
   +      '#size' => 40,
   +      '#maxlength' => 255,
   +      '#required' => TRUE,
   +      '#default_value' => $this->config('media.settings')
   +        ->get('oembed_providers_url'),
   +      '#description' => $this->t('Enter the URL from which to fetch information about all available oEmbed providers, including the http:// or https:// prefix. Generally, this should only be changed if the default URL is blocked or otherwise inaccessible.'),
   +    ];
   

   Does this also work for the media library?

3. +++ b/core/modules/media/src/OEmbed/ProviderRepository.php
   @@ -61,7 +68,9 @@ class ProviderRepository implements ProviderRepositoryInterface {
   -    $this->providersUrl = $config_factory->get('media.settings')->get('oembed_providers_url');
   +    $this->configFactory = $config_factory;
   +    $this->providersUrl = $config_factory->get('media.settings')
   +      ->get('oembed_providers_url');
   

   We don't need this change. $config_factory was already available. No need to move it down. Or am I missing some consistency issue?

4. +++ b/core/modules/media/src/OEmbed/ProviderRepository.php
   @@ -85,7 +94,12 @@ public function getAll() {
   +    // Rewind the response body to seek to the beginning of the stream
   +    // before reading data and read the stream.
   +    // @see https://github.com/guzzle/guzzle/pull/1262#issuecomment-149080749
   +    $response->getBody()->rewind();
   

   Good comment.
   I would otherwise think rewind was not needed.

5. +++ b/core/modules/media/src/OEmbed/ProviderRepository.php
   @@ -119,4 +133,27 @@ public function get($provider_name) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setProvidersUrl($url, $validate = TRUE) {
   +    $old_url = $this->providersUrl;
   +
   +    $this->providersUrl = $url;
   +
   +    if ($validate) {
   +      try {
   +        $this->getAll();
   +      }
   +      catch (ProviderException $e) {
   +        // Restore the previous URL and re-throw the caught exception,
   +        // preserving the stack trace.
   +        $this->providersUrl = $old_url;
   +        throw new ProviderException($e->getMessage(), NULL, $e);
   +      }
   +    }
   +    $this->configFactory->getEditable('media.settings')
   +      ->set('oembed_providers_url', $url);
   +  }
   +
   

   We are always calling this with validate = TRUE

   Do we need the extra $validate variable and if statement?
   It's like: if(TRUE) { // trycatch }

Note:

I did not review the tests files.

I tried the patch, but It did not solve my issue. But that was unrelated the oembed_providers_url setting was not filled.

I think we should change the approach here a bit. I know it was originally my idea, but in retrospect, I'd prefer if we didn't add a new method to ProviderRepositoryInterface. I also removed the kernel-level MediaSettingsFormTest (for now) -- that should really be a functional test, I think, rather than a kernel test. Marking this "needs tests" for functional tests of the MediaSettingsForm validation stuff I added here.

Streamlined the ProviderRepositoryTest as well, since I think it was doing more mocking than it needed to :)

I also removed the call to $response->getBody()->rewind() in ProviderRepository. We probably should not do that; it makes the abstraction rather leaky. Besides, StreamInterface::__toString()'s doc comment specifically says that implementations need to rewind, so casting the response body to a string is sufficient.

Added some functional test coverage of the settings form.

I'm removing the "needs framework manager review" tag, since this no longer has any API changes to the oEmbed system. Just needs subsystem maintainer sign-off.

+1 for exposing the oembed url. Not only this is needed for another url but also if for some reason the oembed url is not set (config overwritten, ...).
Using the patch provided in #59 seems like a correct approach and works for me.

Oh, and this is targeted to 8.8 now :)

In general, plus one for making this configurable! Minor feedback regarding the latest patch.

1. +++ b/core/modules/media/src/Form/MediaSettingsForm.php
   @@ -107,12 +121,43 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $config->set('oembed_providers_url', $form_state->getValue('oembed_providers_url'))
   ...
   +      $config->set('oembed_providers_url', $original_url)->save();
   

   Saving the config in validate seems a bit weird to me? This solves the fact we don't need to change the providerRepository, but not sure if this is better.

2. +++ b/core/modules/media/tests/src/Functional/MediaSettingsTest.php
   @@ -30,6 +35,42 @@ public function testStatusPage() {
   +    $assert_session->pageTextContains('Could not retrieve the oEmbed provider database from https://foo.baz');
   ...
   +    $assert_session->pageTextContains("Could not retrieve the oEmbed provider database from $providers_url");
   ...
   +    $assert_session->pageTextContains('Remote oEmbed providers database returned invalid or empty list.');
   ...
   +    $assert_session->pageTextContains('Remote oEmbed providers database returned invalid or empty list.');
   

   We have tests for a bunch of cases where changing the URL fails. Could we add an extra case where the URL actually is saved to make sure that works as well?

@phenaproxima just pointed out that #3008119: Provide hook_oembed_providers_alter() is a different solution for the same problem. Since changing the default list is probably not very common, I think the hook might be a better solution. It is a bit more flexible and avoids exposing a setting in the UI that a lot of users might not understand.

My vote would be to close this issue and solve this in #3008119: Provide hook_oembed_providers_alter().

I am, to be honest, okay with either solution. The providers URL is already configurable, and has been since day one; this issue was only exposing the setting in the UI. For sites which have to deal with firewalls and other odd situations, they can set the configuration option with Drush, or in a custom module/installation profile.

So, unless there is some other technical reason to continue, I am tentatively +1 for closing this issue (won't fix) and fixing #3008119: Provide hook_oembed_providers_alter().

In #3008119: Provide hook_oembed_providers_alter() comment #10 @seanB said,

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.

So, closing this issue as a duplicate of #3008119: Provide hook_oembed_providers_alter()

The related change record is here: https://www.drupal.org/node/3097113

The requested functionality is now provided by the oEmbed Providers module.