DEP Upgrade legue/flysystem to version 3.0

[sabina-talipova]

Issue

#497

Description

• Upgrade Flysystem to version 3.0
• Upgrade existing functionality in Asset classes by following provided documentation:
  • Upgrade from 1.x;
  • What is new in Flysystem V2 & V3
• New class ExtendedFilesystem extends Filesystem to provide getAdapter() method, that was removed from main legue/flysystem Filesystem class (GitHub discussion)
• New class ExtendedLocalFilesystemAdapter extends LocalFilesystemAdapter to provide prefixPath method that replaces applyPathPrefix method from removed abstract class AbstractAdapter (GitHub discussion)
• New class SilverStripe\Assets\Util to replace removed League\Flysystem\Util and implement some of required methods.

Notes:

• Replace invoke Filesystem has() method to more appropriated directoryExists or fileExists, since has checks both cases directoryExists or fileExists and returns true, if one of them returns true;
• New listContents method returns DirectoryListing, but previous version returned array. For this case, invoke toArray method on result to return array to support existing functionality.

[emteknetnz]

Could you remove the Extended prefix on the new classes ExtendedFilesystem and ExtendedLocalFilesystemAdapter and instead prefix League to the import statement on the league class e.g.

use League\Flysystem\Filesystem as LeagueFilesystem;

We do a similar thing with Email - https://github.com/silverstripe/silverstripe-framework/blob/5/src/Control/Email/Email.php#L23

[emteknetnz]

Suggested change

	try {
	$callback($fs, $parsedFileID->getFileID());
	} catch (UnableToWriteFile $exception) {
	throw new UnableToWriteFile("Could not save {$filename}");
	$result = $callback($fs, $parsedFileID->getFileID());
	if (!$result) {
	throw new UnableToWriteFile("Could not save {$filename}");

If there's a UnableToWriteFile exception thrown from $callback() there's no need to catch it only to re-throw a new UnableToWriteFile exception

[sabina-talipova]

Since new filesystem operations no longer return boolean value, if filesystem operation was a success or not. Now they throw UnableTo*File exceptions and we should process these exceptions.
I implemented this changes following this Docs "No more success result booleans" paragraph and
Filesystem API

[sabina-talipova]

DONE

[emteknetnz]

Suggested change

	public static function isSeekableStream($resource)
	public static function isSeekableStream(resource $resource): void

[sabina-talipova]

DONE
I cannot add resource as a type hint.
Warning: "resource" is not a supported builtin type and will be interpreted as a class name.
See https://wiki.php.net/rfc/scalar_type_hints :
No type hint for resources is added, as this would prevent moving from resources to objects for existing extensions, which some have already done (e.g. GMP).

[emteknetnz]

Since this is in the Util class, may as well just add the following (adapted from this):

private static function checkIsResource($resource): void
{
    if (!is_resource($resource)) {
        throw new \InvalidArgumentException('$resource argument is not a valid resource');
    }
}

And then type check within rewindStream() and isSeekableStream()

I don't this we need to worry about the object scenario since stream_get_meta_data() is type hinted for resource

[sabina-talipova]

DONE

[emteknetnz]

Suggested change

	public static function rewindStream($resource)
	public static function rewindStream(resource $resource): void

[sabina-talipova]

DONE

[sabina-talipova]

I cannot add resource as a type hint.
Warning: "resource" is not a supported builtin type and will be interpreted as a class name.
See https://wiki.php.net/rfc/scalar_type_hints :
No type hint for resources is added, as this would prevent moving from resources to objects for existing extensions, which some have already done (e.g. GMP).

[emteknetnz]

As above, check use a new checkIsResource() method to type check

[sabina-talipova]

DONE

[emteknetnz]

A few more code changes requested

• See comments in the Files changed tab

Some possible references to removed methods may still exist

This list is largely for my own testing.

Looking at the list of methods removed in https://flysystem.thephpleague.com/docs/upgrade-from-1.x/

I tried searching for the following strings in ./vendor/silverstripe/**/*.php - the idea being that there may be some classes that haven't yet been updated and either weren't covered by unit tests in the assets module, or, they are called from different modules and possibly would get caught if we ran that PR against silverstripe/installer (more about that in the section below).

Looks like most things have been correctly updated.

->getMetaData(

• (none I think, though there's also multiple getMetaData() methods on other silverstripe classes)

['basename']

• HashFileIDHelper::parseFileID()
• LegacyFileIDHelper::parseFileID()
• LegacyFileIDHelper::parseSilverStripe30VariantFileID()
• NaturalFileIDHelper::parseFileID()

->rename(

• (none I think, though there's FlysystemAssetStore::rename())

->createDir(

• (none)

->update(

• (none I think, though there's also DataObject::update(), MemoryConfigCollection::update() and Config_ForClass::update())

->updateStream(

• (none)

->put(

• (none)

->putStream(

• (none)

->getTimestamp(

• (none I think, though there DBDate::getTimestamp())

->has(

• (large number of uses, though it doesn't look like it's been depracated or removed in 3.x)

->getMimetype(

• (none I think, though there is DBFile::getMimeType())

->getSize(

• (none I think, though there is File::getSize())

->getVisibility(

• (none I think, though there is File::getVisibility() and FlystemAssetStore::getVisibility())

May be worth testing with silverstripe/installer before merge

It may be worth running this through the silverstripe/installer CI using vendor-code-patcher

Unlike the last time we tried this, adding "league/flysystem": "3.9.0 as 1.99.99" to composer.json on the new branch of silverstripe/installer should work since it doesn't have any conflicting subdependencies

I'm not sure how much other modules such as asset-admin actually call league/flysystem methods directly, so there is some risk with merging this without it going via installer. However If you don't think it's worth doing this though, I'm OK with skipping this step

[emteknetnz]

Suggested change

	try {
	$callback($fs, $parsedFileID->getFileID());
	} catch (UnableToWriteFile $exception) {
	throw $exception;
	}
	$callback($fs, $parsedFileID->getFileID());

If all we're doing is re-throwing the exception after catching it, we may as well not bother catching it

[sabina-talipova]

DONE

[sabina-talipova]

@emteknetnz, I went through your list and this is a small report about my investigation:

Before I've started to work on upgrading new deps, I've checked do we use League\Flysystem classes anywhere else. Only silverstripe\assets uses this module.

• ['basename'] in the following methods HashFileIDHelper::parseFileID(), LegacyFileIDHelper::parseFileID(), LegacyFileIDHelper::parseSilverStripe30VariantFileID(), NaturalFileIDHelper::parseFileID() is a part of result of preg_match() php function. basename is a named subpattern.
  These parts are unrelated of changes that were made.

• getMetaData() methods in another classes related to SilverStripe\Assets\Storage\AssetStore interface and
  SilverStripe\Assets\Storage\AssetContainer interface.

  • class FlysystemAssetStore implements AssetStore implements getMetaData method and uses new methods in League\Flysystem\Filesystem class that replaced old League\Flysystem\Filesystem getMetaData() method.
  • class File extends DataObject implements AssetContainer in getMetaData() method uses getMetaData() method in DBFile class, that in its order uses getMetaData() existing in FlysystemAssetStore

• rename() is a method of FlysystemAssetStore class and it used League\Flysystem method rename() before. I've replaced it with new method move()

• update() methods are unrelated to new changes.

• getTimestamp() method is unrelated to new changes.

• has(). Most of the places where we invoke has method, we invoke them either in implementation of CacheInterface or in Injector class.

• Most of the invoke of has() method in silverstripe-assets. I implemented part of old has() method logic in new SilverStripe\Assets\Flysystem\Filesystem class.

• getMimetype() the similar situation as with getMetaData().

• getSize() in File class has its own logic, unrelated to new changes.

• getVisibility() the similar situation as with getMetaData()

[emteknetnz]

I tried running this PR using installer, there's single failing unit test on framework - https://github.com/emteknetnz/silverstripe-installer/actions/runs/3343823971/jobs/5537884602

I've fixed this on my local - could you please update GeneratedAssets::removeContent():

    public function removeContent($filename)
    {
        $filesystem = $this->getFilesystem();
        if ($filesystem->directoryExists($filename)) {
            $filesystem->deleteDirectory($filename);
        } elseif ($filesystem->fileExists($filename)) {
            $filesystem->delete($filename);
        }
    }