silverstripe / silverstripe-assets

Silverstripe Assets component
BSD 3-Clause "New" or "Revised" License
9 stars 67 forks source link

Use FilesystemInterface instead of Filesystem dependency #368

Open wernerkrauss opened 4 years ago

wernerkrauss commented 4 years ago

I'm trying to implement Flysystem's MountManager which is unfortunaltely not a Filesystem per se, but uses the same interface and can be used like a filesystem.

Unfortunately this is not easily plugable, cause all the dependencies in the code are against the Filesystem class itself, not against its interface.

Is there any reason for this or can we change it to use the interface instead?

maxime-rainville commented 4 years ago

Humm ... that sounds like a very good point and a lack of foresight on our part. Unfortunately, those APIs are baked in now and I don't think we could fix the Silverstripe CMS 4 release line, so it would need to be fixed in the next major release.

kinglozzer commented 4 years ago

@maxime-rainville Filesystem already implements FilesystemInterface, so AFAICT changing typehints shouldn’t cause any issues. @wernerkrauss is that all that needs to be changed to fix this, or are there any other API changes?

maxime-rainville commented 4 years ago

The problem is that there's a few places where we explicitly say this method expect a Filesystem object. Fortunately, most of those are on some of the works I've done in 4.4, which we've marked as @internal so we've got a semver op-tout. However, I'm pretty sure there will be at least a few instance that were pre-existing.

Just had a look at FlysystemAssetStore. I think that's going to be the most problematic one. It does have quite a few explicit references to Filesystem and I didn't mark those as @internal.

wernerkrauss commented 4 years ago

I just did a comparison between Filesystem Object and Filesystem Interface.

Filesystem has some methods that are not part of the Inteface:

getAdapter()
assertAbsent()
assertPresent()
forceCopy()
forceRename()
getWithMetadata()
listFiles()
listPaths()
listWith()

only getAdapter() is used in Assets module which is also available in MountManager. Can't this be catched when adding the Adapter(Interface) objects to AssetStore?