Merge pull request #585 from creative-commoners/pulls/2/file-conversi…

…on-variants

NEW Allow file variants with different extensions

src/FilenameParsing/AbstractFileIDHelper.php

@@ -2,12 +2,29 @@
 
 namespace SilverStripe\Assets\FilenameParsing;
 
+use SilverStripe\Core\Convert;
 use SilverStripe\Core\Injector\Injectable;
+use SilverStripe\Core\Path;
 
 abstract class AbstractFileIDHelper implements FileIDHelper
 {
     use Injectable;
 
+    /**
+     * A variant type for encoding a variant filename with a different extension than the original.
+     */
+    public const EXTENSION_REWRITE_VARIANT = 'ExtRewrite';
+
+    /**
+     * Use the original file's extension
+     */
+    protected const EXTENSION_ORIGINAL = 0;
+
+    /**
+     * Use the variant file's extension
+     */
+    protected const EXTENSION_VARIANT = 1;
+
     public function buildFileID($filename, $hash = null, $variant = null, $cleanfilename = true)
     {
         if ($filename instanceof ParsedFileID) {
@@ -23,6 +40,10 @@ public function buildFileID($filename, $hash = null, $variant = null, $cleanfile
             $filename = $this->cleanFilename($filename);
         }
 
+        if ($variant) {
+            $filename = $this->swapExtension($filename, $variant, self::EXTENSION_VARIANT);
+        }
+
         $name = basename($filename ?? '');
 
         // Split extension
@@ -53,6 +74,48 @@ public function buildFileID($filename, $hash = null, $variant = null, $cleanfile
         return $fileID;
     }
 
+    /**
+     * Get the original file's filename with the extension rewritten to be the same as either the original
+     * or the variant extension.
+     *
+     * @param string $filename Original filename without variant
+     * @param int $extIndex One of self::EXTENSION_ORIGINAL or self::EXTENSION_VARIANT
+     */
+    protected function swapExtension(string $filename, string $variant, int $extIndex): string
+    {
+        // If there's no variant at all, we can rewrite the filenmane
+        if (empty($variant)) {
+            return $filename;
+        }
+
+        // Split variant string in variant list
+        $subVariants = explode('_', $variant);
+
+        // Split our filename into a filename and extension part
+        $fileParts = pathinfo($filename);
+        if (!isset($fileParts['filename']) || !isset($fileParts['extension'])) {
+            return $filename;
+        }
+        $dirname = $fileParts['dirname'] !== '.' ? $fileParts['dirname'] : '';
+        $filenameWithoutExtension = Path::join($dirname, $fileParts['filename']);
+        $extension = $fileParts['extension'];
+
+        // Loop our variant list until we find our special file extension swap variant
+        // Reverse the list first so the variant extension we find is the last extension rewrite variant in a chain
+        $extSwapVariant = preg_quote(self::EXTENSION_REWRITE_VARIANT, '/');
+        foreach (array_reverse($subVariants) as $subVariant) {
+            if (preg_match("/^$extSwapVariant(?<base64>.+)$/", $subVariant, $matches)) {
+                // This array always contain 2 values: The original extension at index 0 and the variant extension at index 1
+                /** @var array $extensionData */
+                $extensionData = Convert::base64url_decode($matches['base64']);
+                $extension = $extensionData[$extIndex];
+                break;
+            }
+        }
+
+        return $filenameWithoutExtension . '.' . $extension;
+    }
+
     public function cleanFilename($filename)
     {
         // Swap backslash for forward slash

src/FilenameParsing/FileIDHelper.php

@@ -7,7 +7,6 @@
  */
 interface FileIDHelper
 {
-
     /**
      * Map file tuple (hash, name, variant) to a filename to be used by flysystem
      *

src/FilenameParsing/HashFileIDHelper.php

@@ -30,10 +30,16 @@ public function parseFileID($fileID)
         }
 
         $filename = $matches['folder'] . $matches['basename'] . $matches['extension'];
+        $variant = $matches['variant'] ?: '';
+
+        if ($variant) {
+            $filename = $this->swapExtension($filename, $variant, self::EXTENSION_ORIGINAL);
+        }
+
         return new ParsedFileID(
             $filename,
             $matches['hash'],
-            isset($matches['variant']) ? $matches['variant'] : '',
+            $variant,
             $fileID
         );
     }

src/FilenameParsing/NaturalFileIDHelper.php

@@ -2,8 +2,6 @@
 
 namespace SilverStripe\Assets\FilenameParsing;
 
-use SilverStripe\Core\Injector\Injectable;
-
 /**
  * Parsed Natural path URLs. Natural path is the same hashless path that appears in the CMS.
  *
@@ -23,10 +21,16 @@ public function parseFileID($fileID)
         }
 
         $filename = $matches['folder'] . $matches['basename'] . $matches['extension'];
+        $variant = $matches['variant'] ?: '';
+
+        if ($variant) {
+            $filename = $this->swapExtension($filename, $variant, self::EXTENSION_ORIGINAL);
+        }
+
         return new ParsedFileID(
             $filename,
             '',
-            isset($matches['variant']) ? $matches['variant'] : '',
+            $variant,
             $fileID
         );
     }

src/ImageBackendFactory.php

@@ -37,9 +37,15 @@ public function __construct(Factory $creator)
      */
     public function create($service, array $params = [])
     {
-        /** @var AssetContainer $assetContainer */
+        /** @var AssetContainer|null $assetContainer */
         $assetContainer = reset($params);
-        if (!$assetContainer instanceof AssetContainer) {
+
+        // If no asset container was passed in, create a new uncached image backend
+        if (!$assetContainer) {
+            return $this->creator->create($service, $params);
+        }
+
+        if (!($assetContainer instanceof AssetContainer)) {
             throw new BadMethodCallException("Can only create Image_Backend for " . AssetContainer::class);
         }
 

src/ImageManipulation.php

@@ -4,6 +4,7 @@
 
 use InvalidArgumentException;
 use LogicException;
+use SilverStripe\Assets\FilenameParsing\AbstractFileIDHelper;
 use SilverStripe\Assets\Storage\AssetContainer;
 use SilverStripe\Assets\Storage\AssetStore;
 use SilverStripe\Assets\Storage\DBFile;
@@ -840,13 +841,25 @@ public function isHeight($height)
         return $this->getHeight() === $height;
     }
 
+    /**
+     * Wrapper for manipulate() that creates a variant file with a different extension than the original file.
+     *
+     * @return DBFile|null The manipulated file
+     */
+    public function manipulateExtension(string $newExtension, callable $callback)
+    {
+        $pathParts = pathinfo($this->getFilename());
+        $variant = $this->variantName(AbstractFileIDHelper::EXTENSION_REWRITE_VARIANT, $pathParts['extension'], $newExtension);
+        return $this->manipulate($variant, $callback);
+    }
+
     /**
      * Wrapper for manipulate that passes in and stores Image_Backend objects instead of tuples
      *
      * @param string $variant
      * @param callable $callback Callback which takes an Image_Backend object, and returns an Image_Backend result.
      * If this callback returns `true` then the current image will be duplicated without modification.
-     * @return DBFile The manipulated file
+     * @return DBFile|null The manipulated file
      */
     public function manipulateImage($variant, $callback)
     {
@@ -914,7 +927,7 @@ function (AssetStore $store, $filename, $hash, $variant) use ($callback) {
      * This callback will be passed the backend, filename, hash, and variant
      * This will not be called if the file does not
      * need to be created.
-     * @return DBFile The manipulated file
+     * @return DBFile|null The manipulated file
      */
     public function manipulate($variant, $callback)
     {
@@ -999,6 +1012,7 @@ public function variantName($format, $arg = null)
      * For legacy reasons, there's no delimiter between this part, and the encoded arguments.
      * This means we have to use a whitelist of "known formats", based on methods
      * available on the {@link Image} class as the "main" user of this trait.
+     * The one exception to this is the variant for swapping file extensions, which is explicitly allowed.
      * This class is commonly decorated with additional manipulation methods through {@link DataExtension}.
      *
      * @param $variantName
@@ -1007,10 +1021,11 @@ public function variantName($format, $arg = null)
      */
     public function variantParts($variantName)
     {
-        $methods = array_map('preg_quote', singleton(Image::class)->allMethodNames() ?? []);
+        $allowedVariantTypes = array_map('preg_quote', singleton(Image::class)->allMethodNames() ?? []);
+        $allowedVariantTypes[] = preg_quote(AbstractFileIDHelper::EXTENSION_REWRITE_VARIANT);
 
         // Regex needs to be case insensitive since allMethodNames() is all lowercased
-        $regex = '#^(?<format>(' . implode('|', $methods) . '))(?<encodedargs>(.*))#i';
+        $regex = '#^(?<format>(' . implode('|', $allowedVariantTypes) . '))(?<encodedargs>(.*))#i';
         preg_match($regex ?? '', $variantName ?? '', $matches);
 
         if (!$matches) {
@@ -1022,7 +1037,7 @@ public function variantParts($variantName)
             throw new InvalidArgumentException('Invalid variant name arguments: ' . $variantName);
         }
 
-        return array_merge([$matches['format']], $args[0]);
+        return array_merge([$matches['format']], $args);
     }
 
     /**

src/InterventionBackend.php

@@ -366,8 +366,10 @@ public function writeToStore(AssetStore $assetStore, $filename, $hash = null, $v
                 throw new BadMethodCallException("Cannot write corrupt file to store");
             }
 
+            // Make sure we're using the extension of the variant file, which can differ from the original file
+            $url = $assetStore->getAsURL($filename, $hash, $variant, false);
+            $extension = pathinfo($url, PATHINFO_EXTENSION);
             // Save file
-            $extension = pathinfo($filename ?? '', PATHINFO_EXTENSION);
             $result = $assetStore->setFromString(
                 $resource->encode($extension, $this->getQuality())->getEncoded(),
                 $filename,

tests/php/FilenameParsing/HashFileIDHelperTest.php

@@ -2,8 +2,11 @@
 namespace SilverStripe\Assets\Tests\FilenameParsing;
 
 use InvalidArgumentException;
+use ReflectionClassConstant;
+use ReflectionMethod;
 use SilverStripe\Assets\FilenameParsing\HashFileIDHelper;
 use SilverStripe\Assets\FilenameParsing\ParsedFileID;
+use SilverStripe\Core\Convert;
 
 class HashFileIDHelperTest extends FileIDHelperTester
 {
@@ -173,4 +176,48 @@ public function testHashlessBuildFileID()
         $this->expectException(\InvalidArgumentException::class);
         $this->getHelper()->buildFileID('Filename.txt', '');
     }
+
+    public function provideRewriteExtension()
+    {
+        $jpgToPng = 'ExtRewrite' . Convert::base64url_encode(['jpg', 'png']);
+
+        return [
+            'no variant' => ['', 'hel_lo.txt', 'hel_lo.txt'],
+            'invalid extension' => ['xyz', 'h_ello.abc+', 'h_ello.abc+'],
+            'no extension' => ['', 'hell_o.', 'hell_o.'],
+            'no filename' => ['', '.hta_ccess', '.hta_ccess'],
+            'no rewrite' => ['xyz', 'hell_o.jpg', 'hell_o.jpg'],
+            'no rewrite multi variant' => ['xyz_abc', 'he_llo.jpg', 'he_llo.jpg'],
+            'rewitten extension' => [$jpgToPng, 'hel_lo.jpg', 'hel_lo.png'],
+            'rewitten extension with other variants' => ["{$jpgToPng}_xyz", 'hel_lo.jpg', 'hel_lo.png'],
+        ];
+    }
+
+    /**
+     * @dataProvider provideRewriteExtension
+     */
+    public function testRewriteVariantExtension($variant, $inFilename, $outFilename)
+    {
+        $helper = new HashFileIDHelper();
+        $reflectionMethod = new ReflectionMethod($helper, 'swapExtension');
+        $reflectionMethod->setAccessible(true);
+        $reflectionConst = new ReflectionClassConstant($helper, 'EXTENSION_VARIANT');
+        $actualFilename = $reflectionMethod->invoke($helper, $inFilename, $variant, $reflectionConst->getValue());
+
+        $this->assertEquals($outFilename, $actualFilename);
+    }
+
+    /**
+     * @dataProvider provideRewriteExtension
+     */
+    public function testRestoreOriginalExtension($variant, $outFilename, $inFilename)
+    {
+        $helper = new HashFileIDHelper();
+        $reflectionMethod = new ReflectionMethod($helper, 'swapExtension');
+        $reflectionMethod->setAccessible(true);
+        $reflectionConst = new ReflectionClassConstant($helper, 'EXTENSION_ORIGINAL');
+        $actualFilename = $reflectionMethod->invoke($helper, $inFilename, $variant, $reflectionConst->getValue());
+
+        $this->assertEquals($outFilename, $actualFilename);
+    }
 }