Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate legacy thumbnails (fixes #235) #242

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
<?php

namespace SilverStripe\Assets;
namespace SilverStripe\Assets\Dev\Tasks;

use Psr\Log\LoggerInterface;
use Psr\Log\NullLogger;
use SilverStripe\Assets\FilenameParsing\LegacyFileIDHelper;
use SilverStripe\Assets\File;
use SilverStripe\Assets\Flysystem\FlysystemAssetStore;
use SilverStripe\Assets\Folder;
use SilverStripe\Core\Config\Configurable;
use SilverStripe\Core\Environment;
use SilverStripe\Core\Injector\Injectable;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
<?php

namespace SilverStripe\Assets\Tests;
namespace SilverStripe\Assets\Tests\Dev\Tasks;

use Silverstripe\Assets\Dev\TestAssetStore;
use SilverStripe\Assets\File;
use SilverStripe\Assets\FileMigrationHelper;
use SilverStripe\Assets\FilenameParsing\FileIDHelperResolutionStrategy;
use SilverStripe\Assets\FilenameParsing\LegacyFileIDHelper;
use SilverStripe\Assets\FilenameParsing\ParsedFileID;
use SilverStripe\Assets\Filesystem;
use SilverStripe\Assets\Flysystem\FlysystemAssetStore;
use SilverStripe\Assets\Folder;
use SilverStripe\Assets\Image;
use SilverStripe\Assets\LegacyThumbnailMigrationHelper;
use SilverStripe\Assets\Dev\Tasks\LegacyThumbnailMigrationHelper;
use SilverStripe\Assets\Storage\AssetStore;
use SilverStripe\Assets\Tests\FileMigrationHelperTest\Extension;
use SilverStripe\Core\Config\Config;
use SilverStripe\Assets\Tests\Dev\Tasks\FileMigrationHelperTest\Extension;
use SilverStripe\Core\Convert;
use SilverStripe\Dev\SapphireTest;

Expand Down Expand Up @@ -49,7 +48,7 @@ public function setUp()
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');
$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());
Expand All @@ -68,21 +67,48 @@ public function tearDown()
parent::tearDown();
}

/**
* Tests cases where files have already been migrated to 4.3 or older,
* and some images have since been unpublished or otherwise marked protected.
* The previously orphaned thumbnails wouldn't have been moved to the protected store in this case.
* The migration task should find those orphans, and reunite them with the original image.
* And they lived happily ever after.
*/
public function testMigratesThumbnailsForProtectedFiles()
{
/** @var TestAssetStore $store */
$store = singleton(AssetStore::class); // will use TestAssetStore

// Remove the legacy helper, otherwise it'll move the _resampled files as well,
// which "pollutes" our test case.
/** @var FileIDHelperResolutionStrategy $strategy */
$publicStrategy = $store->getPublicResolutionStrategy();
maxime-rainville marked this conversation as resolved.
Show resolved Hide resolved
$helpers = $publicStrategy->getResolutionFileIDHelpers();
$origHelpers = [];
foreach ($helpers as $i => $helper) {
$origHelpers[] = clone $helper;
if ($helper instanceof LegacyFileIDHelper) {
unset($helpers[$i]);
}
}
$publicStrategy->setResolutionFileIDHelpers($helpers);
$store->setPublicResolutionStrategy($publicStrategy);

/** @var Image $image */
$image = $this->objFromFixture(Image::class, 'nested');

$formats = ['ResizedImage' => [60,60]];
$expectedLegacyPath = $this->createLegacyResampledImageFixture($store, $image, $formats);

// Protect file *after* creating legacy fixture
// Protect file *after* creating legacy fixture,
// but without moving the _resampled "orphan"
$image->protectFile();
$image->write();

// We need to retain the legacy helper for later assertions.
$publicStrategy->setResolutionFileIDHelpers($origHelpers);
$store->setPublicResolutionStrategy($publicStrategy);

$helper = new LegacyThumbnailMigrationHelper();
$moved = $helper->run($store);

Expand All @@ -100,7 +126,10 @@ public function testMigratesThumbnailsForProtectedFiles()
new ParsedFileID($image->Filename, $image->Hash),
$store->getProtectedFilesystem()
);
$this->assertCount(1, $protectedVariants);

// findVariants() returns *both* the original file and the variant
// See FileIDHelperResolutionStrategyTest->testFindVariant()
$this->assertCount(2, $protectedVariants);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findVariants returns a generator. Technically, those are not countable. PHPUnit seems to get around that by iterating over the generator until it reaches the end or goes over the limit. That does meant that when the test fails, you always get a "Failed asserting that actual size 0 matches expected size 2." even if the actual size is something other than zero.

I'm not going any where with this ... just pointing it out. I was half way going down the rabbit hole when I realised that it wasn't a problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it is broken.

The moved variant is on the public store. You are looking on the protected store, so you are not getting the variant at all.

You are getting the main file twice:

  • Once because it matches the NaturalFileIDHelper
  • Once because it matches the LegacyFileIDHelper

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaaah great catch! This is a super gnarly test. I've removed it with fa4b14c, and added it as a known limitation. See comment on main thread for more rationale: #242 (comment)

}

public function testMigratesWithExistingThumbnailInNewLocation()
Expand Down