Add a public api for apps to add mounts #12577

icewind1991 · 2014-12-03T13:30:44Z

This adds a public api which apps can use to add additional mountpoints in a more structured way then listening to hooks.

First PR for #12216

cc @PVince81 @DeepDiver1975

LukasReschke · 2014-12-03T13:32:50Z

apps/files_external/lib/config/configadapter.php

+			if (isset($options['personal']) && $options['personal']) {
+				$mounts[] = new PersonalMount($options['class'], $mountPoint, $options['options'], $loader);
+			} else {
+				$mounts[] = new Mount($options['class'], $mountPoint, $options['options'], $loader);


Is expecting a Loader, you're passing an ILoader. - IDE complains.
(same for PersonalMount above)

PVince81 · 2014-12-03T14:22:26Z

This is going in the right direction but probably needs more work to find out whether we can reduce the number of classes/interfaces that work with storages and mount points.

Just throwing a few ideas:

If any mount point is a storage anyway, should we make the concept "mount point" and "storage" into a single class ? IStorageManager would be a collection or storages ?
If we do keep them separate:
Instantiate a mount point/storage with a IStorageFactory (instead of ILoader).
if MountManager is a collection of mount points, could this be merged into the Filesystem class (or a future non-static Filesystem class/service ?)
...

icewind1991 · 2014-12-03T14:30:29Z

If any mount point is a storage anyway, should we make the concept "mount point" and "storage" into a single class

A single storage can belong to multiple mount points (i.e. external storage which is mounted for multiple users)

Instantiate a mount point/storage with a IStorageFactory (instead of ILoader).

👍

if MountManager is a collection of mount points, could this be merged into the Filesystem class (or a future non-static Filesystem class/service ?)

I would rather keep it a separate class (composition) and have IMountManager as member of \OC\Files\Node\Root which would become the main "state" of the fs

PVince81 · 2014-12-03T14:30:54Z

lib/public/files/config/imountprovider.php

+	 * @param \OCP\Files\Storage\ILoader $loader
+	 * @return \OCP\Files\Mount\IMount[]
+	 */
+	public function getMountsForUser(IUser $user, ILoader $loader);


I wonder, should it rather be: instantiate a MountManager with an injected IUser, and then just call getMounts() to get that user's mount ? This way every user has a different mount point. Unless you say this is needed to be able to switch between users due to shares ?

Yes, because setting up mounts for multiple users is fairly common due to shares imo it makes more sense to have the user as argument here

Ok, based on the explanation you gave above it makes sense. If IMountProvider is some kind of factory that will instantiate mounts, then it has to be global.

Okay, so IMountProvider is the instance that the newer ext storage plugins will have to implement and register.
The providers are registered to the global IMountManager (aka IMountProviderCollection)

And then the filesystem code will ask the IMountManager to give a list of IMount for a given user. Got it.

So in general it means that what we call a mount point "IMount" is IStorage + storage Settings + scope (applicable user/group)

Hmmm... would it make sense to separate the scope/user from the storage config and put it on a different level ? Storage config should only be about settings like host, username, password, etc.

Maybe this could help remove the distinction between Mount and Storage and we could somehow merge them.

DeepDiver1975 · 2014-12-03T14:38:47Z

currently some apps out there are using following code piece to register external storage implementations:
https://github.com/owncloud/core/blob/master/apps/files_external/lib/config.php#L45
https://github.com/owncloud/core/blob/master/apps/files_external/appinfo/app.php#L45

Is this already respected in this pr?
Will this be addressed in a second pr?

THX

DeepDiver1975 · 2014-12-03T14:48:42Z

@icewind1991 ^^

icewind1991 · 2014-12-03T14:49:56Z

@DeepDiver1975 that's for another PR, this leaves the Config class intact for now

PVince81 · 2014-12-03T15:11:54Z

Would be cool if you could post a diagram (even if scanned from paper). We could then reuse it later for the documentation. There are already too many classes and it's easy to lose the overview.

PVince81 · 2014-12-03T15:31:54Z

@icewind1991 I hope you don't mind the various suggestions, I just wanted to make sure we considered these many alternatives 😄

icewind1991 · 2014-12-03T16:09:37Z

Provides a general overview of the components and their relations.

This PR introduces the Config section which is currently done by hooks

#11091 introduces the Factory class which is currently done by both \OC\Files\Filesystem and \OC_Util

\OC\Files\Node\Root is the base of the "modern" filesystem api and is the alternative for the "old" filesystem api \OC\Files\Filesystem and \OC\Files\View

PVince81 · 2014-12-03T16:21:40Z

Nice diagram! Thanks a lot, that clears out a bit 😄

PVince81 · 2014-12-03T16:25:33Z

lib/public/files/mount/imountpoint.php

+	 *
+	 * @param string $mountPoint new mount point
+	 */
+	public function setMountPoint($mountPoint);


Bringing back an older question: should the IMountPoint instance know where it is mounted ? Or should this be stores on a highler level, the mount manager ? But maybe that information is needed in this level already to be able to move mount points.

On the other hand you could say that FileInfo represents a file and it also contains the full path, so might be fine.

I would keep it at the current level so we have a single object that holds all info needed to use the mounted storage

scrutinizer-notifier · 2014-12-04T20:00:45Z

The inspection completed: 6 new issues, 25 updated code elements

icewind1991 · 2014-12-05T12:25:18Z

@owncloud-bot retest this please

ghost · 2014-12-05T13:16:49Z

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/3841/
🚀 Test PASSed. 🚀

icewind1991 · 2014-12-05T13:19:50Z

@DeepDiver1975 @schiesbn @PVince81 can this be merged?

PVince81 · 2014-12-08T10:49:32Z

👍 makes sense, as discussed

LukasReschke · 2014-12-08T21:57:31Z

👍

Add a public api for apps to add mounts

jnfrmarks · 2015-01-23T13:26:34Z

@LukasReschke

Is there a write up of the api? I assume that we need to doc and test this for 8.0.

LukasReschke · 2015-01-23T13:27:46Z

That's an internal API for application developers, so nothing directly exposed to the end-user.

As far I know there is no documentation about this for developers, however there is owncloud-archive/documentation#693 to track this.

jnfrmarks · 2015-01-23T13:36:37Z

What is meant by "public"? I thought that meant available to all, not just internal use.

LukasReschke · 2015-01-23T13:41:00Z

A public API refers to an API exposed to application developers. An internal API is just to be used by the core folks.

This is just a programming API, not something exposed via REST / SOAP / etc ;-)

DeepDiver1975 · 2015-01-23T13:43:09Z

It's the public PHP API - used by application developers. And yes this means we need to document this - just like a hell of other php classes - see http://api.owncloud.org/namespaces/OCP.Files.Config.html

LukasReschke · 2015-01-23T13:45:07Z

Well - that's why I linked to #12577 (comment) ;-) – But there is no need that @jnfrmarks tests this IMHO

DeepDiver1975 · 2015-01-23T13:58:21Z

But there is no need that @jnfrmarks tests this IMHO

indeed - nothing to take care of from an QA perspective

LukasReschke reviewed Dec 3, 2014
View reviewed changes

PVince81 reviewed Dec 3, 2014
View reviewed changes

icewind1991 force-pushed the public-mount-api branch from 0350ec3 to 1852ddc Compare December 3, 2014 15:27

PVince81 reviewed Dec 3, 2014
View reviewed changes

icewind1991 force-pushed the public-mount-api branch 2 times, most recently from 27978a5 to 66d4422 Compare December 4, 2014 14:34

icewind1991 added 2 commits December 4, 2014 16:47

Add public api for mount configurations

f4701d7

Use the mount config api for files_external

a369d78

icewind1991 force-pushed the public-mount-api branch from 66d4422 to a369d78 Compare December 4, 2014 15:47

LukasReschke mentioned this pull request Dec 5, 2014

User Management - ownCloud 8.0 #12620

Closed

23 tasks

MorrisJobke mentioned this pull request Dec 8, 2014

Filesystem documentation owncloud-archive/documentation#693

Closed

LukasReschke added a commit that referenced this pull request Dec 8, 2014

Merge pull request #12577 from owncloud/public-mount-api

25a87d4

Add a public api for apps to add mounts

LukasReschke merged commit 25a87d4 into master Dec 8, 2014

LukasReschke deleted the public-mount-api branch December 8, 2014 21:57

MorrisJobke added this to the 8.0-current milestone Jan 23, 2015

lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a public api for apps to add mounts #12577

Add a public api for apps to add mounts #12577

icewind1991 commented Dec 3, 2014

LukasReschke Dec 3, 2014

icewind1991 Dec 3, 2014

PVince81 commented Dec 3, 2014

icewind1991 commented Dec 3, 2014

PVince81 Dec 3, 2014

icewind1991 Dec 3, 2014

PVince81 Dec 3, 2014

PVince81 Dec 3, 2014

DeepDiver1975 commented Dec 3, 2014

DeepDiver1975 commented Dec 3, 2014

icewind1991 commented Dec 3, 2014

PVince81 commented Dec 3, 2014

PVince81 commented Dec 3, 2014

icewind1991 commented Dec 3, 2014

PVince81 commented Dec 3, 2014

PVince81 Dec 3, 2014

icewind1991 Dec 3, 2014

scrutinizer-notifier commented Dec 4, 2014

icewind1991 commented Dec 5, 2014

ghost commented Dec 5, 2014

icewind1991 commented Dec 5, 2014

PVince81 commented Dec 8, 2014

LukasReschke commented Dec 8, 2014

jnfrmarks commented Jan 23, 2015

LukasReschke commented Jan 23, 2015

jnfrmarks commented Jan 23, 2015

LukasReschke commented Jan 23, 2015

DeepDiver1975 commented Jan 23, 2015

LukasReschke commented Jan 23, 2015

DeepDiver1975 commented Jan 23, 2015

Add a public api for apps to add mounts #12577

Add a public api for apps to add mounts #12577

Conversation

icewind1991 commented Dec 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Dec 3, 2014

icewind1991 commented Dec 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepDiver1975 commented Dec 3, 2014

DeepDiver1975 commented Dec 3, 2014

icewind1991 commented Dec 3, 2014

PVince81 commented Dec 3, 2014

PVince81 commented Dec 3, 2014

icewind1991 commented Dec 3, 2014

PVince81 commented Dec 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scrutinizer-notifier commented Dec 4, 2014

icewind1991 commented Dec 5, 2014

ghost commented Dec 5, 2014

icewind1991 commented Dec 5, 2014

PVince81 commented Dec 8, 2014

LukasReschke commented Dec 8, 2014

jnfrmarks commented Jan 23, 2015

LukasReschke commented Jan 23, 2015

jnfrmarks commented Jan 23, 2015

LukasReschke commented Jan 23, 2015

DeepDiver1975 commented Jan 23, 2015

LukasReschke commented Jan 23, 2015

DeepDiver1975 commented Jan 23, 2015