Let apps register Sabre plugins or collections (#26761)#2829
Let apps register Sabre plugins or collections (#26761)#2829LukasReschke wants to merge 1 commit into
Conversation
* Let apps register Sabre plugins or collections * Move comments DAV plugins to comments app * Fix use OCP\AppFramework\QueryException; Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
|
@LukasReschke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @blizzz, @rullzer and @nickvergessen to be potential reviewers. |
| return []; | ||
| } | ||
|
|
||
| private function loadSabrePluginsFromInfoXml($plugins) { |
There was a problem hiding this comment.
Let's change that to array $plugins
| return []; | ||
| } | ||
|
|
||
| private function extractCollectionList($array) { |
| } | ||
| } | ||
|
|
||
| private function loadSabreCollectionsFromInfoXml($collections) { |
| * @return array | ||
| */ | ||
| public function getAppPlugins() { | ||
| if (is_null($this->plugins)) { |
There was a problem hiding this comment.
out of curiosity, what's the difference?
There was a problem hiding this comment.
Performance, is_null is a function and therefor slower:
http://stackoverflow.com/questions/8228837/is-nullx-vs-x-null-in-php
There was a problem hiding this comment.
interesting, according to a user comment on php.net is_null is actually faster on php7 http://php.net/manual/en/function.is-null.php#117344
| * @return array | ||
| */ | ||
| public function getAppCollections() { | ||
| if (is_null($this->collections)) { |
| foreach ($this->appManager->getInstalledApps() as $app) { | ||
| // load plugins and collections from info.xml | ||
| $info = $this->appManager->getAppInfo($app); | ||
| if (!isset($info['types']) || !in_array('dav', $info['types'])) { |
LukasReschke
added
2. developing
| */ | ||
|
|
||
| namespace OCA\DAV\Comments; | ||
| namespace OCA\Comments\Dav; |
There was a problem hiding this comment.
This breaks 3rd party apps, that use comments, because now the file comments app needs to be enabled, while prior it worked because dav is force enabled.
So I'd like to not do this 😞
There was a problem hiding this comment.
The comments infrastructure was also intentionally decoupled from comments on files feature, what the comments app eventually does. Also not a fan.
There was a problem hiding this comment.
So @LukasReschke want to split the comments moving out of this PR? I think it makes sense to merge the base, but as said above not doing it for existing features.
|
Hey! |
|
I've cherry-picked the plugin/collection changes in #6835 I guess we can close this, since the comments code should stay in the dav app. |
9 participants
From owncloud/core#26761 – let's see what our integration tests say to this.