Skip to content

Commit

Permalink
FIX Ensure manipulations start with base file (#612)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
GuySartorelli authored Jun 4, 2024
1 parent db48540 commit c4081b9
Show file tree
Hide file tree
Showing 7 changed files with 35 additions and 13 deletions.
3 changes: 2 additions & 1 deletion src/Conversion/FileConverter.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace SilverStripe\Assets\Conversion;

use SilverStripe\Assets\File;
use SilverStripe\Assets\Storage\DBFile;

/**
Expand All @@ -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;
}
3 changes: 2 additions & 1 deletion src/Conversion/FileConverterManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand Down
3 changes: 2 additions & 1 deletion src/Conversion/InterventionImageFileConverter.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
7 changes: 1 addition & 6 deletions src/ImageManipulation.php
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
24 changes: 24 additions & 0 deletions tests/php/ImageManipulationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}
}

0 comments on commit c4081b9

Please sign in to comment.