diff --git a/src/Dev/Tasks/LegacyThumbnailMigrationHelper.php b/src/Dev/Tasks/LegacyThumbnailMigrationHelper.php new file mode 100644 index 00000000..00c329d9 --- /dev/null +++ b/src/Dev/Tasks/LegacyThumbnailMigrationHelper.php @@ -0,0 +1,199 @@ + '%$' . LoggerInterface::class, + ]; + + /** @var LoggerInterface */ + private $logger; + + public function __construct() + { + $this->logger = new NullLogger(); + } + + /** + * Perform migration + * + * @param FlysystemAssetStore $store + * @return array Map of old to new moved paths + */ + public function run(FlysystemAssetStore $store) + { + // Set max time and memory limit + Environment::increaseTimeLimitTo(); + Environment::increaseMemoryLimitTo(); + + // Loop over all folders + $allMoved = []; + $originalState = null; + if (class_exists(Versioned::class)) { + $originalState = Versioned::get_reading_mode(); + Versioned::set_stage(Versioned::DRAFT); + } + + // Migrate root folder (not returned from query) + $moved = $this->migrateFolder($store, new Folder()); + if ($moved) { + $allMoved = array_merge($allMoved, $moved); + } + + // Migrate all nested folders + $folders = $this->getFolderQuery(); + foreach ($folders->dataQuery()->execute() as $folderData) { + // More memory efficient way to looping through large sets + /** @var Folder $folder */ + $folder = $folders->createDataObject($folderData); + $moved = $this->migrateFolder($store, $folder); + if ($moved) { + $allMoved = array_merge($allMoved, $moved); + } + } + if (class_exists(Versioned::class)) { + Versioned::set_reading_mode($originalState); + } + return $allMoved; + } + + /** + * @param LoggerInterface $logger + */ + public function setLogger(LoggerInterface $logger) + { + $this->logger = $logger; + + return $this; + } + + /** + * Migrate a folder + * + * @param FlysystemAssetStore $store + * @param Folder $folder + * @param string $legacyFilename + * @return array Map of old to new file paths (relative to store root) + */ + protected function migrateFolder(FlysystemAssetStore $store, Folder $folder) + { + $moved = []; + + // Get normalised store relative path + $folderPath = preg_replace('#^/#', '', $folder->getFilename()); + $resampledFolderPath = $folderPath . '_resampled'; + + // Legacy thumbnails couldn't have been stored in a protected filesystem + /** @var \League\Flysystem\Filesystem $filesystem */ + $filesystem = $store->getPublicFilesystem(); + + if (!$filesystem->has($resampledFolderPath)) { + return $moved; + } + + // Recurse through folder + foreach ($filesystem->listContents($resampledFolderPath, true) as $fileInfo) { + if ($fileInfo['type'] !== 'file') { + continue; + } + + $oldResampledPath = $fileInfo['path']; + + // Get "variant folders" inside _resampled folder. + $variantFolders = preg_replace('#.*_resampled/(.*)#', '$1', $fileInfo['dirname']); + $encodedVariants = explode(DIRECTORY_SEPARATOR, $variantFolders); + + // Replicate new variant format. + // We're always placing these files in the public filesystem, *without* a content hash path. + // This means you need to run the optional migration task introduced in 4.4, + // which moves public files out of content hash folders. + // Image->manipulate() is in charge of creating the full file name (incl. variant), + // and assumes the manipulation is run on an existing original file, so we can't use it here. + // Any AssetStore-level filesystem operations (like move()) suffer from the same limitation, + // so we need to drop down to path based filesystem renames. + $newResampledPath = implode('', [ + $folderPath, + $fileInfo['filename'], + '__', + implode('_', $encodedVariants), + '.' . $fileInfo['extension'] + ]); + + // Don't overwrite existing files in the new location, + // they might have been generated based on newer file contents + if ($filesystem->has($newResampledPath)) { + $filesystem->delete($oldResampledPath); + continue; + } + + $filesystem->rename($oldResampledPath, $newResampledPath); + + $this->logger->info(sprintf('Moved legacy thumbnail %s to %s', $oldResampledPath, $newResampledPath)); + + $moved[$oldResampledPath] = $newResampledPath; + } + + // Remove folder and any subfolders. + // Assumes all files have been handled in the loop above, + // and either deleted (if new location already exists), + // or moved out of the folder (if new location didn't exist). + $filesystem->deleteDir($resampledFolderPath); + + return $moved; + } + + /** + * Get list of Folder dataobjects to inspect for + * + * @return \SilverStripe\ORM\DataList + */ + protected function getFolderQuery() + { + $table = DataObject::singleton(File::class)->baseTable(); + // Select all records which have a Filename value, but not FileFilename. + /** @skipUpgrade */ + return File::get() + ->filter('ClassName', [Folder::class, 'Folder']) + ->filter('FileFilename', array('', null)) + ->alterDataQuery(function (DataQuery $query) use ($table) { + return $query->addSelectFromTable($table, ['Filename']); + }); + } +} diff --git a/src/Dev/TestAssetStore.php b/src/Dev/TestAssetStore.php index 80512cda..5817716a 100644 --- a/src/Dev/TestAssetStore.php +++ b/src/Dev/TestAssetStore.php @@ -132,10 +132,12 @@ public static function reset() /** * Helper method to get local filesystem path for this file * - * @param AssetContainer $asset + * @param AssetContainer $asset + * @param boolean $forceProtected + * @param boolean $relative Return path relative to asset store root. * @return string */ - public static function getLocalPath(AssetContainer $asset, $forceProtected = false) + public static function getLocalPath(AssetContainer $asset, $forceProtected = false, $relative = false) { if ($asset instanceof Folder) { return self::base_path() . '/' . $asset->getFilename(); @@ -154,7 +156,7 @@ public static function getLocalPath(AssetContainer $asset, $forceProtected = fal } /** @var Local $adapter */ $adapter = $filesystem->getAdapter(); - return $adapter->applyPathPrefix($fileID); + return $relative ? $fileID : $adapter->applyPathPrefix($fileID); } public function cleanFilename($filename) diff --git a/src/File.php b/src/File.php index 8dc73e4f..415f0a8c 100644 --- a/src/File.php +++ b/src/File.php @@ -255,14 +255,6 @@ class File extends DataObject implements AssetContainer, Thumbnail, CMSPreviewab */ private static $apply_restrictions_to_admin = true; - /** - * If enabled, legacy file dataobjects will be automatically imported into the APL - * - * @config - * @var bool - */ - private static $migrate_legacy_file = false; - /** * @config * @var boolean @@ -1206,21 +1198,6 @@ public function getTag() return (string)$this->renderWith($template); } - public function requireDefaultRecords() - { - parent::requireDefaultRecords(); - - // Check if old file records should be migrated - if (!$this->config()->get('migrate_legacy_file')) { - return; - } - - $migrated = FileMigrationHelper::singleton()->run(); - if ($migrated) { - DB::alteration_message("{$migrated} File DataObjects upgraded", "changed"); - } - } - /** * Get the back-link tracking objects that link to this file via HTML fields * diff --git a/src/ImageManipulation.php b/src/ImageManipulation.php index b3d0e5ed..9778c69b 100644 --- a/src/ImageManipulation.php +++ b/src/ImageManipulation.php @@ -990,7 +990,6 @@ public function manipulate($variant, $callback) * @param string $format The format name. * @param mixed $arg,... Additional arguments * @return string - * @throws InvalidArgumentException */ public function variantName($format, $arg = null) { @@ -999,6 +998,38 @@ public function variantName($format, $arg = null) return $format . Convert::base64url_encode($args); } + /** + * Reverses {@link variantName()}. + * The "format" part of a variant name is a method name on the owner of this trait. + * 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. + * This class is commonly decorated with additional manipulation methods through {@link DataExtension}. + * + * @param $variantName + * @return array|null An array of arguments passed to {@link variantName}. The first item is the "format". + * @throws InvalidArgumentException + */ + public function variantParts($variantName) + { + $methods = array_map('preg_quote', singleton(Image::class)->allMethodNames()); + + // Regex needs to be case insensitive since allMethodNames() is all lowercased + $regex = '#^(?(' . implode('|', $methods) . '))(?(.*))#i'; + preg_match($regex, $variantName, $matches); + + if (!$matches) { + throw new InvalidArgumentException('Invalid variant name: ' . $variantName); + } + + $args = Convert::base64url_decode($matches['encodedargs']); + if (!$args) { + throw new InvalidArgumentException('Invalid variant name arguments: ' . $variantName); + } + + return array_merge([$matches['format']], $args[0]); + } + /** * Validate a width or size is valid and casts it to integer * diff --git a/tests/php/Dev/Tasks/FileMigrationHelperTest.php b/tests/php/Dev/Tasks/FileMigrationHelperTest.php index 4875de84..c5086319 100644 --- a/tests/php/Dev/Tasks/FileMigrationHelperTest.php +++ b/tests/php/Dev/Tasks/FileMigrationHelperTest.php @@ -11,6 +11,7 @@ use SilverStripe\Assets\Image; use SilverStripe\Assets\Tests\Dev\Tasks\FileMigrationHelperTest\Extension; use SilverStripe\Core\Config\Config; +use SilverStripe\Core\Convert; use SilverStripe\Dev\SapphireTest; /** @@ -42,8 +43,6 @@ protected function getBasePath() public function setUp() { - Config::nest(); // additional nesting here necessary - Config::modify()->merge(File::class, 'migrate_legacy_file', false); parent::setUp(); // Set backend root to /FileMigrationHelperTest/assets @@ -76,7 +75,6 @@ public function tearDown() TestAssetStore::reset(); Filesystem::removeFolder($this->getBasePath()); parent::tearDown(); - Config::unnest(); } /** diff --git a/tests/php/Dev/Tasks/LegacyThumbnailMigrationHelperTest.php b/tests/php/Dev/Tasks/LegacyThumbnailMigrationHelperTest.php new file mode 100644 index 00000000..e3d0195f --- /dev/null +++ b/tests/php/Dev/Tasks/LegacyThumbnailMigrationHelperTest.php @@ -0,0 +1,311 @@ + array( + Extension::class, + ) + ); + + /** + * get the BASE_PATH for this test + * + * @return string + */ + protected function getBasePath() + { + // Note that the actual filesystem base is the 'assets' subdirectory within this + return $this->joinPaths(ASSETS_PATH, '/LegacyThumbnailMigrationHelperTest'); + } + + + public function setUp() + { + parent::setUp(); + + // Set backend root to /LegacyThumbnailMigrationHelperTest/assets + TestAssetStore::activate('LegacyThumbnailMigrationHelperTest/assets'); + + // Ensure that each file has a local record file in this new assets base + $from = $this->joinPaths(__DIR__, '../../ImageTest/test-image-low-quality.jpg'); + foreach (File::get()->exclude('ClassName', Folder::class) as $file) { + /** @var $file File */ + $file->setFromLocalFile($from, $file->generateFilename()); + $file->write(); + $file->publishFile(); + $dest = $this->joinPaths(TestAssetStore::base_path(), $file->generateFilename()); + Filesystem::makeFolder(dirname($dest)); + copy($from, $dest); + } + } + + public function tearDown() + { + TestAssetStore::reset(); + Filesystem::removeFolder($this->getBasePath()); + parent::tearDown(); + } + + public function testMigratesWithExistingThumbnailInNewLocation() + { + /** @var TestAssetStore $store */ + $store = singleton(AssetStore::class); // will use TestAssetStore + + /** @var Image $image */ + $image = $this->objFromFixture(Image::class, 'nested'); + + $formats = ['ResizedImage' => [60,60]]; + $expectedNewPath = $this->getNewResampledPath($image, $formats, $keep = true); + $expectedLegacyPath = $this->createLegacyResampledImageFixture($store, $image, $formats); + + $helper = new LegacyThumbnailMigrationHelper(); + $moved = $helper->run($store); + $this->assertCount(0, $moved); + + // Moved contains store relative paths + $base = TestAssetStore::base_path(); + + $this->assertFileNotExists( + $this->joinPaths($base, $expectedLegacyPath), + 'Legacy file has been removed' + ); + $this->assertFileExists( + $this->joinPaths($base, $expectedNewPath), + 'New file is retained (potentially with newer content)' + ); + } + + public function testMigratesMultipleFilesInSameFolder() + { + /** @var TestAssetStore $store */ + $store = singleton(AssetStore::class); // will use TestAssetStore + + /** @var Image[] $image */ + $images = [ + $this->objFromFixture(Image::class, 'nested'), + $this->objFromFixture(Image::class, 'nested-sibling') + ]; + + // Use same format for *both* files (edge cases!) + $expected = []; + $formats = ['ResizedImage' => [60,60]]; + foreach ($images as $image) { + $expectedNewPath = $this->getNewResampledPath($image, $formats); + $expectedLegacyPath = $this->createLegacyResampledImageFixture($store, $image, $formats); + $expected[$expectedLegacyPath] = $expectedNewPath; + } + + $helper = new LegacyThumbnailMigrationHelper(); + $moved = $helper->run($store); + $this->assertCount(2, $moved); + + // Moved contains store relative paths + $base = TestAssetStore::base_path(); + + foreach ($expected as $expectedLegacyPath => $expectedNewPath) { + $this->assertFileNotExists( + $this->joinPaths($base, $expectedLegacyPath), + 'Legacy file has been removed' + ); + $this->assertFileExists( + $this->joinPaths($base, $expectedNewPath), + 'New file is retained (potentially with newer content)' + ); + } + } + + + /** + * @dataProvider dataProvider + */ + public function testMigrate($fixtureId, $formats) + { + /** @var TestAssetStore $store */ + $store = singleton(AssetStore::class); // will use TestAssetStore + + /** @var Image $image */ + $image = $this->objFromFixture(Image::class, $fixtureId); + + // Simulate where the new thumbnail would be created by the system. + // Important to do this *before* creating the legacy file, + // because the LegacyFileIDHelper will pick it up as the "new" location otherwise + $expectedNewPath = $this->getNewResampledPath($image, $formats); + $expectedLegacyPath = $this->createLegacyResampledImageFixture($store, $image, $formats); + + $helper = new LegacyThumbnailMigrationHelper(); + $moved = $helper->run($store); + $this->assertCount(1, $moved); + + // Moved contains store relative paths + $base = TestAssetStore::base_path(); + + $this->assertArrayHasKey( + $expectedLegacyPath, + $moved + ); + $this->assertEquals( + $moved[$expectedLegacyPath], + $expectedNewPath, + 'New file is mapped as expected' + ); + $this->assertFileNotExists( + $this->joinPaths($base, $expectedLegacyPath), + 'Legacy file has been removed' + ); + $this->assertFileExists( + $this->joinPaths($base, $expectedNewPath), + 'New file has been created' + ); + $origFolder = $image->Parent()->getFilename(); + $this->assertEquals( + $origFolder ? $origFolder : '.' . DIRECTORY_SEPARATOR, + dirname($expectedNewPath) . DIRECTORY_SEPARATOR, + 'Thumbnails are created in same folder as original file' + ); + } + + public function dataProvider() + { + return [ + 'Single variant toplevel' => [ + 'toplevel', + ['ResizedImage' => [60,60]] + ], + 'Multi variant toplevel' => [ + 'toplevel', + ['ResizedImage' => [60,60], 'CropHeight' => [30]] + ], + 'Multi variant nested' => [ + 'nested', + ['ResizedImage' => [60,60], 'CropHeight' => [30]] + ] + ]; + } + + /** + * @param Image $baseImage + * @param array + * @return string Path relative to the asset store root. + */ + protected function createLegacyResampledImageFixture(AssetStore $store, Image $baseImage, $formats) + { + $resampledRelativePath = $this->legacyCacheFilename($baseImage, $formats); + + // Using raw copy operation since File->copyFile() messes with the _resampled path nane, + // and anything on asset abstraction unhelpfully copies + // existing (new style) variants as well (creating false positives) + $origPath = $this->joinPaths( + TestAssetStore::base_path(), + $baseImage->generateFilename() + ); + $resampledPath = $this->joinPaths( + TestAssetStore::base_path(), + $resampledRelativePath + ); + Filesystem::makeFolder(dirname($resampledPath)); + copy($origPath, $resampledPath); + + return $resampledRelativePath; + } + + /** + * Replicates the logic of creating 3.x style formatted images, + * based on Image->cacheFilename() and Image->generateFormattedImage(). + * Will create folder structures required for this. + * + * @return string Path relative to the asset store root. + */ + protected function legacyCacheFilename($image, $formats) + { + $cacheFilename = ''; + + if ($image->Parent()->exists()) { + $cacheFilename = $image->Parent()->Filename; + } + + $cacheFilename = $this->joinPaths($cacheFilename, '_resampled'); + + foreach ($formats as $format => $args) { + $cacheFilename = $this->joinPaths( + $cacheFilename, + $format . Convert::base64url_encode($args) + ); + } + + $cacheFilename = $this->joinPaths( + $cacheFilename, + basename($image->generateFilename()) + ); + + return $cacheFilename; + } + + /** + * Create a file variant to get its path, but then remove it. + * We want to check that it's moved into the same location + * through the migration task further along the test. + * + * @param Image $image + * @param array $formats + * @return String Path relative to the asset store root + */ + protected function getNewResampledPath(Image $image, $formats, $keep = false) + { + $resampleds = []; + + /** @var Image $newResampledImage */ + $resampled = $image; + + // Perform the manipulation (only to get the resulting path) + foreach ($formats as $format => $args) { + $resampled = call_user_func_array([$resampled, $format], $args); + $resampleds [] = $resampled; + } + + $path = TestAssetStore::getLocalPath($resampled, false, true); // relative to store + $path = preg_replace('#^/#', '', $path); // normalise with other store relative paths + + // Not using File->delete() since that actually deletes the original file, not only variant. + if (!$keep) { + foreach ($resampleds as $resampled) { + unlink(TestAssetStore::getLocalPath($resampled)); + } + } + + return $path; + } + + /** + * @return string + */ + protected function joinPaths() + { + $paths = array(); + + foreach (func_get_args() as $arg) { + if ($arg !== '') { + $paths[] = $arg; + } + } + + return preg_replace('#/+#', '/', join('/', $paths)); + } +} diff --git a/tests/php/Dev/Tasks/LegacyThumbnailMigrationHelperTest.yml b/tests/php/Dev/Tasks/LegacyThumbnailMigrationHelperTest.yml new file mode 100644 index 00000000..eb9b1dc7 --- /dev/null +++ b/tests/php/Dev/Tasks/LegacyThumbnailMigrationHelperTest.yml @@ -0,0 +1,19 @@ +SilverStripe\Assets\Folder: + parent: + Name: ParentFolder + subfolder: + Name: SubFolder + Parent: =>SilverStripe\Assets\Folder.parent +SilverStripe\Assets\Image: + toplevel: + Name: toplevel.jpg + nested: + Name: nested.jpg + ParentID: =>SilverStripe\Assets\Folder.subfolder + nested-sibling: + Name: nested-sibling.jpg + ParentID: =>SilverStripe\Assets\Folder.subfolder +SilverStripe\Assets\File: + file1: + Name: anotherfile.jpg + diff --git a/tests/php/ImageTest.php b/tests/php/ImageTest.php index 0f0fcd02..fa94757f 100644 --- a/tests/php/ImageTest.php +++ b/tests/php/ImageTest.php @@ -349,6 +349,14 @@ public function testCacheFilename() $this->assertContains($neededPart, $imageFilename, 'Filename for cached image is correctly generated'); } + public function testGenerateImageInSameFolderAsOriginal() + { + // All fixtures are in a subfolder + $original = $this->objFromFixture(Image::class, 'imageWithoutTitle'); + $generated = $original->Pad(200, 200, 'CCCCCC', 0); + $this->assertEquals($original->Parent()->getFilename(), $generated->Parent()->getFilename()); + } + /** * Ensure dimensions are cached */ @@ -381,6 +389,31 @@ public function testCacheDimensions() $this->assertTrue($cache->has($customKey)); } + public function testVariantParts() + { + /** @var Image $image */ + $image = singleton(Image::class); + $format = 'Pad'; + $args = [331, 313, '222222', 0]; + $name = $image->variantName($format, $args); + $this->assertEquals( + array_merge([$format], $args), + $image->variantParts($name) + ); + } + + /** + * @expectedException \InvalidArgumentException + */ + public function testVariantPartsThrowsOnInvalidName() + { + /** @var Image $image */ + $image = singleton(Image::class); + $args = ['foo']; + $name = $image->variantName('Invalid', $args); + $image->variantParts($name); + } + protected function getDimensionCacheKey($hash, $variant) { return InterventionBackend::CACHE_DIMENSIONS . sha1($hash .'-'.$variant);