Make DI work for all apps #3977
Conversation
As stated in #3901 (comment) appid's don't have to match the namespace. Work around this Signed-off-by: Roeland Jago Douma <[email protected]>
|
@rullzer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @BernhardPosselt, @nickvergessen and @ChristophWurst to be potential reviewers. |
|
Isn't \OC\AppFramework\App::buildAppNamespace supposed to take care of that? see https://github.com/nextcloud/server/blob/master/lib/private/AppFramework/App.php#L56 If there's an issue for an app it should add the namespace to its info.xml |
|
@BernhardPosselt I assume you are refering to https://github.com/nextcloud/server/pull/3977/files#diff-954b90756615239eaac02e54722e1f1aR396 |
|
@BernhardPosselt also note that the merged code did not yet get the namespace of the app. |
| return parent::query($name); | ||
| } else if ($this['AppName'] === 'core' && strpos($name, 'OC\\Core\\') === 0) { | ||
| return parent::query($name); | ||
| } else if (strpos($name, \OC\AppFramework\App::buildAppNamespace($this['AppName'])) === 0 || |
There was a problem hiding this comment.
This breaks for the files app, it will match any classes OCA\Files.
You need to add the trailing slash on buildAppNamespace
| return parent::query($name); | ||
| } else if ($this['AppName'] === 'core' && strpos($name, 'OC\\Core\\') === 0) { | ||
| return parent::query($name); | ||
| } else if (strpos($name, \OC\AppFramework\App::buildAppNamespace($this['AppName'])) === 0 || | ||
| $this->getFallbackNamespace($name) === \OC\AppFramework\App::buildAppNamespace($this['AppName'])) { |
There was a problem hiding this comment.
Kill this,
buildAppNamespace already does it:
// if the tag is not found, fall back to uppercasing the first letter
self::$nameSpaceCache[$appId] = ucfirst($appId);
There was a problem hiding this comment.
This is needed because else all the unit tests break for the TwoFactor auth from @ChristophWurst (I'll kick him about that next week)
Signed-off-by: Joas Schilling <[email protected]>
|
Also resolved the comment from Bernhard |
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Roeland Jago Douma <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #3977 +/- ##
============================================
+ Coverage 54.28% 54.33% +0.04%
- Complexity 21161 21163 +2
============================================
Files 1305 1305
Lines 80801 80817 +16
Branches 1282 1282
============================================
+ Hits 43863 43911 +48
+ Misses 36938 36906 -32
Continue to review full report at Codecov.
|
As stated in #3901 (comment)
appid's don't have to match the namespace.
Work around this
Followup of #3901
@icewind1991 please verify