Skip to content
Snippets Groups Projects
Commit 26327f96 authored by catch's avatar catch
Browse files

Issue #3186184 by phenaproxima, larowlan, xavier.masson, Berdir, mstrelan,... 

Issue #3186184 by phenaproxima, larowlan, xavier.masson, Berdir, mstrelan, hchonov, Chris Burge, longwave, Lendude: Make oEmbed provider repository more fault-tolerant if provider database is unavailable
parent 26fed74b
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
core/modules/media/media.services.yml
+ 1
− 1
View file @ 26327f96
......@@ -12,7 +12,7 @@ services:
arguments: ['@media.oembed.provider_repository', '@media.oembed.resource_fetcher', '@http_client', '@module_handler', '@cache.default']
media.oembed.provider_repository:
class: Drupal\media\OEmbed\ProviderRepository
arguments: ['@http_client', '@config.factory', '@datetime.time', '@cache.default']
arguments: ['@http_client', '@config.factory', '@datetime.time', '@keyvalue', '@logger.factory']
media.oembed.resource_fetcher:
class: Drupal\media\OEmbed\ResourceFetcher
arguments: ['@http_client', '@media.oembed.provider_repository', '@cache.default']
......
core/modules/media/src/OEmbed/ProviderRepository.php
+ 73
− 18
View file @ 26327f96
......@@ -4,8 +4,10 @@
use Drupal\Component\Datetime\TimeInterface;
use Drupal\Component\Serialization\Json;
use Drupal\Core\Cache\CacheBackendInterface;
use Drupal\Core\Config\ConfigFactoryInterface;
use Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait;
use Drupal\Core\KeyValueStore\KeyValueFactoryInterface;
use Drupal\Core\Logger\LoggerChannelFactoryInterface;
use GuzzleHttp\ClientInterface;
use GuzzleHttp\Exception\TransferException;
......@@ -14,6 +16,17 @@
*/
class ProviderRepository implements ProviderRepositoryInterface {
use DeprecatedServicePropertyTrait;
/**
* The service properties that should raise a deprecation error.
*
* @var string[]
*/
private $deprecatedProperties = [
'cacheBackend' => 'cache.default',
];
/**
* How long the provider data should be cached, in seconds.
*
......@@ -43,11 +56,18 @@ class ProviderRepository implements ProviderRepositoryInterface {
protected $time;
/**
* The cache backend.
* The key-value store.
*
* @var \Drupal\Core\KeyValueStore\KeyValueStoreInterface
*/
protected $keyValue;
/**
* The logger channel.
*
* @var \Drupal\Core\Cache\CacheBackendInterface
* @var \Drupal\Core\Logger\LoggerChannelInterface
*/
protected $cacheBackend;
protected $logger;
/**
* Constructs a ProviderRepository instance.
......@@ -58,44 +78,76 @@ class ProviderRepository implements ProviderRepositoryInterface {
* The config factory service.
* @param \Drupal\Component\Datetime\TimeInterface $time
* The time service.
* @param \Drupal\Core\Cache\CacheBackendInterface $cache_backend
* The cache backend.
* @param \Drupal\Core\KeyValueStore\KeyValueFactoryInterface $key_value_factory
* The key-value store factory.
* @param \Drupal\Core\Logger\LoggerChannelFactoryInterface $logger_factory
* The logger channel factory.
* @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, CacheBackendInterface $cache_backend = NULL, $max_age = 604800) {
public function __construct(ClientInterface $http_client, ConfigFactoryInterface $config_factory, TimeInterface $time, $key_value_factory = NULL, $logger_factory = NULL, int $max_age = 604800) {
$this->httpClient = $http_client;
$this->providersUrl = $config_factory->get('media.settings')->get('oembed_providers_url');
$this->time = $time;
if (empty($cache_backend)) {
$cache_backend = \Drupal::cache();
@trigger_error('Passing NULL as the $cache_backend parameter to ' . __METHOD__ . '() is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. See https://www.drupal.org/node/3223594', E_USER_DEPRECATED);
if (!($key_value_factory instanceof KeyValueFactoryInterface)) {
@trigger_error('The keyvalue service should be passed to ' . __METHOD__ . '() since drupal:9.3.0 and is required in drupal:10.0.0. See https://www.drupal.org/node/3186186', E_USER_DEPRECATED);
$key_value_factory = \Drupal::service('keyvalue');
}
if (!($logger_factory instanceof LoggerChannelFactoryInterface)) {
// If $max_age was passed in $logger_factory's position, ensure that we
// use the correct value.
if (is_numeric($logger_factory)) {
$max_age = $logger_factory;
}
@trigger_error('The logger.factory service should be passed to ' . __METHOD__ . '() since drupal:9.3.0 and is required in drupal:10.0.0. See https://www.drupal.org/node/3186186', E_USER_DEPRECATED);
$logger_factory = \Drupal::service('logger.factory');
}
$this->cacheBackend = $cache_backend;
$this->maxAge = (int) $max_age;
$this->maxAge = $max_age;
$this->keyValue = $key_value_factory->get('media');
$this->logger = $logger_factory->get('media');
}
/**
* {@inheritdoc}
*/
public function getAll() {
$cache_id = 'media:oembed_providers';
$cached = $this->cacheBackend->get($cache_id);
if ($cached) {
return $cached->data;
$current_time = $this->time->getCurrentTime();
$stored = $this->keyValue->get('oembed_providers');
// If we have stored data that hasn't yet expired, return that. We need to
// store the data in a key-value store because, if the remote provider
// database is unavailable, we'd rather return stale data than throw an
// exception. This means we cannot use a normal cache backend or expirable
// key-value store, since those could delete the stale data at any time.
if ($stored && $stored['expires'] > $current_time) {
return $stored['data'];
}
try {
$response = $this->httpClient->request('GET', $this->providersUrl);
}
catch (TransferException $e) {
if (isset($stored['data'])) {
// Use the stale data to fall back gracefully, but warn site
// administrators that we used stale data.
$this->logger->warning('Remote oEmbed providers could not be retrieved due to error: @error. Using previously stored data. This may contain out of date information.', [
'@error' => $e->getMessage(),
]);
return $stored['data'];
}
// We have no previous data and the request failed.
throw new ProviderException("Could not retrieve the oEmbed provider database from $this->providersUrl", NULL, $e);
}
$providers = Json::decode((string) $response->getBody());
if (!is_array($providers) || empty($providers)) {
if (isset($stored['data'])) {
// Use the stale data to fall back gracefully, but as above, warn site
// administrators that we used stale data.
$this->logger->warning('Remote oEmbed providers database returned invalid or empty list. Using previously stored data. This may contain out of date information.');
return $stored['data'];
}
// We have no previous data and the current data is corrupt.
throw new ProviderException('Remote oEmbed providers database returned invalid or empty list.');
}
......@@ -111,7 +163,10 @@ public function getAll() {
}
}
$this->cacheBackend->set($cache_id, $keyed_providers, $this->time->getCurrentTime() + $this->maxAge);
$this->keyValue->set('oembed_providers', [
'data' => $keyed_providers,
'expires' => $current_time + $this->maxAge,
]);
return $keyed_providers;
}
......
core/modules/media/tests/modules/media_test_oembed/src/ProviderRepository.php
+ 0
− 9
View file @ 26327f96
......@@ -2,7 +2,6 @@
namespace Drupal\media_test_oembed;
use Drupal\Core\Cache\NullBackend;
use Drupal\media\OEmbed\Provider;
use Drupal\media\OEmbed\ProviderRepository as BaseProviderRepository;
......@@ -14,14 +13,6 @@
*/
class ProviderRepository extends BaseProviderRepository {
/**
* {@inheritdoc}
*/
public function __construct(...$arguments) {
parent::__construct(...$arguments);
$this->cacheBackend = new NullBackend('default');
}
/**
* {@inheritdoc}
*/
......
core/modules/media/tests/src/Kernel/ProviderRepositoryTest.php
+ 19
− 3
View file @ 26327f96
......@@ -2,6 +2,8 @@
namespace Drupal\Tests\media\Kernel;
use Drupal\Core\Cache\CacheBackendInterface;
use Drupal\Core\Cache\NullBackend;
use Drupal\KernelTests\KernelTestBase;
use Drupal\media\OEmbed\ProviderRepository;
......@@ -18,12 +20,26 @@ class ProviderRepositoryTest extends KernelTestBase {
* @group legacy
*/
public function testDeprecations(): void {
$this->expectDeprecation('Passing NULL as the $cache_backend parameter to Drupal\media\OEmbed\ProviderRepository::__construct() is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. See https://www.drupal.org/node/3223594');
new ProviderRepository(
// Passing a cache backend in the key-value store's place, and the max age
// in the logger factory's place, should raise deprecation notices.
$this->expectDeprecation('The keyvalue service should be passed to Drupal\media\OEmbed\ProviderRepository::__construct() since drupal:9.3.0 and is required in drupal:10.0.0. See https://www.drupal.org/node/3186186');
$this->expectDeprecation('The logger.factory service should be passed to Drupal\media\OEmbed\ProviderRepository::__construct() since drupal:9.3.0 and is required in drupal:10.0.0. See https://www.drupal.org/node/3186186');
$providers = new ProviderRepository(
$this->container->get('http_client'),
$this->container->get('config.factory'),
$this->container->get('datetime.time')
$this->container->get('datetime.time'),
new NullBackend('test'),
86400
);
$this->expectDeprecation('The property cacheBackend (cache.default service) is deprecated in Drupal\media\OEmbed\ProviderRepository and will be removed before Drupal 10.0.0.');
$this->assertInstanceOf(CacheBackendInterface::class, $providers->cacheBackend);
// Ensure that the $max_age was properly set, even though it was passed in
// the logger factory's position.
$reflector = new \ReflectionClass($providers);
$property = $reflector->getProperty('maxAge');
$property->setAccessible(TRUE);
$this->assertSame(86400, $property->getValue($providers));
}
}
core/modules/media/tests/src/Unit/ProviderRepositoryTest.php 0 → 100644
+ 234
− 0
View file @ 26327f96
<?php
namespace Drupal\Tests\media\Unit;
use Drupal\Core\KeyValueStore\KeyValueMemoryFactory;
use Drupal\Core\Logger\LoggerChannelFactory;
use Drupal\media\OEmbed\ProviderException;
use Drupal\media\OEmbed\ProviderRepository;
use Drupal\Tests\UnitTestCase;
use GuzzleHttp\Client;
use GuzzleHttp\Handler\MockHandler;
use GuzzleHttp\HandlerStack;
use GuzzleHttp\Psr7\Response;
/**
* @coversDefaultClass \Drupal\media\OEmbed\ProviderRepository
*
* @group media
*/
class ProviderRepositoryTest extends UnitTestCase {
/**
* The provider repository under test.
*
* @var \Drupal\media\OEmbed\ProviderRepository
*/
private $repository;
/**
* The HTTP client handler which will serve responses.
*
* @var \GuzzleHttp\Handler\MockHandler
*/
private $responses;
/**
* The key-value store.
*
* @var \Drupal\Core\KeyValueStore\KeyValueStoreInterface
*/
private $keyValue;
/**
* The time that the current test began.
*
* @var int
*/
private $currentTime;
/**
* {@inheritdoc}
*/
protected function setUp(): void {
parent::setUp();
$config_factory = $this->getConfigFactoryStub([
'media.settings' => [
'oembed_providers_url' => 'https://oembed.com/providers.json',
],
]);
$key_value_factory = new KeyValueMemoryFactory();
$this->keyValue = $key_value_factory->get('media');
$this->currentTime = time();
$time = $this->prophesize('\Drupal\Component\Datetime\TimeInterface');
$time->getCurrentTime()->willReturn($this->currentTime);
$this->responses = new MockHandler();
$client = new Client([
'handler' => HandlerStack::create($this->responses),
]);
$this->repository = new ProviderRepository(
$client,
$config_factory,
$time->reveal(),
$key_value_factory,
new LoggerChannelFactory()
);
}
/**
* Tests that a successful fetch stores the provider database in key-value.
*/
public function testSuccessfulFetch(): void {
$body = <<<END
[
{
"provider_name": "YouTube",
"provider_url": "https:\/\/www.youtube.com\/",
"endpoints": [
{
"schemes": [
"https:\/\/*.youtube.com\/watch*",
"https:\/\/*.youtube.com\/v\/*"
],
"url": "https:\/\/www.youtube.com\/oembed",
"discovery": true
}
]
}
]
END;
$response = new Response(200, [], $body);
$this->responses->append($response);
$provider = $this->repository->get('YouTube');
$stored_data = [
'data' => [
'YouTube' => $provider,
],
'expires' => $this->currentTime + 604800,
];
$this->assertSame($stored_data, $this->keyValue->get('oembed_providers'));
}
/**
* Tests handling of invalid JSON when fetching the provider database.
*
* @param int $expiration_offset
* An offset to add to the current time to determine when the primed data,
* if any, expires.
*
* @dataProvider providerInvalidResponse
*/
public function testInvalidResponse(int $expiration_offset): void {
$provider = $this->prophesize('\Drupal\media\OEmbed\Provider')
->reveal();
// This stored data should be returned, irrespective of whether it's fresh.
$this->keyValue->set('oembed_providers', [
'data' => [
'YouTube' => $provider,
],
'expires' => $this->currentTime + $expiration_offset,
]);
$response = new Response(200, [], "This certainly isn't valid JSON.");
$this->responses->append($response, $response);
$this->assertSame($provider, $this->repository->get('YouTube'));
// When there is no stored data, we should get an exception.
$this->keyValue->delete('oembed_providers');
$this->expectException(ProviderException::class);
$this->repository->get('YouTube');
}
/**
* Data provider for ::testInvalidResponse().
*
* @return array[]
* Sets of arguments to pass to the test method.
*/
public function providerInvalidResponse(): array {
return [
'expired' => [
-86400,
],
'fresh' => [
86400,
],
];
}
/**
* Tests handling of exceptions when fetching the provider database.
*/
public function testRequestException(): void {
$provider = $this->prophesize('\Drupal\media\OEmbed\Provider')
->reveal();
// This data is expired (stale), but it should be returned anyway.
$this->keyValue->set('oembed_providers', [
'data' => [
'YouTube' => $provider,
],
'expires' => $this->currentTime - 86400,
]);
$response = new Response(503);
$this->responses->append($response, $response);
$this->assertSame($provider, $this->repository->get('YouTube'));
// When there is no stored data, we should get an exception.
$this->keyValue->delete('oembed_providers');
$this->expectException(ProviderException::class);
$this->repository->get('YouTube');
}
/**
* Tests a successful fetch but with a single corrupt item.
*/
public function testCorruptProviderIgnored(): void {
$body = <<<END
[
{
"provider_name": "YouTube",
"provider_url": "https:\/\/www.youtube.com\/",
"endpoints": [
{
"schemes": [
"https:\/\/*.youtube.com\/watch*",
"https:\/\/*.youtube.com\/v\/*"
],
"url": "https:\/\/www.youtube.com\/oembed",
"discovery": true
}
]
},
{
"provider_name": "Uncle Rico's football videos",
"provider_url": "not a real url",
"endpoints": []
}
]
END;
$response = new Response(200, [], $body);
$this->responses->append($response);
$youtube = $this->repository->get('YouTube');
// The corrupt provider should not be stored.
$stored_data = [
'data' => [
'YouTube' => $youtube,
],
'expires' => $this->currentTime + 604800,
];
$this->assertSame($stored_data, $this->keyValue->get('oembed_providers'));
$this->expectException('InvalidArgumentException');
$this->repository->get("Uncle Rico's football videos");
}
}
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment