From c4081b956807b197d1a7df4eb0fa4b112121e9e2 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Wed, 5 Jun 2024 11:17:12 +1200 Subject: [PATCH] FIX Ensure manipulations start with base file (#612) It turns out there's some special logic going on in manipulation that needs to take place starting with the File record. If you try to use the DBFile on its own without first performin a manipulation, the attributes applied to the file won't be passed down to subsequent manipulations. --- src/Conversion/FileConverter.php | 3 ++- src/Conversion/FileConverterManager.php | 3 ++- .../InterventionImageFileConverter.php | 3 ++- src/ImageManipulation.php | 7 +----- .../TestImageConverter.php | 4 ++-- .../TestTxtToImageConverter.php | 4 ++-- tests/php/ImageManipulationTest.php | 24 +++++++++++++++++++ 7 files changed, 35 insertions(+), 13 deletions(-) diff --git a/src/Conversion/FileConverter.php b/src/Conversion/FileConverter.php index 54d3c025..f5ef347b 100644 --- a/src/Conversion/FileConverter.php +++ b/src/Conversion/FileConverter.php @@ -2,6 +2,7 @@ namespace SilverStripe\Assets\Conversion; +use SilverStripe\Assets\File; use SilverStripe\Assets\Storage\DBFile; /** @@ -27,5 +28,5 @@ public function supportsConversion(string $fromExtension, string $toExtension, a * @param array $options Any options defined for this converter which should apply to the conversion. * @throws FileConverterException if invalid options are passed, or the conversion is not supported or fails. */ - public function convert(DBFile $from, string $toExtension, array $options = []): DBFile; + public function convert(DBFile|File $from, string $toExtension, array $options = []): DBFile; } diff --git a/src/Conversion/FileConverterManager.php b/src/Conversion/FileConverterManager.php index 504d26c6..92236a99 100644 --- a/src/Conversion/FileConverterManager.php +++ b/src/Conversion/FileConverterManager.php @@ -2,6 +2,7 @@ namespace SilverStripe\Assets\Conversion; +use SilverStripe\Assets\File; use SilverStripe\Assets\Storage\DBFile; use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\Injector\Injector; @@ -27,7 +28,7 @@ class FileConverterManager * Note that if a converter supports the conversion generally but doesn't support these options, that converter will not be used. * @throws FileConverterException if the conversion failed or there were no converters available. */ - public function convert(DBFile $from, string $toExtension, array $options = []): DBFile + public function convert(DBFile|File $from, string $toExtension, array $options = []): DBFile { $fromExtension = $from->getExtension(); foreach (static::config()->get('converters') as $converterClass) { diff --git a/src/Conversion/InterventionImageFileConverter.php b/src/Conversion/InterventionImageFileConverter.php index 7c76b22d..7f75a0a1 100644 --- a/src/Conversion/InterventionImageFileConverter.php +++ b/src/Conversion/InterventionImageFileConverter.php @@ -4,6 +4,7 @@ use Imagick; use Intervention\Image\Exception\ImageException; +use SilverStripe\Assets\File; use SilverStripe\Assets\Image_Backend; use SilverStripe\Assets\InterventionBackend; use SilverStripe\Assets\Storage\AssetStore; @@ -33,7 +34,7 @@ public function supportsConversion(string $fromExtension, string $toExtension, a return $this->supportedByIntervention($fromExtension, $backend) && $this->supportedByIntervention($toExtension, $backend); } - public function convert(DBFile $from, string $toExtension, array $options = []): DBFile + public function convert(DBFile|File $from, string $toExtension, array $options = []): DBFile { // Do some basic validation up front for things we know aren't supported $problems = $this->validateOptions($options); diff --git a/src/ImageManipulation.php b/src/ImageManipulation.php index 7b849070..ed1d1e09 100644 --- a/src/ImageManipulation.php +++ b/src/ImageManipulation.php @@ -721,13 +721,8 @@ public function ThumbnailURL($width, $height) public function Convert(string $toExtension): ?AssetContainer { $converter = Injector::inst()->get(FileConverterManager::class); - if ($this instanceof File) { - $from = $this->File; - } elseif ($this instanceof DBFile) { - $from = $this; - } try { - return $converter->convert($from, $toExtension); + return $converter->convert($this, $toExtension); } catch (FileConverterException $e) { /** @var LoggerInterface $logger */ $logger = Injector::inst()->get(LoggerInterface::class . '.errorhandler'); diff --git a/tests/php/Conversion/FileConversionManagerTest/TestImageConverter.php b/tests/php/Conversion/FileConversionManagerTest/TestImageConverter.php index 4a505e17..939cf962 100644 --- a/tests/php/Conversion/FileConversionManagerTest/TestImageConverter.php +++ b/tests/php/Conversion/FileConversionManagerTest/TestImageConverter.php @@ -15,9 +15,9 @@ public function supportsConversion(string $fromExtension, string $toExtension, a return in_array($fromExtension, $formats) && in_array($toExtension, $formats); } - public function convert(DBFile $from, string $toExtension, array $options = []): DBFile + public function convert(DBFile|File $from, string $toExtension, array $options = []): DBFile { - $result = clone $from; + $result = ($from instanceof File) ? clone $from->File : clone $from; $result->Filename = 'converted.' . $toExtension; return $result; } diff --git a/tests/php/Conversion/FileConversionManagerTest/TestTxtToImageConverter.php b/tests/php/Conversion/FileConversionManagerTest/TestTxtToImageConverter.php index 9a17993f..1ad729fe 100644 --- a/tests/php/Conversion/FileConversionManagerTest/TestTxtToImageConverter.php +++ b/tests/php/Conversion/FileConversionManagerTest/TestTxtToImageConverter.php @@ -15,9 +15,9 @@ public function supportsConversion(string $fromExtension, string $toExtension, a return strtolower($fromExtension) === 'txt' && in_array(strtolower($toExtension), $formats); } - public function convert(DBFile $from, string $toExtension, array $options = []): DBFile + public function convert(DBFile|File $from, string $toExtension, array $options = []): DBFile { - $result = clone $from; + $result = ($from instanceof File) ? clone $from->File : clone $from; $result->Filename = 'converted.' . $toExtension; return $result; } diff --git a/tests/php/ImageManipulationTest.php b/tests/php/ImageManipulationTest.php index 0f966f35..c872f113 100644 --- a/tests/php/ImageManipulationTest.php +++ b/tests/php/ImageManipulationTest.php @@ -7,6 +7,7 @@ use Psr\Log\LoggerInterface; use SilverStripe\Assets\Conversion\FileConverterException; use SilverStripe\Assets\Conversion\FileConverterManager; +use SilverStripe\Assets\Conversion\InterventionImageFileConverter; use Silverstripe\Assets\Dev\TestAssetStore; use SilverStripe\Assets\File; use SilverStripe\Assets\FilenameParsing\AbstractFileIDHelper; @@ -604,4 +605,27 @@ public function testConvert(string $originalFileFixtureClass, string $originalFi $this->assertNull($result); } } + + public function provideConvertChainWithLazyLoad(): array + { + return [ + [true], + [false], + ]; + } + + /** + * @dataProvider provideConvertChainWithLazyLoad + */ + public function testConvertChainWithLazyLoad(bool $lazyLoad): void + { + // Make sure we have a known set of converters for testing + FileConverterManager::config()->set('converters', [InterventionImageFileConverter::class]); + $file = $this->objFromFixture(Image::class, 'imageWithTitle'); + /** @var DBFile */ + $result = $file->LazyLoad($lazyLoad)->Convert('webp'); + $this->assertSame($lazyLoad, $result->IsLazyLoaded()); + $result = $file->Convert('webp')->LazyLoad($lazyLoad); + $this->assertSame($lazyLoad, $result->IsLazyLoaded()); + } }