From 172de0b957c9ff16e67fa6231e8913506c0d6568 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Thu, 26 Sep 2019 17:26:52 +0100 Subject: [PATCH] resourceloader: Add version to ResourceLoaderImage urls for long-cache The code previously here did not work well as it merely forwarded the hash from the current web request. This had numerous issues: 1. It was often null because requests for stylesheets do not cary a version hash. 2. When requested by JavaScript, the version hash would be a combination-hash of many unrelated modules, thus when requested as part of different batches, it would produce different urls which is not ideal. The impact of this is minimal currently because we basically never use these urls, as SVGs are almost always embedded instead of ref'ed by url. PNG urls are only generated for non-JS modules and then only used in older browsers not supporting SVG. And, even after all that, for the edge case of an SVG being ref'ed by url, they would be stored in LocalStorage by mw.loader with the name+version of the module the image belonged to, not the version hash of the batch request it came with. Which means that, yes, localstorage key for "somemodule+someversion" would have different values for different users, based on which batch the value came with, because the image urls were using the version hash of the batch request from ResourceLoaderContext. This is weird, but didn't cause bugs or inefficiencies because the user would never be exposed to the other possible urls for that image because we always check LocalStorage first. It did cause fragmentation server-side in Varnish, though. This is all fixed now by always including a version, and setting it to the version of the module. This means there is no more Varnish fragmentation for these. And it means that browsers are now allowed to cache the images served from these urls for 30+ days (immutable) instead of only 5min, which is what happened when they didn't have a version parameter (or set to null). Bug: T233343 Change-Id: I4af7fda03698ed4c288d154e7787fb2f3cbbe6c5 --- includes/resourceloader/ResourceLoaderImage.php | 10 ++++++---- .../resourceloader/ResourceLoaderImageModule.php | 4 ++++ .../resourceloader/ResourceLoaderImageTest.php | 14 +------------- 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/includes/resourceloader/ResourceLoaderImage.php b/includes/resourceloader/ResourceLoaderImage.php index 2b0f5ed353..3878205220 100644 --- a/includes/resourceloader/ResourceLoaderImage.php +++ b/includes/resourceloader/ResourceLoaderImage.php @@ -55,8 +55,9 @@ class ResourceLoaderImage { private $extension; /** - * @param string $name Image name - * @param string $module Module name + * @param string $name Self-name of the image as known to ResourceLoaderImageModule. + * @param string $module Self-name of the module containing this image. + * Used to find the image in the registry e.g. through a load.php url. * @param string|array $descriptor Path to image file, or array structure containing paths * @param string $basePath Directory to which paths in descriptor refer * @param array $variants @@ -217,7 +218,7 @@ class ResourceLoaderImage { * @param string $script URL to load.php * @param string|null $variant Variant to get the URL for * @param string $format Format to get the URL for, 'original' or 'rasterized' - * @return string + * @return string URL */ public function getUrl( ResourceLoaderContext $context, $script, $variant, $format ) { $query = [ @@ -231,7 +232,8 @@ class ResourceLoaderImage { } // The following parameters are at the end to keep the original order of the parameters. $query['skin'] = $context->getSkin(); - $query['version'] = $context->getVersion(); + $rl = $context->getResourceLoader(); + $query['version'] = $rl->makeVersionQuery( $context, [ $this->getModule() ] ); return wfAppendQuery( $script, $query ); } diff --git a/includes/resourceloader/ResourceLoaderImageModule.php b/includes/resourceloader/ResourceLoaderImageModule.php index 0585cfdbe7..d46d26fdfc 100644 --- a/includes/resourceloader/ResourceLoaderImageModule.php +++ b/includes/resourceloader/ResourceLoaderImageModule.php @@ -358,6 +358,10 @@ class ResourceLoaderImageModule extends ResourceLoaderModule { } /** + * This method must not be used by getDefinitionSummary as doing so would cause + * an infinite loop (we use ResourceLoaderImage::getUrl below which calls + * Module:getVersionHash, which calls Module::getDefinitionSummary). + * * @param ResourceLoaderContext $context * @param ResourceLoaderImage $image Image to get the style for * @param string $script URL to load.php diff --git a/tests/phpunit/unit/includes/resourceloader/ResourceLoaderImageTest.php b/tests/phpunit/unit/includes/resourceloader/ResourceLoaderImageTest.php index 5265b3ee17..02042b8667 100644 --- a/tests/phpunit/unit/includes/resourceloader/ResourceLoaderImageTest.php +++ b/tests/phpunit/unit/includes/resourceloader/ResourceLoaderImageTest.php @@ -2,6 +2,7 @@ /** * @group ResourceLoader + * @covers ResourceLoaderImage */ class ResourceLoaderImageTest extends MediaWikiUnitTestCase { @@ -53,7 +54,6 @@ class ResourceLoaderImageTest extends MediaWikiUnitTestCase { } /** - * @covers ResourceLoaderImage::getPath * @dataProvider provideGetPath */ public function testGetPath( $imageName, $languageCode, $path ) { @@ -84,10 +84,6 @@ class ResourceLoaderImageTest extends MediaWikiUnitTestCase { $this->assertEquals( $image->getPath( $context ), $this->imagesPath . '/' . $path ); } - /** - * @covers ResourceLoaderImage::getExtension - * @covers ResourceLoaderImage::getMimeType - */ public function testGetExtension() { $image = $this->getTestImage( 'def' ); $this->assertEquals( $image->getExtension(), 'svg' ); @@ -99,11 +95,6 @@ class ResourceLoaderImageTest extends MediaWikiUnitTestCase { $this->assertEquals( $image->getExtension( 'rasterized' ), 'gif' ); } - /** - * @covers ResourceLoaderImage::getImageData - * @covers ResourceLoaderImage::variantize - * @covers ResourceLoaderImage::massageSvgPathdata - */ public function testGetImageData() { $context = $this->createMock( ResourceLoaderContext::class ); @@ -124,9 +115,6 @@ class ResourceLoaderImageTest extends MediaWikiUnitTestCase { $this->assertEquals( $image->getImageData( $context, null, 'rasterized' ), $data ); } - /** - * @covers ResourceLoaderImage::massageSvgPathdata - */ public function testMassageSvgPathdata() { $image = $this->getTestImage( 'ghi' ); $data = file_get_contents( $this->imagesPath . '/ghi.svg' ); -- 2.20.1