Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load apps that have a dav type set before the dav server plugins #12448

Merged
merged 1 commit into from
Nov 14, 2018

Conversation

juliusknorr
Copy link
Member

To implement comments support for an app, it is required to register a comments entity:

$this->eventDispatcher->addListener(CommentsEntityEvent::EVENT_ENTITY, function(CommentsEntityEvent $event) {
			$event->addEntityCollection('deckCard', function($name) {});
});

This is usually done in the app.php. When using the comments dav endpoint, the app is only loaded if either one of the types logging/authentication/filesystem is set in the info.xml, but this will remove the ability to enable the app just for a specific group. See

We already have a type 'dav' that is used for loading dav plugins, so this PR will make sure we load app apps with a type 'dav' set in info.xml before registering plugins in the DAV Server class.

cc @nickvergessen @blizzz

@@ -103,9 +103,7 @@ private function populate() {
if (!isset($info['types']) || !in_array('dav', $info['types'], true)) {
continue;
}
// FIXME: switch to public API once available
// load app to make sure its classes are available

This comment was marked as resolved.

@blizzz
Copy link
Member

blizzz commented Nov 14, 2018

then we should add this type for comments, too, right? Not sure which other apps have plugins.

(it does incorporate logging, but we should not rely on that)

@juliusknorr
Copy link
Member Author

then we should add this type for comments, too, right? Not sure which other apps have plugins.

The comments plugins are still located in the dav app, so for now we don't need that i guess.

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reasonable

@MorrisJobke MorrisJobke mentioned this pull request Nov 14, 2018
24 tasks
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code makes sense 👍

@MorrisJobke MorrisJobke merged commit c993b6e into master Nov 14, 2018
@MorrisJobke MorrisJobke deleted the bugfix/noid/dav-loadapps branch November 14, 2018 20:42
@juliusknorr
Copy link
Member Author

/backport to stable14

@juliusknorr
Copy link
Member Author

/backport to stable13

@backportbot-nextcloud
Copy link

backport to stable13 in #12594

@backportbot-nextcloud
Copy link

backport to stable13 in #12595

@juliusknorr
Copy link
Member Author

/backport to stable14

@backportbot-nextcloud
Copy link

backport to stable14 in #12596

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants