[5.x] Always append original filenames to Glide URLs (#9616)

Co-authored-by: duncanmcclean <duncanmcclean@users.noreply.github.com> Co-authored-by: Jason Varga <jason@pixelfear.com>
statamic · Apr 16, 2024 · 4615e64 · 4615e64
1 parent 9eeaa42
commit 4615e64
Show file tree

Hide file tree

Showing 7 changed files with 41 additions and 76 deletions.
diff --git 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
@@ -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
@@ -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
@@ -39,15 +39,15 @@ 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;
             case 'asset':
                 $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
@@ -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
@@ -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
@@ -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));
     }
 
     /**