Introduce public storage class for extending

[PVince81]

Description

When writing external storage it is complicated to reimplement IStorage
and many others. This PR provides public classes to be extended to
provider storage specific implementations.

Related Issue

Because apps need to be able to extend the common storage class or use the flysystem adapter without accessing the private namespaces.

Also see owncloud-archive/documentation#2721

Motivation and Context

How Has This Been Tested?

• TODO: use it in files_external_ftp

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

@DeepDiver1975 @jvillafanez @labkode FYI

[PVince81]

Tested with this PR owncloud/files_external_ftp#11 from the FTP app.

To make it a bit better I'll also make the storages from files_external extend that one.

[PVince81]

Ok, done. The storages from files_external now extend OCP\Files\Storage\StorageAdapter.
I've tested with Dropbox and it still works.

[jvillafanez]

We should define the abstract methods required to be implemented, first in OC\Files\Storage\Common and then in the public adapter. That way is way easier for anyone to know what he should implement.

It's REALLY annoying creating a new subclass, implementing a couple of methods and when you consider it's finally ready then you notice that an additional method should be implemented because the class explodes.

[PVince81]

@jvillafanez do you mean checking all abstract methods from the interface and redefine these as abstract within the StorageAdapter class ?

Indeed, the Common class doesn't implement ALL the methods, there are still a few remaining... (me really longing for a better simplified storage API in the future)

[jvillafanez]

Yes. In case of the stat method, you have to check the new public class (not defined there), check the Common class (again not defined there) and finally the interface. The same happens with the getId method. Those are a lot of "jumps" to know what are the methods that should be implemented.

There is also a weird inheritance chain: if you want to implement the new public storage, you have to check the Common class, which is private, and then the interface, which is public again. Maybe the Common class should be moved to the public library....

I think it would be better if the abstract classes define the list of abstract methods and they also document how they should be implemented. That way, just looking at the class, you have all the information you need to implement the class.

[PVince81]

I think it would be better if the abstract classes define the list of abstract methods and they also document how they should be implemented. That way, just looking at the class, you have all the information you need to implement the class.

Ok, I'll do that short term.

Middle to long term we should probably move all methods from Common into StorageAdapter and make every other storage, even the internal ones, extend from it.

[PVince81]

@jvillafanez I've added the abstract methods that a storage must implement in StorageAdapter and FlysystemStorageAdapter.

[PVince81]

Okay, the code checker wants me to add @since tags for this "new" public APIs, will do...

[jvillafanez]

The only thing I miss is the expected exceptions the methods are supposed to throw. The rest is really nice!!.

Should we include some documentation of common methods that might be overwritten for performance? I'm thinking about the rename, isReadable, isCreatable, etc.

[PVince81]

Ahem ahem, there are no expected exceptions in this API... 😢
Do you mean adding StorageNotAvailableException in every method signature ?

As for the docs, we could extend what I already posted to mention best practices when implementing an ext storage: owncloud-archive/documentation#2728

[jvillafanez]

Do you mean adding StorageNotAvailableException in every method signature ?

At least that is being taken into account I think.

The problem is that if nobody tells you that you should throw an StorageNotAvailableException when you can't connect to the server, what I'd return is a false or any other value that could be interpreted as an error, which might cause some issues.

If there is no expected exceptions, throwing that exception is conceptually wrong 😢

[PVince81]

In the long term I hope we can redefine the storage APIs completely to be more simply and base them on our own concept (#22388 (comment)), not mimic PHP's system functions, which means exceptions for any errors.

@jvillafanez do you want me to add StorageNotAvailableException in the PHPDoc of the adapters for now ?

[jvillafanez]

If it's being handled I think it's good to have it documented

[PVince81]

@jvillafanez added here 4208efc

I took the opportunity to also add these on the IStorage interface where they were missing...

[jvillafanez]

duplicated below

[PVince81]

my copy-paste skills have failed me 😢

[jvillafanez]

Other than that, 👍

[PVince81]

Fixed and squashed

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.