Log exceptions for invalid oEmbed providers

Thanks for looking into this!

+1 RTBC

Looks great! Nice, straightforward test coverage. My feedback is all minor stuff, mostly style things. Thanks for writing this!

1. +++ b/core/modules/media/src/OEmbed/ProviderRepository.php
   @@ -119,4 +121,18 @@ public function get($provider_name) {
   +    if (!$this->logger) {
   +      $this->logger = \Drupal::service('logger.channel.media');
   +    }
   +
   +    return $this->logger;
   

   I'm not a huge fan of the whole "getter that is actually a setter" pattern. Let's reduce all this down to one line: return $this->logger ?: \Drupal::service('logger.channel.media');

2. +++ b/core/modules/media/tests/src/Unit/OEmbed/ProviderRepositoryTest.php
   @@ -0,0 +1,140 @@
   +  public function setUp() {
   +    parent::setUp();
   +
   +    $this->httpClient = $this->createMock(ClientInterface::class);
   +    $this->configFactory = $this->createMock(ConfigFactoryInterface::class);
   +    $this->time = $this->createMock(TimeInterface::class);
   +    $this->logger = $this->createMock(LoggerInterface::class);
   +    $this->cacheBackend = $this->createMock(CacheBackendInterface::class);
   +  }
   

   There's only one test method, so we don't really need to implement setUp() on this class. Therefore, we also don't really need to have the mocked objects as properties of the class. Let's just make them local variables of the test method.

3. +++ b/core/modules/media/tests/src/Unit/OEmbed/ProviderRepositoryTest.php
   @@ -0,0 +1,140 @@
   +    $this->cacheBackend
   +      ->expects($this->once())
   +      ->method('get')
   +      ->with('media:oembed_providers')
   +      ->willReturn(NULL);
   

   Let's use Prophecy to mock the classes that need it; it's a lot more readable. This will become something like:

   $cache_backend = $this->prophesize(CacheBackendInterface::class);
   $cache_backend->get('media:oembed_providers')->willReturn(NULL);
   

4. +++ b/core/modules/media/tests/src/Unit/OEmbed/ProviderRepositoryTest.php
   @@ -0,0 +1,140 @@
   +    $this->logger
   +      ->expects($this->once())
   +      ->method('error');
   

   Using Prophecy, this will be something like:
   $logger->error(Argument::type('string'))->shouldBeCalled()

5. +++ b/core/modules/media/tests/src/Unit/OEmbed/ProviderRepositoryTest.php
   @@ -0,0 +1,140 @@
   +    $providers = $providerRepository->getAll();
   +
   +    $this->assertTrue(is_array($providers));
   

   This should be something like $this->assertInternalType('array', $provider_repository->getAll());

Thanks! Let's see if this passes tests. Looking forward to getting this committed!

Removed the unused use GuzzleHttp\Psr7\Response; use statement.

Thanks for this! Looking super good.

1. +++ b/core/modules/media/src/OEmbed/ProviderRepository.php
   @@ -119,4 +121,15 @@ public function get($provider_name) {
   +   * Get the logger service.
   

   Should be "Gets".

2. +++ b/core/modules/media/tests/src/Unit/OEmbed/ProviderRepositoryTest.php
   @@ -0,0 +1,132 @@
   +    $this->httpClient = $this->prophesize(ClientInterface::class);
   +    $this->configFactory = $this->prophesize(ConfigFactoryInterface::class);
   +    $this->time = $this->prophesize(TimeInterface::class);
   +    $this->logger = $this->prophesize(LoggerInterface::class);
   +    $this->cacheBackend = $this->prophesize(CacheBackendInterface::class);
   

   Can all these be moved into local variables of testGetAllWhenProviderUrlIsInvalid()? :)

3. +++ b/core/modules/media/tests/src/Unit/OEmbed/ProviderRepositoryTest.php
   @@ -0,0 +1,132 @@
   +      ->willReturn($config);
   

   This should be returning the mocked double, so ->willReturn($config->reveal()).

4. +++ b/core/modules/media/tests/src/Unit/OEmbed/ProviderRepositoryTest.php
   @@ -0,0 +1,132 @@
   +    $response = $this->prophesize(ResponseInterface::class);
   

   I didn't know this until very recently, but responses don't really need to be prophesized :) Something like new Response(200, [], Json::encode(...))

5. +++ b/core/modules/media/tests/src/Unit/OEmbed/ProviderRepositoryTest.php
   @@ -0,0 +1,132 @@
   +      ->willReturn(json_encode([
   

   Should be using \Drupal\Component\Serialization\Json::encode().

Perfect.

1. +++ b/core/modules/media/src/OEmbed/ProviderRepository.php
   @@ -9,12 +9,14 @@
   +  use LoggerAwareTrait;
   

   This is not really used in core.

2. +++ b/core/modules/media/src/OEmbed/ProviderRepository.php
   @@ -99,7 +101,7 @@ public function getAll() {
          catch (ProviderException $e) {
            // Just skip all the invalid providers.
   -        // @todo Log the exception message to help with debugging.
   +        $this->getLogger()->error($e->getMessage());
          }
   

   Also why are we logging here? I'm not sure that this is the correct approach. We are throwing ProviderException above so why not make this the responsibility of the callers?

3. +++ b/core/modules/media/src/OEmbed/ProviderRepository.php
   @@ -119,4 +121,15 @@ public function get($provider_name) {
   +  /**
   +   * Gets the logger service.
   +   *
   +   * @return \Psr\Log\LoggerInterface
   +   *   The logger.
   +   */
   +  protected function getLogger() {
   +    return $this->logger ?: \Drupal::service('logger.channel.media');
   +  }
   

   Maybe we should be injecting the logger here.

Glorious! All of @alexpott's feedback is addressed; back to RTBC.

+++ b/core/modules/media/src/OEmbed/ProviderRepository.php
@@ -58,13 +66,21 @@ class ProviderRepository implements ProviderRepositoryInterface {
+   * @param \Drupal\core\Logger\LoggerChannelInterface $logger
+   *   (optional) The logger service.
...
-  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, TimeInterface $time, CacheBackendInterface $cache_backend = NULL, $max_age = 604800, LoggerChannelInterface $logger = NULL) {
...
+    if ($logger === NULL) {
+      @trigger_error('A logger channel service must be passed to \Drupal\media\OEmbed\ProviderRepository::__construct(). It was addedd in 8.8.0 and will be required before Drupal 9.0.0.', E_USER_DEPRECATED);
+      $logger = \Drupal::logger('media');
+    }
+    $this->logger = $logger;

The problem with adding $logger to the end of the arguments is that $cache_backend is optional. Really we need to move it to before $cache_backend.

This shuffle is kinda complex though... so something like this...

  /**
   * Constructs a ProviderRepository instance.
   *
   * @param \GuzzleHttp\ClientInterface $http_client
   *   The HTTP client.
   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   *   The config factory service.
   * @param \Drupal\Component\Datetime\TimeInterface $time
   *   The time service.
   * @param \Drupal\Core\Cache\CacheBackendInterface $cache_backend
   *   (optional) The cache backend.
   * @param \Drupal\core\Logger\LoggerChannelInterface $logger
   *   (optional) The logger service.
   * @param int $max_age
   *   (optional) How long the cache data should be kept. Defaults to a week.
   */
  public function __construct(ClientInterface $http_client, ConfigFactoryInterface $config_factory, TimeInterface $time, $logger = NULL, $cache_backend = NULL, $max_age = 604800) {
    $this->httpClient = $http_client;
    $this->providersUrl = $config_factory->get('media.settings')->get('oembed_providers_url');
    $this->time = $time;
    // Old format of the constructor.
    if (($logger instanceof CacheBackendInterface::class) || $logger === NULL) {
      $this->cacheBackend = $logger;
      @trigger_error('A logger channel service must be passed to \Drupal\media\OEmbed\ProviderRepository::__construct(). It was addedd in 8.8.0 and will be required before Drupal 9.0.0.', E_USER_DEPRECATED);
      $logger = \Drupal::logger('media');
    }
    if (is_int($cache_backend)) {
      $this->maxAge = (int) $cache_backend;
    }
    else {
      $this->maxAge = (int) $max_age;
    }
    if (!($logger instanceof LoggerChannelInterface)) {
      throw new \RuntimeException('Something helpful');
    }
    $this->logger = $logger;
  }

We need not to the type hinting manually.

No worries. Let's see how the testbot likes this.

+++ b/core/modules/media/src/OEmbed/ProviderRepository.php
@@ -54,17 +62,33 @@ class ProviderRepository implements ProviderRepositoryInterface {
+    if ($logger_factory instanceof CacheBackendInterface || $logger_factory === NULL) {
+      @trigger_error('The logger.factory service must be passed to \Drupal\media\OEmbed\ProviderRepository::__construct(). It was added in Drupal 8.8.0 and will be required before Drupal 9.0.0.', E_USER_DEPRECATED);
+      $this->cacheBackend = $logger_factory;
+      $logger_factory = \Drupal::service('logger.factory');
+    }

We're missing an else to handle $cache_backend when in is a CacheBackendInterface

Discussed more with @averagejoe3000...

+++ b/core/modules/media/src/OEmbed/ProviderRepository.php
@@ -54,17 +62,37 @@ class ProviderRepository implements ProviderRepositoryInterface {
+    if ($cache_backend instanceof CacheBackendInterface || $cache_backend === NULL) {
+      $this->cache_backend = $cache_backend;
+    }

I think we beed the else thrown exception here too...

Maybe we can change direction above to have the first bit sort out the arguments to conform to the new expectations and then do the pseudo-typehinting here.

Thanks @averagejoe3000. Sorry that this change is proving tricky. Maintaining deprecated constructors were arguments are moving is not an easy task!

1. +++ b/core/modules/media/src/OEmbed/ProviderRepository.php
   @@ -54,17 +62,41 @@ class ProviderRepository implements ProviderRepositoryInterface {
   +    // Handle the deprecated form of the constructor.
   

   Because this is complex we should introduce a legacy test that tests our assumptions about how this logic works and how the deprecation is triggered.

2. +++ b/core/modules/media/src/OEmbed/ProviderRepository.php
   @@ -54,17 +62,41 @@ class ProviderRepository implements ProviderRepositoryInterface {
   +      throw new \InvalidArgumentException('The cache.default service must be passed to ' . __METHOD__);
   

   must be an instance of CacheBackendInterface - according to the docs it is optional.

As of #3186184: Make oEmbed provider repository more fault-tolerant if provider database is unavailable (9.3.x and up), the provider repository is much more docile about throwing exceptions. It will now throw only if it really doesn't have any data to work with, even in its permanent non-volatile cache, which is truly a show-stopping error condition that warrants a throw.

Therefore, I think we can reduce the scope of this issue and patch, to simply add a log line warning about providers which are skipped. I can see such a thing being helpful for debugging purposes. The referenced issue also injected the logger factory and handled the backwards compatibility implications of that, so this patch can be really, really simple. Just a log line and a unit test. I'm downgrading it to minor priority for that reason.

Hiding patches in favor of the merge request. Which has tests, so removing that tag as well.

MR looks good and has test coverage. +1 for commit

Committed/pushed to 9.3.x, thanks!