Let apps register Sabre plugins or collections by PVince81 · Pull Request #26761 · owncloud/core

PVince81 · 2016-12-02T21:31:38Z

Description

See #26195 (comment) for info.xml format.
This checks the sabre tags in info.xml for all apps and automatically registers the Sabre plugins and collections to the root collection.
Note that this only happens for v2 remote.php.

Related Issue

Fixes #26195

Motivation and Context

Since we want Webdav to be used for APIs, apps need a way to register Sabre plugins and also to hook their collections to the remote.php/dav collection tree.

How Has This Been Tested?

Manually tested with the customgroups app, not ready yet.

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 check this out

mention-bot · 2016-12-02T21:31:41Z

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975 and @tcitworld to be potential reviewers.

PVince81 · 2016-12-02T21:46:27Z

~~TODO: also register sabre plugins in v1 endpoint~~ see discussion

PVince81 · 2016-12-02T21:46:52Z

maybe let info.xml specify for which endpoint v1, v2 or both through attributes ?

PVince81 · 2016-12-02T21:54:38Z

TODO: attr to tell whether the plugin needs to be registered also for public webdav ? (mostly for file properties)

PVince81 · 2016-12-02T21:56:24Z

To demo and test this feature, I tried moving the DAV plugins for the comments into the "comments" app in 64b66d7

So far only the root collection seems to work. Might need further tweaking.

PVince81 · 2016-12-02T22:02:46Z

BUG: looks like IUserSession isn't initialized/populated yet when a plugin's initialize() is called, so makes it difficult to check the current user or check if user is authed (happens in CommentsPlugin):

PVince81 · 2016-12-02T22:11:12Z

Solved the auth issue by moving the registration of app sabre plugins into beforeMethod handler: 010d4a4

DeepDiver1975 · 2016-12-04T11:14:29Z

 	}

+	private function registerSabrePluginsFromApps(\OCA\DAV\Server $server) {
+		\OC_App::loadApps();


load all apps? in the past we only did load apps of type filesystem to ensure some performance on the dav backend.
new app type? e.g. dav?

A new app type sounds great, I'll add it

DeepDiver1975 · 2016-12-04T11:14:45Z

+			}
+			// FIXME: switch to public API once available
+			// load commands using info.xml
+			$info = \OC_App::getAppInfo($app);


here: https://github.com/owncloud/core/pull/26588/files#diff-0513a8452fe3af4380e1bb03e0ef6aee

DeepDiver1975 · 2016-12-04T11:15:17Z

+			try {
+				return \OC::$server->query($plugin);
+			} catch (QueryException $e) {
+				if (class_exists($command)) {


$command? unknown

uh oh... I copied your command code and forgot to replace

DeepDiver1975 · 2016-12-04T11:15:30Z

+			try {
+				return \OC::$server->query($plugin);
+			} catch (QueryException $e) {
+				if (class_exists($command)) {


DeepDiver1975 · 2016-12-04T11:17:04Z

TODO: also register sabre plugins in v1 endpoint

v1 only holds files - and it's just the old interface - no need to anything new there

PVince81 · 2016-12-05T08:36:19Z

@DeepDiver1975 ok then we can't port the "comments" app to be a "dav" app with Sabre plugins, because then the comments plugin would be missing in the v1 endpoint.

The other alternative is to change the web UI to use webdav v2: #25494. But then if there's any client who used to expected the oc:comments-unread property on the old endpoint they'd stop working. (but the likeliness of the existence of such clients is very low)

DeepDiver1975 · 2016-12-05T09:05:12Z

@DeepDiver1975 ok then we can't port the "comments" app to be a "dav" app with Sabre plugins, because then the comments plugin would be missing in the v1 endpoint.

comments arn't even part of v1 as of today - right? So no need to add it

PVince81 · 2016-12-05T09:06:47Z

comments arn't even part of v1 as of today

Only one piece is: we have a Webdav property on files that is added through the CommentsPlugin. The property is there to tell how many unread comments there are to be able to display an indicator in the UI.
Without this property we'd need to do extra API calls + JS-side joins to be able to add this info to the file list.

DeepDiver1975 · 2016-12-05T09:07:35Z

Only one piece is: we have a Webdav property on files that is added through the CommentsPlugin. The property is there to tell how many unread comments there are to be able to display an indicator in the UI.
Without this property we'd need to do extra API calls + JS-side joins to be able to add this info to the file list.

I see - lets hardcode this piece and we are fine

PVince81 · 2016-12-05T09:09:02Z

Hardcode, hmmm... There are multiple levels where we can do this:

Keep CommentsPlugin within the "dav" app
or
Move the oc:comments-unread property logic to the FilesPlugin. But then it would conflict with the CommentsPlugin one when the "comments" app is enabled.

So I guess 1) is the best approach for now.

PVince81 · 2016-12-05T12:31:20Z

Rebased onto master to be able to use one of the new IAppManager calls.

Remaining tasks:

write unit tests
move CommentPropertiesPlugin back to apps/dav for backward compatibility
clear out public webdav case (could be done separately as we currently seldom register plugins for that one, but will need it eventually once we do federated metadata, etc)

PVince81 · 2016-12-05T18:08:57Z

I refactored the whole thing to simplify it.
Wrote a unit test for the plugin manager.

Please review @DeepDiver1975 @jvillafanez @VicDeo @PhilippSchaffrath

First have a look at 07d43a5 which brings the Sabre plugins registration code.
Then see how it's used in ace036b where the comments app is changed to use it.

DeepDiver1975 · 2016-12-06T20:52:08Z

+		foreach ($collections as $collection) {
+			try {
+				$this->collections[] = $this->container->query($collection);
+			} catch (QueryException $e) {


QueryException is unknown

DeepDiver1975 · 2016-12-06T22:42:05Z

👍

lock · 2019-08-04T06:02:44Z

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.

PVince81 added the 2 - Developing label Dec 2, 2016

PVince81 added this to the 9.2 milestone Dec 2, 2016

PVince81 mentioned this pull request Dec 2, 2016

Provide API to let apps register their own Sabre plugins #26195

Closed

PVince81 force-pushed the dav-plugins-from-apps branch from 010d4a4 to e31bfce Compare December 3, 2016 14:53

DeepDiver1975 reviewed Dec 4, 2016

View reviewed changes

PVince81 force-pushed the dav-plugins-from-apps branch from e31bfce to a050cad Compare December 5, 2016 12:29

Vincent Petry added 2 commits December 5, 2016 19:07

Let apps register Sabre plugins or collections

07d43a5

Move comments DAV plugins to comments app

ace036b

PVince81 force-pushed the dav-plugins-from-apps branch from a050cad to ace036b Compare December 5, 2016 18:07

PVince81 added 3 - To Review and removed 2 - Developing labels Dec 5, 2016

PVince81 added the app:dav label Dec 5, 2016

Fix use OCP\AppFramework\QueryException;

8f2c9cb

DeepDiver1975 requested changes Dec 6, 2016

View reviewed changes

DeepDiver1975 approved these changes Dec 6, 2016

View reviewed changes

DeepDiver1975 merged commit 64a4daa into master Dec 6, 2016

DeepDiver1975 deleted the dav-plugins-from-apps branch December 6, 2016 23:19

LukasReschke mentioned this pull request Dec 22, 2016

Let apps register Sabre plugins or collections (#26761) nextcloud/server#2829

Closed

lock Bot locked as resolved and limited conversation to collaborators Aug 4, 2019

Uh oh!

Conversation

PVince81 commented Dec 2, 2016

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

mention-bot commented Dec 2, 2016

Uh oh!

PVince81 commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Dec 2, 2016

Uh oh!

PVince81 commented Dec 2, 2016

Uh oh!

PVince81 commented Dec 2, 2016

Uh oh!

PVince81 commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Dec 2, 2016

Uh oh!

DeepDiver1975 Dec 4, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 Dec 4, 2016

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 Dec 4, 2016

Choose a reason for hiding this comment

Uh oh!

PVince81 Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 Dec 4, 2016

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 commented Dec 4, 2016

Uh oh!

PVince81 commented Dec 5, 2016

Uh oh!

DeepDiver1975 commented Dec 5, 2016

Uh oh!

PVince81 commented Dec 5, 2016

Uh oh!

DeepDiver1975 commented Dec 5, 2016

Uh oh!

PVince81 commented Dec 5, 2016

Uh oh!

PVince81 commented Dec 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Dec 5, 2016

Uh oh!

DeepDiver1975 Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 commented Dec 6, 2016

Uh oh!

lock Bot commented Aug 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PVince81 commented Dec 2, 2016 •

edited

Loading

PVince81 commented Dec 2, 2016 •

edited

Loading

PVince81 commented Dec 5, 2016 •

edited

Loading