Migrate legacy thumbnails (fixes #235)

[chillu]

Merge after silverstripe/silverstripe-framework#8924

Relies on @max #223 to be merged first. The newest commits by me are the actual PR here

[maxime-rainville]

Looks generally good. I'm thinking of adding some sort of "normalise path" method to asset store to help with the file migration task. That might have save you some trouble.

There's a couple edge cases that I've highlighted and some possible improvements.

I'm thinking maybe add a test case for two image files in the same folder with variants sharing the same _resampled sub folder.

Maybe add an assertion that the variants don't get regenerated when the developer asks for them later on. That's more of a nice to have.

[maxime-rainville]

Travis failure is caused by the main file migration task which hasn't been converted to use the strategies yet.

[tractorcow]

That's a lot of code!

Is there something in particular you want me to review? I can't review all of this but I can look at specifics if you need input.

[chillu]

@tractorcow Awesome, thanks for the offer! Maybe just think about gnarly edge cases that we might've missed? Stuff like "chained format thumbnail file names", "multiple files in same _resampled folder".

I'm thinking maybe add a test case for two image files in the same folder with variants sharing the same _resampled sub folder.

Hah, funny I actually had that case in there (nested-sibling.jpg), but the fixture handling got too complicated so I trimmed it back down. Will have another look

[chillu]

@tractorcow Oh I just realised you might not have picked up on the fact that most commits in this PR are pulled in from a separate PR: #223. That other PR is actually more important to get your input on (it refactors a lot of the asset abstraction behaviour), the thumbnail task here is just another cleanup thing that's less important.

[chillu]

Maybe add an assertion that the variants don't get regenerated when the developer asks for them later on. That's more of a nice to have.

Yeah, I don't see how that would happen - the locations of generated thumbnails should be asserted as part of the existing image manipulation tests, has nothing to do with the migration task, right? Anyway, I've added ImageTest->testGenerateImageInSameFolderAsOriginal()

Ready for re-review :)

[maxime-rainville]

I've merge 1.4 back into this.

[maxime-rainville]

I've confirmed that this thing works correctly.

I've used a script to generate dummy SS3 variants and confirmed the files get moved around after running the thumbnail helper.

The tasks needs to be moved to the SilverStripe\Assets\Dev\Tasks namespace to be inline with the other ones.

I'll get that done tomorrow morning.

[maxime-rainville]

Needs to be updated to SilverStripe\Assets\Dev\Tasks

[maxime-rainville]

Fix namespace.

[maxime-rainville]

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.

[maxime-rainville]

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

[chillu]

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)

[chillu]

I've decided to remove a test around thumbnails on protected files, since it's hitting diminishing returns: Probably spent a day on getting all the stars aligned in the various abstractions of abstractions in there. The test currently fails, which correctly points out that this feature isn't implemented. I've documented it as a known limitation instead. It's an edge case.

Does not move legacy thumbnails to the protected store if the original file has been unpublished or protected since an earlier 4.x migration run

/**
     * 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();
        $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,
        // 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);

        $this->assertCount(1, $moved);

        $publicVariants = $store->getPublicResolutionStrategy()
            ->findVariants(
                new ParsedFileID($image->Filename, $image->Hash),
                $store->getPublicFilesystem()
            );
        $this->assertCount(0, $publicVariants);

        $protectedVariants = $store->getProtectedResolutionStrategy()
            ->findVariants(
                new ParsedFileID($image->Filename, $image->Hash),
                $store->getProtectedFilesystem()
            );

        // findVariants() returns *both* the original file and the variant
        // See FileIDHelperResolutionStrategyTest->testFindVariant()
        // TODO This is counting the original file twice, doesn't include the actual variant
        // See https://github.com/silverstripe/silverstripe-assets/pull/242/files#r279615136
        $this->assertCount(1, $protectedVariants);
    }

[maxime-rainville]

I've fixed some linting error to please the PHPCS Gods.

Merge on green.