cdn.module | 34 ------------ cdn.services.yml | 4 +- src/File/FileUrlGenerator.php | 81 +++++++++++++++++++++++++--- tests/src/Unit/File/FileUrlGeneratorTest.php | 44 +++++++++++---- 4 files changed, 111 insertions(+), 52 deletions(-) diff --git a/cdn.module b/cdn.module index cad6811..20c8fac 100644 --- a/cdn.module +++ b/cdn.module @@ -30,40 +30,6 @@ function cdn_help($route_name, RouteMatchInterface $route_match) { } } -/** - * Implements hook_file_url_alter(). - */ -function cdn_file_url_alter(&$uri) { - // Don't alter file URLs when running update.php. - // @todo Remove the second condition after the CDN module requires the Drupal core minor that ships with https://www.drupal.org/project/drupal/issues/2969056 - if (defined('MAINTENANCE_MODE') || stripos($_SERVER['PHP_SELF'], 'update.php') !== FALSE) { - return; - } - - // Don't alter CSS file URLs while settings.php is disabling CSS aggregation. - if (substr($uri, -4) === '.css' && isset($GLOBALS['config']['system.performance']['css']['preprocess']) && $GLOBALS['config']['system.performance']['css']['preprocess'] === FALSE) { - return; - } - - // Don't serve CKEditor from a CDN when far future future is enabled (CKEditor - // insists on computing other assets to load based on this URL). - if ($uri === 'core/assets/vendor/ckeditor/ckeditor.js' && \Drupal::service('Drupal\cdn\CdnSettings')->farfutureIsEnabled()) { - return; - } - - // Don't alter file URLs while processing a CSS file. - // @see \Drupal\cdn\Asset\CssOptimizer - global $_cdn_in_css_file; - if ($_cdn_in_css_file) { - return; - } - - $result = \Drupal::service('Drupal\cdn\File\FileUrlGenerator')->generate($uri); - if ($result) { - $uri = $result; - } -} - /** * Implements hook_editor_js_settings_alter(). */ diff --git a/cdn.services.yml b/cdn.services.yml index 5c4e73b..ed68455 100644 --- a/cdn.services.yml +++ b/cdn.services.yml @@ -3,7 +3,9 @@ services: arguments: ['@config.factory'] Drupal\cdn\File\FileUrlGenerator: - arguments: ['%app.root%', '@stream_wrapper_manager', '@request_stack', '@private_key', '@Drupal\cdn\CdnSettings'] + public: false + decorates: file_url_generator + arguments: ['@Drupal\cdn\File\FileUrlGenerator.inner', '%app.root%', '@stream_wrapper_manager', '@request_stack', '@private_key', '@Drupal\cdn\CdnSettings'] # Event subscribers. Drupal\cdn\EventSubscriber\ConfigSubscriber: diff --git a/src/File/FileUrlGenerator.php b/src/File/FileUrlGenerator.php index e26ffa2..0be3e73 100644 --- a/src/File/FileUrlGenerator.php +++ b/src/File/FileUrlGenerator.php @@ -7,10 +7,12 @@ namespace Drupal\cdn\File; use Drupal\cdn\CdnSettings; use Drupal\Component\Utility\Crypt; use Drupal\Component\Utility\UrlHelper; +use Drupal\Core\File\FileUrlGeneratorInterface; use Drupal\Core\PrivateKey; use Drupal\Core\Site\Settings; use Drupal\Core\StreamWrapper\StreamWrapperManager; use Drupal\Core\StreamWrapper\StreamWrapperManagerInterface; +use Drupal\Core\Url; use Symfony\Component\HttpFoundation\RequestStack; /** @@ -18,10 +20,17 @@ use Symfony\Component\HttpFoundation\RequestStack; * * @see https://www.drupal.org/node/2669074 */ -class FileUrlGenerator { +class FileUrlGenerator implements FileUrlGeneratorInterface { const RELATIVE = ':relative:'; + /** + * The decorated file URL generator. + * + * @var \Drupal\Core\File\FileUrlGeneratorInterface + */ + protected $decorated; + /** * The app root. * @@ -60,6 +69,8 @@ class FileUrlGenerator { /** * Constructs a new CDN file URL generator object. * + * @param \Drupal\Core\File\FileUrlGeneratorInterface $decorated + * The decorated file URL generator. * @param string $root * The app root. * @param \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface $stream_wrapper_manager @@ -71,7 +82,8 @@ class FileUrlGenerator { * @param \Drupal\cdn\CdnSettings $cdn_settings * The CDN settings service. */ - public function __construct($root, StreamWrapperManagerInterface $stream_wrapper_manager, RequestStack $request_stack, PrivateKey $private_key, CdnSettings $cdn_settings) { + public function __construct(FileUrlGeneratorInterface $decorated, $root, StreamWrapperManagerInterface $stream_wrapper_manager, RequestStack $request_stack, PrivateKey $private_key, CdnSettings $cdn_settings) { + $this->decorated = $decorated; $this->root = $root; $this->streamWrapperManager = $stream_wrapper_manager; $this->requestStack = $request_stack; @@ -79,6 +91,35 @@ class FileUrlGenerator { $this->settings = $cdn_settings; } + /** + * {@inheritdoc} + */ + public function generateString(string $uri): string { + return $this->doGenerate($uri) ?? $this->decorated->generateString($uri); + } + + /** + * {@inheritdoc} + */ + public function generateAbsoluteString(string $uri): string { + return $this->doGenerate($uri) ?? $this->decorated->generateAbsoluteString($uri); + } + + /** + * {@inheritdoc} + */ + public function generate(string $uri): Url { + $result = $this->doGenerate($uri); + return $result ? Url::fromUri($result) : $this->decorated->generate($uri); + } + + /** + * {@inheritdoc} + */ + public function transformRelative(string $file_url, bool $root_relative = TRUE): string { + return $this->transformRelative($file_url, $root_relative); + } + /** * Generates a CDN file URL for local files that are mapped to a CDN. * @@ -95,22 +136,46 @@ class FileUrlGenerator { * The URI to a file for which we need a CDN URL, or the path to a shipped * file. * - * @return string|false - * A string containing the scheme-relative CDN file URI, or FALSE if this + * @return string|null + * A string containing the scheme-relative CDN file URI, or NULL if this * file URI should not be served from a CDN. */ - public function generate(string $uri) { + public function doGenerate(string $uri): ?string { + // Don't alter file URLs when running update.php. + // @todo Remove the second condition after the CDN module requires the Drupal core minor that ships with https://www.drupal.org/project/drupal/issues/2969056 + if (defined('MAINTENANCE_MODE') || stripos($_SERVER['PHP_SELF'], 'update.php') !== FALSE) { + return NULL; + } + + // Don't alter CSS file URLs while settings.php is disabling CSS aggregation. + if (substr($uri, -4) === '.css' && isset($GLOBALS['config']['system.performance']['css']['preprocess']) && $GLOBALS['config']['system.performance']['css']['preprocess'] === FALSE) { + return NULL; + } + + // Don't alter file URLs while processing a CSS file. + // @see \Drupal\cdn\Asset\CssOptimizer + global $_cdn_in_css_file; + if ($_cdn_in_css_file) { + return NULL; + } + if (!$this->settings->isEnabled()) { - return FALSE; + return NULL; } if (!$this->canServe($uri)) { - return FALSE; + return NULL; + } + + // Don't serve CKEditor from a CDN when far future future is enabled (CKEditor + // insists on computing other assets to load based on this URL). + if ($uri === 'core/assets/vendor/ckeditor/ckeditor.js' && $this->settings->farfutureIsEnabled()) { + return NULL; } $cdn_domain = $this->getCdnDomain($uri); if ($cdn_domain === FALSE) { - return FALSE; + return NULL; } if (!$scheme = StreamWrapperManager::getScheme($uri)) { diff --git a/tests/src/Unit/File/FileUrlGeneratorTest.php b/tests/src/Unit/File/FileUrlGeneratorTest.php index 1706e51..28f2bdb 100644 --- a/tests/src/Unit/File/FileUrlGeneratorTest.php +++ b/tests/src/Unit/File/FileUrlGeneratorTest.php @@ -2,6 +2,7 @@ namespace Drupal\Tests\cdn\Unit\File; +use Drupal\Core\File\FileUrlGenerator as CoreFileUrlGenerator; use Drupal\cdn\CdnSettings; use Drupal\cdn\File\FileUrlGenerator; use Drupal\Component\Utility\Crypt; @@ -12,6 +13,7 @@ use Drupal\Core\StreamWrapper\LocalStream; use Drupal\Core\StreamWrapper\PublicStream; use Drupal\Core\StreamWrapper\StreamWrapperInterface; use Drupal\Core\StreamWrapper\StreamWrapperManagerInterface; +use Drupal\Core\Url; use Drupal\Tests\UnitTestCase; use Prophecy\Argument; use Symfony\Component\HttpFoundation\Request; @@ -23,6 +25,13 @@ use Symfony\Component\HttpFoundation\RequestStack; */ class FileUrlGeneratorTest extends UnitTestCase { + /** + * A hardcoded return value for the mocked core file URL generator service. + * + * @var string + */ + const DECORATED_SERVICE_RETURNED_URL = '//decorated'; + /** * The private key to use in tests. * @@ -43,7 +52,10 @@ class FileUrlGeneratorTest extends UnitTestCase { } /** + * @covers ::generateString * @covers ::generate + * @covers ::generateAbsoluteString + * @covers ::doGenerate * @dataProvider urlProvider */ public function testGenerate($scheme, $base_path, $uri, $expected_result) { @@ -78,7 +90,9 @@ class FileUrlGeneratorTest extends UnitTestCase { ], 'stream_wrappers' => ['public'], ]); - $this->assertSame($expected_result, $gen->generate($uri)); + $this->assertSame($expected_result, $gen->generateString($uri)); + $this->assertSame($expected_result, $gen->generateAbsoluteString($uri)); + $this->assertSame($expected_result, $gen->generate($uri)->toUriString()); } public function urlProvider() { @@ -221,12 +235,12 @@ class FileUrlGeneratorTest extends UnitTestCase { $cases_with_scheme = []; foreach ($cases as $description => $case) { foreach (['https://', 'http://', '//'] as $scheme) { - list($base_path, $uri, $expected_result) = $case; + [$base_path, $uri, $expected_result] = $case; $cases_with_scheme['scheme=' . $scheme . ', ' . $description] = [ $scheme, $base_path, $uri, - !is_string($expected_result) ? $expected_result : $scheme . substr($expected_result, 2), + !is_string($expected_result) ? static::DECORATED_SERVICE_RETURNED_URL : $scheme . substr($expected_result, 2), ]; } } @@ -235,7 +249,10 @@ class FileUrlGeneratorTest extends UnitTestCase { } /** + * @covers ::generateString * @covers ::generate + * @covers ::generateAbsoluteString + * @covers ::doGenerate */ public function testGenerateFarfuture() { $config = [ @@ -266,23 +283,23 @@ class FileUrlGeneratorTest extends UnitTestCase { // In root: 1) non-existing file, 2) shipped file, 3) managed file. $gen = $this->createFileUrlGenerator('', $config); - $this->assertSame('//cdn.example.com/core/misc/does-not-exist.js', $gen->generate('core/misc/does-not-exist.js')); + $this->assertSame('//cdn.example.com/core/misc/does-not-exist.js', $gen->generateString('core/misc/does-not-exist.js')); $drupal_js_mtime = filemtime($this->root . '/core/misc/drupal.js'); $drupal_js_security_token = Crypt::hmacBase64($drupal_js_mtime . ':relative:' . UrlHelper::encodePath('/core/misc/drupal.js'), static::$privateKey . Settings::getHashSalt()); - $this->assertSame('//cdn.example.com/cdn/ff/' . $drupal_js_security_token . '/' . $drupal_js_mtime . '/:relative:/core/misc/drupal.js', $gen->generate('core/misc/drupal.js')); + $this->assertSame('//cdn.example.com/cdn/ff/' . $drupal_js_security_token . '/' . $drupal_js_mtime . '/:relative:/core/misc/drupal.js', $gen->generateString('core/misc/drupal.js')); // Since the public stream wrapper is not available in the unit test, // and we use file_exists() in the target method, we are using the // file:// scheme that ships with PHP. This does require // injecting a leading into the path that we compare against, to match // the method. $llama_jpg_security_token = Crypt::hmacBase64($llama_jpg_mtime . 'file' . UrlHelper::encodePath('/' . $llama_jpg_filepath), static::$privateKey . Settings::getHashSalt()); - $this->assertSame('//cdn.example.com/cdn/ff/' . $llama_jpg_security_token . '/' . $llama_jpg_mtime . '/file/' . UrlHelper::encodePath($llama_jpg_filepath), $gen->generate('file://' . $llama_jpg_filepath)); + $this->assertSame('//cdn.example.com/cdn/ff/' . $llama_jpg_security_token . '/' . $llama_jpg_mtime . '/file/' . UrlHelper::encodePath($llama_jpg_filepath), $gen->generateString('file://' . $llama_jpg_filepath)); // In subdir: 1) non-existing file, 2) shipped file, 3) managed file. $gen = $this->createFileUrlGenerator('/subdir', $config); - $this->assertSame('//cdn.example.com/subdir/core/misc/does-not-exist.js', $gen->generate('core/misc/does-not-exist.js')); - $this->assertSame('//cdn.example.com/subdir/cdn/ff/' . $drupal_js_security_token . '/' . $drupal_js_mtime . '/:relative:/core/misc/drupal.js', $gen->generate('core/misc/drupal.js')); - $this->assertSame('//cdn.example.com/subdir/cdn/ff/' . $llama_jpg_security_token . '/' . $llama_jpg_mtime . '/file/' . UrlHelper::encodePath($llama_jpg_filepath), $gen->generate('file://' . $llama_jpg_filepath)); + $this->assertSame('//cdn.example.com/subdir/core/misc/does-not-exist.js', $gen->generateString('core/misc/does-not-exist.js')); + $this->assertSame('//cdn.example.com/subdir/cdn/ff/' . $drupal_js_security_token . '/' . $drupal_js_mtime . '/:relative:/core/misc/drupal.js', $gen->generateString('core/misc/drupal.js')); + $this->assertSame('//cdn.example.com/subdir/cdn/ff/' . $llama_jpg_security_token . '/' . $llama_jpg_mtime . '/file/' . UrlHelper::encodePath($llama_jpg_filepath), $gen->generateString('file://' . $llama_jpg_filepath)); unlink($llama_jpg_filepath); } @@ -299,6 +316,14 @@ class FileUrlGeneratorTest extends UnitTestCase { * The FileUrlGenerator to test. */ protected function createFileUrlGenerator($base_path, array $raw_config) { + $core_file_url_generator = $this->prophesize(CoreFileUrlGenerator::class); + $core_file_url_generator->generate(Argument::type('string')) + ->willReturn(Url::fromUri(static::DECORATED_SERVICE_RETURNED_URL)); + $core_file_url_generator->generateString(Argument::type('string')) + ->willReturn(static::DECORATED_SERVICE_RETURNED_URL); + $core_file_url_generator->generateAbsoluteString(Argument::type('string')) + ->willReturn(static::DECORATED_SERVICE_RETURNED_URL); + $request = $this->prophesize(Request::class); $request->getBasePath() ->willReturn($base_path); @@ -349,6 +374,7 @@ class FileUrlGeneratorTest extends UnitTestCase { ->willReturn(static::$privateKey); return new FileUrlGenerator( + $core_file_url_generator->reveal(), $this->root, $stream_wrapper_manager->reveal(), $request_stack->reveal(),