diff --git a/src/FilenameParsing/AbstractFileIDHelper.php b/src/FilenameParsing/AbstractFileIDHelper.php index a928d938..2e55fbb9 100644 --- a/src/FilenameParsing/AbstractFileIDHelper.php +++ b/src/FilenameParsing/AbstractFileIDHelper.php @@ -2,12 +2,28 @@ namespace SilverStripe\Assets\FilenameParsing; +use SilverStripe\Core\Convert; use SilverStripe\Core\Injector\Injectable; 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 + */ + private const EXT_ORIGINAL = 0; + + /** + * use the variant file's extension + */ + private const EXT_VARIANT = 1; + public function buildFileID($filename, $hash = null, $variant = null, $cleanfilename = true) { if ($filename instanceof ParsedFileID) { @@ -23,6 +39,10 @@ public function buildFileID($filename, $hash = null, $variant = null, $cleanfile $filename = $this->cleanFilename($filename); } + if ($variant) { + $filename = $this->rewriteVariantExtension($filename, $variant); + } + $name = basename($filename ?? ''); // Split extension @@ -53,6 +73,66 @@ public function buildFileID($filename, $hash = null, $variant = null, $cleanfile return $fileID; } + /** + * Get the original filename with the extension rewritten to be the same as the variant file extension. + * + * @param string $filename Original filename without variant + */ + public function rewriteVariantExtension(string $filename, string $variant): string + { + return $this->swapExtension($filename, $variant, self::EXT_VARIANT); + } + + /** + * Get the variant filename with the extension rewritten to be the same as the original file extension. + * + * @param string $filename Original filename without variant + */ + public function restoreOriginalExtension(string $filename, string $variant): string + { + return $this->swapExtension($filename, $variant, self::EXT_ORIGINAL); + } + + /** + * 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::EXT_ORIGINAL or self::EXT_VARIANT + */ + private 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 + if (!preg_match('/(?.+)\.(?[a-z\d]+)$/i', $filename, $matches)) { + return $filename; + } + $filenameWithoutExtension = $matches['basename']; + $extension = $matches['ext']; + + // 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(?.+)$/", $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 diff --git a/src/FilenameParsing/FileIDHelper.php b/src/FilenameParsing/FileIDHelper.php index 8c7de879..f33148a3 100644 --- a/src/FilenameParsing/FileIDHelper.php +++ b/src/FilenameParsing/FileIDHelper.php @@ -7,7 +7,6 @@ */ interface FileIDHelper { - /** * Map file tuple (hash, name, variant) to a filename to be used by flysystem * diff --git a/src/FilenameParsing/HashFileIDHelper.php b/src/FilenameParsing/HashFileIDHelper.php index 3e2b3702..c2692e78 100644 --- a/src/FilenameParsing/HashFileIDHelper.php +++ b/src/FilenameParsing/HashFileIDHelper.php @@ -30,10 +30,16 @@ public function parseFileID($fileID) } $filename = $matches['folder'] . $matches['basename'] . $matches['extension']; + $variant = isset($matches['variant']) ? $matches['variant'] : ''; + + if (isset($variant)) { + $filename = $this->restoreOriginalExtension($filename, $variant); + } + return new ParsedFileID( $filename, $matches['hash'], - isset($matches['variant']) ? $matches['variant'] : '', + $variant, $fileID ); } diff --git a/src/FilenameParsing/NaturalFileIDHelper.php b/src/FilenameParsing/NaturalFileIDHelper.php index ea8309f2..02280da8 100644 --- a/src/FilenameParsing/NaturalFileIDHelper.php +++ b/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 = isset($matches['variant']) ? $matches['variant'] : ''; + + if (isset($variant)) { + $filename = $this->restoreOriginalExtension($filename, $variant); + } + return new ParsedFileID( $filename, '', - isset($matches['variant']) ? $matches['variant'] : '', + $variant, $fileID ); } diff --git a/src/ImageBackendFactory.php b/src/ImageBackendFactory.php index 81738ff7..8449e09e 100644 --- a/src/ImageBackendFactory.php +++ b/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); } diff --git a/src/ImageManipulation.php b/src/ImageManipulation.php index e7a74785..36506c68 100644 --- a/src/ImageManipulation.php +++ b/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,6 +841,18 @@ 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 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 * @@ -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 = '#^(?(' . implode('|', $methods) . '))(?(.*))#i'; + $regex = '#^(?(' . implode('|', $allowedVariantTypes) . '))(?(.*))#i'; preg_match($regex ?? '', $variantName ?? '', $matches); if (!$matches) { diff --git a/src/InterventionBackend.php b/src/InterventionBackend.php index 0758e662..1d3dea76 100644 --- a/src/InterventionBackend.php +++ b/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, diff --git a/tests/php/FilenameParsing/HashFileIDHelperTest.php b/tests/php/FilenameParsing/HashFileIDHelperTest.php index 616e890f..eb3753a5 100644 --- a/tests/php/FilenameParsing/HashFileIDHelperTest.php +++ b/tests/php/FilenameParsing/HashFileIDHelperTest.php @@ -4,6 +4,7 @@ use InvalidArgumentException; use SilverStripe\Assets\FilenameParsing\HashFileIDHelper; use SilverStripe\Assets\FilenameParsing\ParsedFileID; +use SilverStripe\Core\Convert; class HashFileIDHelperTest extends FileIDHelperTester { @@ -173,4 +174,42 @@ 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' => ['', 'hello.txt', 'hello.txt'], + 'invalid extension' => ['xyz', 'hello.abc+', 'hello.abc+'], + 'no extension' => ['', 'hello.', 'hello.'], + 'no filename' => ['', '.htaccess', '.htaccess'], + 'no rewrite' => ['xyz', 'hello.jpg', 'hello.jpg'], + 'no rewrite multi variant' => ['xyz_abc', 'hello.jpg', 'hello.jpg'], + 'rewitten extension' => [$jpgToPng, 'hello.jpg', 'hello.png'], + 'rewitten extension with other variants' => ["{$jpgToPng}_xyz", 'hello.jpg', 'hello.png'], + ]; + } + + /** + * @dataProvider provideRewriteExtension + */ + public function testRewriteVariantExtension($variant, $inFilename, $outFilename) + { + $helper = new HashFileIDHelper(); + $actualFilename = $helper->rewriteVariantExtension($inFilename, $variant); + + $this->assertEquals($outFilename, $actualFilename); + } + + /** + * @dataProvider provideRewriteExtension + */ + public function testRestoreOriginalExtension($variant, $outFilename, $inFilename) + { + $helper = new HashFileIDHelper(); + $actualFilename = $helper->restoreOriginalExtension($inFilename, $variant); + + $this->assertEquals($outFilename, $actualFilename); + } } diff --git a/tests/php/ImageManipulationTest.php b/tests/php/ImageManipulationTest.php index 3005c427..314ae1f2 100644 --- a/tests/php/ImageManipulationTest.php +++ b/tests/php/ImageManipulationTest.php @@ -5,8 +5,10 @@ use Prophecy\Prophecy\ObjectProphecy; use Silverstripe\Assets\Dev\TestAssetStore; use SilverStripe\Assets\File; +use SilverStripe\Assets\FilenameParsing\AbstractFileIDHelper; use SilverStripe\Assets\Folder; use SilverStripe\Assets\Image; +use SilverStripe\Assets\Image_Backend; use SilverStripe\Assets\InterventionBackend; use SilverStripe\Assets\Storage\AssetStore; use SilverStripe\Assets\Storage\DBFile; @@ -470,4 +472,88 @@ public function testThumbnailURL() 'Current user should not automatically be granted access to view thumbnail' ); } + + public function testManipulateExtension() + { + $image = $this->objFromFixture(Image::class, 'imageWithTitle'); + $manipulated = $image->manipulateExtension( + 'webp', + function (AssetStore $store, string $filename, string $hash, string $variant) use ($image) { + $backend = $image->getImageBackend(); + $tuple = $backend->writeToStore( + $store, + $filename, + $hash, + $variant, + ['conflict' => AssetStore::CONFLICT_USE_EXISTING] + ); + return [$tuple, $backend]; + } + ); + + $store = Injector::inst()->get(AssetStore::class); + + // Having a valid image backend means all the image manipulation methods can be chained on top + $this->assertInstanceOf(Image_Backend::class, $manipulated->getImageBackend()); + // Double check the variant was created and stored correctly + $this->assertSame([AbstractFileIDHelper::EXTENSION_REWRITE_VARIANT, 'png', 'webp'], $manipulated->variantParts($manipulated->getVariant())); + $this->assertTrue($store->exists($manipulated->getFilename(), $manipulated->getHash(), $manipulated->getVariant())); + } + + public function testManipulateExtensionNonImageToImage() + { + $original = $this->objFromFixture(File::class, 'notImage'); + $manipulated = $original->manipulateExtension( + 'png', + function (AssetStore $store, string $filename, string $hash, string $variant) { + $backend = Injector::inst()->create(Image_Backend::class); + // In lieu of actually generating a screenshot of the txt file and making an image from it, + // we'll just load an image from the filesystem. + $backend->loadFrom(__DIR__ . '/ImageTest/test-image.png'); + $tuple = $backend->writeToStore( + $store, + $filename, + $hash, + $variant, + ['conflict' => AssetStore::CONFLICT_USE_EXISTING] + ); + return [$tuple, $backend]; + } + ); + + $store = Injector::inst()->get(AssetStore::class); + + // Having a valid image backend means all the image manipulation methods can be chained on top + $this->assertInstanceOf(Image_Backend::class, $manipulated->getImageBackend()); + // Double check the variant was created and stored correctly + $this->assertSame([AbstractFileIDHelper::EXTENSION_REWRITE_VARIANT, 'txt', 'png'], $manipulated->variantParts($manipulated->getVariant())); + $this->assertTrue($store->exists($manipulated->getFilename(), $manipulated->getHash(), $manipulated->getVariant())); + } + + public function testManipulateExtensionNonImageToNonImage() + { + $original = $this->objFromFixture(File::class, 'notImage'); + $manipulated = $original->manipulateExtension( + 'csv', + function (AssetStore $store, string $filename, string $hash, string $variant) { + $tuple = $store->setFromString( + 'Any content will do - csv is just a text file afterall', + $filename, + $hash, + $variant, + ['conflict' => AssetStore::CONFLICT_USE_EXISTING] + ); + return [$tuple, null]; + } + ); + + $store = Injector::inst()->get(AssetStore::class); + + // Backend should be null since the resulting variant isn't an image + $this->assertNull($manipulated->getImageBackend()); + // Double check the variant was created and stored correctly + $this->assertSame([AbstractFileIDHelper::EXTENSION_REWRITE_VARIANT, 'txt', 'csv'], $manipulated->variantParts($manipulated->getVariant())); + $this->assertTrue($store->exists($manipulated->getFilename(), $manipulated->getHash(), $manipulated->getVariant())); + $this->assertSame('Any content will do - csv is just a text file afterall', $manipulated->getString()); + } } diff --git a/tests/php/ImageTest.yml b/tests/php/ImageTest.yml index 33c0895d..55966ec8 100644 --- a/tests/php/ImageTest.yml +++ b/tests/php/ImageTest.yml @@ -42,3 +42,11 @@ SilverStripe\Assets\Image: FileHash: 1b22f41d0d27755f06b77eaa27e074eff84d3019 Parent: =>SilverStripe\Assets\Folder.folder1 Name: landscape-to-portrait.jpg + +SilverStripe\Assets\File: + notImage: + Title: This is not an image + FileFilename: folder/not-image.txt + FileHash: 6ab0df7d967f44e98d4bfa403020c6921a2b46e7 + Parent: =>SilverStripe\Assets\Folder.folder1 + Name: not-image.txt