From 4615e64f025790fcb2c86c348461db23880a3e97 Mon Sep 17 00:00:00 2001 From: Duncan McClean Date: Tue, 16 Apr 2024 19:45:26 +0100 Subject: [PATCH] [5.x] Always append original filenames to Glide URLs (#9616) Co-authored-by: duncanmcclean Co-authored-by: Jason Varga --- composer.json | 2 +- config/assets.php | 12 ------ src/Imaging/GlideManager.php | 54 ++++++++++++--------------- src/Imaging/GlideUrlBuilder.php | 18 +-------- tests/Imaging/GlideUrlBuilderTest.php | 7 +--- tests/Imaging/ImageGeneratorTest.php | 6 +-- tests/Tags/GlideTest.php | 18 ++++----- 7 files changed, 41 insertions(+), 76 deletions(-) diff --git a/composer.json b/composer.json index 195a33b3b6..e9b036bd63 100644 --- a/composer.json +++ b/composer.json @@ -18,7 +18,7 @@ "laravel/prompts": "^0.1.16", "league/commonmark": "^2.2", "league/csv": "^9.0", - "league/glide": "^2.0", + "league/glide": "^2.3", "maennchen/zipstream-php": "^3.1", "michelf/php-smartypants": "^1.8.1", "nesbot/carbon": "^2.62.1", diff --git a/config/assets.php b/config/assets.php index edb24aeccf..2ed388c032 100644 --- a/config/assets.php +++ b/config/assets.php @@ -114,18 +114,6 @@ 'generate_presets_on_upload' => true, - /* - |-------------------------------------------------------------------------- - | Append Original Filename - |-------------------------------------------------------------------------- - | - | Append the original filename to Glide generated URLs. - | This helps with Search Engine Optimization. - | - */ - - 'append_original_filename' => false, - ], /* diff --git a/src/Imaging/GlideManager.php b/src/Imaging/GlideManager.php index 84ed30be2b..7175e258e3 100644 --- a/src/Imaging/GlideManager.php +++ b/src/Imaging/GlideManager.php @@ -30,30 +30,28 @@ public function server(array $config = []) 'watermarks' => public_path(), ], $config)); - if (config('statamic.assets.image_manipulation.append_original_filename', false)) { - $server - ->setCachePathCallable(function ($path, $params) { - // to avoid having to recreate the getCachePath method from glide server - // we run getCachePath again without this callback function - $customCallable = $this->getCachePathCallable(); + $server + ->setCachePathCallable(function ($path, $params) { + // to avoid having to recreate the getCachePath method from glide server + // we run getCachePath again without this callback function + $customCallable = $this->getCachePathCallable(); - $this->setCachePathCallable(null); - $cachePath = $this->getCachePath($path, $params); - $this->setCachePathCallable($customCallable); + $this->setCachePathCallable(null); + $cachePath = $this->getCachePath($path, $params); + $this->setCachePathCallable($customCallable); - // then we append our original filename to the end - $filename = Str::afterLast($cachePath, '/'); - $cachePath = Str::beforeLast($cachePath, '/'); + // then we append our original filename to the end + $filename = Str::afterLast($cachePath, '/'); + $cachePath = Str::beforeLast($cachePath, '/'); - $cachePath .= '/'.Str::beforeLast($filename, '.').'/'.Str::of($path)->after('/'); + $cachePath .= '/'.Str::beforeLast($filename, '.').'/'.pathinfo($path, PATHINFO_BASENAME); - if ($extension = ($params['fm'] ?? false)) { - $cachePath = Str::beforeLast($cachePath, '.').'.'.$extension; - } + if ($extension = ($params['fm'] ?? false)) { + $cachePath = Str::beforeLast($cachePath, '.').'.'.$extension; + } - return $cachePath; - }); - } + return $cachePath; + }); return $server; } @@ -142,17 +140,13 @@ public function clearAsset($asset) $manifestKey = ImageGenerator::assetCacheManifestKey($asset); // Delete generated glide cache for asset. - if (config('statamic.assets.image_manipulation.append_original_filename', false)) { - // Make sure to use the default cache path when clearing the cache - tap($this->server(), function ($server) use ($pathPrefix, $asset) { - $customCallable = $server->getCachePathCallable(); - $server->setCachePathCallable(null); - $server->deleteCache($pathPrefix.'/'.$asset->path()); - $server->setCachePathCallable($customCallable); - }); - } else { - $this->server()->deleteCache($pathPrefix.'/'.$asset->path()); - } + // Make sure to use the default cache path when clearing the cache + tap($this->server(), function ($server) use ($pathPrefix, $asset) { + $customCallable = $server->getCachePathCallable(); + $server->setCachePathCallable(null); + $server->deleteCache($pathPrefix.'/'.$asset->path()); + $server->setCachePathCallable($customCallable); + }); // Use manifest to clear each manipulation key from cache store. collect($this->cacheStore()->get($manifestKey, []))->each(function ($manipulationKey) { diff --git a/src/Imaging/GlideUrlBuilder.php b/src/Imaging/GlideUrlBuilder.php index 4b22442003..fe7afa2068 100644 --- a/src/Imaging/GlideUrlBuilder.php +++ b/src/Imaging/GlideUrlBuilder.php @@ -39,7 +39,7 @@ public function build($item, $params, $filename = null) $path = 'http/'.base64_encode($item); if (! $filename) { - $filename = $this->optionallySetFilename(Str::afterLast($item, '/')); + $filename = Str::afterLast($item, '/'); } break; @@ -47,7 +47,7 @@ public function build($item, $params, $filename = null) $path = 'asset/'.base64_encode($this->item->containerId().'/'.$this->item->path()); if (! $filename) { - $filename = $this->optionallySetFilename(Str::afterLast($this->item->path(), '/')); + $filename = Str::afterLast($this->item->path(), '/'); } break; @@ -74,18 +74,4 @@ public function build($item, $params, $filename = null) return URL::prependSiteRoot($builder->getUrl($path, $params)); } - - /** - * Should the filename be set based on the config setting - * - * @return bool|string - */ - private function optionallySetFilename(string $filename) - { - if (! config('statamic.assets.image_manipulation.append_original_filename', false)) { - return false; - } - - return $filename; - } } diff --git a/tests/Imaging/GlideUrlBuilderTest.php b/tests/Imaging/GlideUrlBuilderTest.php index c60955cde8..1ebd12b5e3 100644 --- a/tests/Imaging/GlideUrlBuilderTest.php +++ b/tests/Imaging/GlideUrlBuilderTest.php @@ -4,7 +4,6 @@ use Statamic\Assets\Asset; use Statamic\Assets\AssetContainer; -use Statamic\Facades\Config; use Statamic\Imaging\GlideUrlBuilder; use Tests\TestCase; @@ -44,7 +43,7 @@ public function testPathWithSpace() public function testExternal() { $this->assertEquals( - '/img/http/'.base64_encode('http://example.com').'?w=100', + '/img/http/'.base64_encode('http://example.com').'/example.com?w=100', $this->builder->build('http://example.com', ['w' => '100']) ); } @@ -58,7 +57,7 @@ public function testAsset() $encoded = base64_encode('main/img/foo.jpg'); $this->assertEquals( - "/img/asset/$encoded?w=100", + "/img/asset/$encoded/foo.jpg?w=100", $this->builder->build($asset, ['w' => '100']) ); } @@ -83,8 +82,6 @@ public function testFilename() public function testConfigAddsFilename() { - Config::set('statamic.assets.image_manipulation.append_original_filename', true); - $asset = new Asset; $asset->container((new AssetContainer)->handle('main')); $asset->path('img/foo.jpg'); diff --git a/tests/Imaging/ImageGeneratorTest.php b/tests/Imaging/ImageGeneratorTest.php index 8feaa7aa14..97fabc4836 100644 --- a/tests/Imaging/ImageGeneratorTest.php +++ b/tests/Imaging/ImageGeneratorTest.php @@ -71,7 +71,7 @@ public function it_generates_an_image_by_asset() $expectedCacheManifest = [$manipulationCacheKey]; $expectedPathPrefix = 'containers/test_container'; - $expectedPath = "{$expectedPathPrefix}/foo/hoff.jpg/{$md5}.jpg"; + $expectedPath = "{$expectedPathPrefix}/foo/hoff.jpg/{$md5}/hoff.jpg"; $this->assertEquals($manifestCacheKey, ImageGenerator::assetCacheManifestKey($asset)); $this->assertEquals($expectedPathPrefix, ImageGenerator::assetCachePathPrefix($asset)); @@ -152,7 +152,7 @@ public function it_generates_an_image_by_local_path() // way it does. It will not include the fit parameter since it's not an asset. $md5 = $this->getGlideMd5($imagePath, $userParams); - $expectedPath = "paths/testimages/foo/hoff.jpg/{$md5}.jpg"; + $expectedPath = "paths/testimages/foo/hoff.jpg/{$md5}/hoff.jpg"; $this->assertEquals($expectedPath, $path); $this->assertCount(1, $paths = $this->generatedImagePaths()); @@ -200,7 +200,7 @@ public function it_generates_an_image_by_external_url() // While writing this test I noticed that we don't include the domain in the // cache path, so the same file path on two different domains will conflict. // TODO: Fix this. - $expectedPath = "http/foo/hoff.jpg/{$md5}.jpg"; + $expectedPath = "http/foo/hoff.jpg/{$md5}/hoff.jpg"; $this->assertEquals($expectedPath, $path); $this->assertCount(1, $paths = $this->generatedImagePaths()); diff --git a/tests/Tags/GlideTest.php b/tests/Tags/GlideTest.php index a298948754..00f8198c8d 100644 --- a/tests/Tags/GlideTest.php +++ b/tests/Tags/GlideTest.php @@ -18,9 +18,9 @@ public function it_outputs_a_relative_url_by_default_when_the_glide_route_is_rel { $this->createImageInPublicDirectory(); - $this->assertEquals('/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2.jpg', $this->absoluteTestTag()); - $this->assertEquals('/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2.jpg', $this->absoluteTestTag(false)); - $this->assertEquals('http://localhost/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2.jpg', $this->absoluteTestTag(true)); + $this->assertEquals('/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2/bar.jpg', $this->absoluteTestTag()); + $this->assertEquals('/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2/bar.jpg', $this->absoluteTestTag(false)); + $this->assertEquals('http://localhost/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2/bar.jpg', $this->absoluteTestTag(true)); } /** @@ -32,9 +32,9 @@ public function it_outputs_an_absolute_url_by_default_when_the_glide_route_is_ab { $this->createImageInPublicDirectory(); - $this->assertEquals('http://localhost/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2.jpg', $this->absoluteTestTag()); - $this->assertEquals('/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2.jpg', $this->absoluteTestTag(false)); - $this->assertEquals('http://localhost/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2.jpg', $this->absoluteTestTag(true)); + $this->assertEquals('http://localhost/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2/bar.jpg', $this->absoluteTestTag()); + $this->assertEquals('/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2/bar.jpg', $this->absoluteTestTag(false)); + $this->assertEquals('http://localhost/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2/bar.jpg', $this->absoluteTestTag(true)); } /** @@ -46,9 +46,9 @@ public function it_outputs_an_absolute_url_by_default_when_the_glide_route_is_ab { $this->createImageInPublicDirectory(); - $this->assertEquals('https://localhost/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2.jpg', $this->absoluteTestTag()); - $this->assertEquals('/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2.jpg', $this->absoluteTestTag(false)); - $this->assertEquals('https://localhost/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2.jpg', $this->absoluteTestTag(true)); + $this->assertEquals('https://localhost/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2/bar.jpg', $this->absoluteTestTag()); + $this->assertEquals('/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2/bar.jpg', $this->absoluteTestTag(false)); + $this->assertEquals('https://localhost/glide/paths/bar.jpg/689e9cd88cc1d852c9a4d3a1e27d68c2/bar.jpg', $this->absoluteTestTag(true)); } /**