Implement AppManager::getAppPath #29871

VicDeo · 2017-12-15T17:39:52Z

Description

Allows to get the app path and app web path
Kill legacy code that does the same. OC_App::getAppPath and OC_App::getAppWebPath are cut down to work via AppManager implementation
It is possible to use some AppManager methods without Db connection now (e.g. when OC is not installed)

Related Issue

Motivation and Context

public API enhancement
unit testing is possible now
killing old crap

How Has This Been Tested?

\OC::$server->getAppManager()->getAppPath('customgroups');
\OC::$server->getAppManager()->getAppWebPath('customgroups');

where 'customgroups' is your favorite app. In a various configs including multiple app directories.

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.

phil-davis · 2017-12-15T21:35:19Z

lib/private/App/AppManager.php

+	}
+
+	/**
+	 * Get the directory for the given app.


Method description needs an update. (copy-pasted from method above but not yet modified)

codecov · 2017-12-20T11:11:19Z

Codecov Report

Merging #29871 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #29871      +/-   ##
============================================
- Coverage     58.21%   58.19%   -0.03%     
+ Complexity    18512    18458      -54     
============================================
  Files          1093     1092       -1     
  Lines         63693    63619      -74     
============================================
- Hits          37082    37025      -57     
+ Misses        26611    26594      -17

Impacted Files	Coverage Δ	Complexity Δ
drone/src/apps/dav/lib/Connector/Sabre/File.php	`83.33% <0%> (-1.15%)`	`103% <0%> (-36%)`
drone/src/lib/private/Server.php	`82.47% <0%> (-0.24%)`	`129% <0%> (+1%)`
drone/src/lib/private/Files/View.php	`84.61% <0%> (-0.18%)`	`370% <0%> (-6%)`
drone/src/lib/private/legacy/app.php	`59.11% <0%> (-0.14%)`	`184% <0%> (-14%)`
drone/src/lib/private/User/User.php	`82.82% <0%> (-0.11%)`	`63% <0%> (ø)`
drone/src/lib/private/User/Session.php	`62.13% <0%> (-0.04%)`	`124% <0%> (ø)`
drone/src/lib/private/SystemConfig.php	`44.44% <0%> (ø)`	`14% <0%> (ø)`	⬇️
...ate/Files/External/Service/UserStoragesService.php	`100% <0%> (ø)`	`11% <0%> (ø)`	⬇️
drone/src/lib/private/Files/Filesystem.php	`70.03% <0%> (ø)`	`127% <0%> (ø)`	⬇️
.../private/Files/External/Auth/Password/Password.php	`0% <0%> (ø)`	`1% <0%> (ø)`	⬇️
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cce5ef...131e688. Read the comment docs.

PVince81

👍 looks good to me.

Feel free to ignore the comments if they apply on old code you just moved to another location. If you do fix them, let's make sure there is no risk of regression.

PVince81 · 2017-12-21T10:49:09Z

lib/private/App/AppManager.php

+		if (trim($appId) === '') {
+			return false;
+		}
+		if (($appRoot = $this->findAppInDirectories($appId)) != false) {


!== false ?

same below

feel free to leave it if it's just copy-pasted bad old code

PVince81 · 2017-12-22T09:52:13Z

lib/private/App/AppManager.php

+	 * @param string $appId
+	 * @return false|string[]
+	 */
+	protected  function findAppInDirectories($appId) {


double space between protected and function

PVince81 · 2017-12-22T09:52:51Z

lib/private/App/AppManager.php

+			$versionToLoad = [];
+			foreach ($possibleAppRoots as $possibleAppRoot) {
+				$version = $this->getAppVersionByPath($possibleAppRoot['path'] . '/' . $appId);
+				if (empty($versionToLoad) || version_compare($version, $versionToLoad['version'], '>')) {


empty() , fortunately we don't have a version "0" which would qualify as true for empty("0")

OMG! an evil app-developer can write an app that will never be loaded :)
Just kidding. this piece of sh code chooses the most recent app version from the appRoots.
$versionToLoad is an array containing properties ['path', 'url', 'version'] of the winner so empty is valid in this context.
empty($versionToLoad) means there is no winner currently.

VicDeo · 2017-12-26T15:07:26Z

@PVince81 I changed comparison to be strict and removed double space.
findAppInDirectories always returns either false or non-empty array so these changes are regression-safe. (going to retest)

PVince81 · 2018-01-08T10:00:02Z

lib/public/App/IAppManager.php

@@ -166,4 +166,26 @@ public function readAppPackage($path);
 	 * @since 10.0.3
 	 */
 	public function canInstall();
+
+	/**
+	 * Get the directory for the given app.


mind adding a detail whether this is an absolute path or relative path (and relative to what)

PVince81 · 2018-01-08T10:00:16Z

lib/public/App/IAppManager.php

+	 * @return string|false
+	 * @since 10.0.5
+	 */
+	public function getAppWebPath($appId);


what's a web path ? maybe an example in PHPDoc will help clarify

PVince81

Please check the two clarifications (optional)

VicDeo · 2018-01-08T10:06:04Z

@PVince81 thanks, will do

//Just for my reference as both comments are related to
apps_paths content https://github.com/owncloud/core/blob/master/config/config.sample.php#L715

VicDeo · 2018-01-08T13:03:51Z

@PVince81 PHPDocs updated

PVince81 · 2018-01-08T13:04:36Z

tests were green before, only PHP Doc changes, insta-merge

PVince81 · 2018-01-08T13:04:50Z

@VicDeo please backport to stable10

VicDeo · 2018-01-08T13:19:34Z

Stable10: #30041

lock · 2019-08-01T13:02:28Z

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.

VicDeo self-assigned this Dec 15, 2017

VicDeo added this to the development milestone Dec 15, 2017

VicDeo added the 2 - Developing label Dec 15, 2017

VicDeo force-pushed the implement-25336 branch 2 times, most recently from 42c6a97 to 44b9f2a Compare December 15, 2017 18:27

phil-davis reviewed Dec 15, 2017

View reviewed changes

VicDeo force-pushed the implement-25336 branch 2 times, most recently from 24bccd0 to 1da9920 Compare December 19, 2017 21:08

VicDeo force-pushed the implement-25336 branch from 1da9920 to 7532b0d Compare December 20, 2017 19:15

VicDeo changed the title ~~[WIP] Implement getAppPath~~ Implement getAppPath Dec 20, 2017

VicDeo added 3 - To Review backport-request and removed 2 - Developing labels Dec 20, 2017

VicDeo requested a review from PVince81 December 21, 2017 10:14

PVince81 approved these changes Dec 22, 2017

View reviewed changes

VicDeo force-pushed the implement-25336 branch from 7532b0d to ccd4f5a Compare December 26, 2017 14:59

VicDeo changed the title ~~Implement getAppPath~~ Implement AppManager::getAppPath Dec 26, 2017

ownclouders added the status/STALE label Jan 3, 2018

VicDeo removed the status/STALE label Jan 5, 2018

ownclouders added the status/STALE label Jan 5, 2018

PVince81 reviewed Jan 8, 2018

View reviewed changes

PVince81 approved these changes Jan 8, 2018

View reviewed changes

ownclouders removed the status/STALE label Jan 8, 2018

VicDeo added 2 commits January 8, 2018 15:46

Refactor getAppPath

391a09b

Add more explanations to the method descriptions

131e688

VicDeo force-pushed the implement-25336 branch from ccd4f5a to 131e688 Compare January 8, 2018 13:02

PVince81 merged commit 9dbcc9a into master Jan 8, 2018

PVince81 deleted the implement-25336 branch January 8, 2018 13:04

VicDeo mentioned this pull request Jan 8, 2018

[Stable10] Implement AppManager::getAppPath #30041

Merged

9 tasks

VicDeo removed the backport-request label Jan 8, 2018

This was referenced Jan 15, 2018

Do not cache app path if app is not found #30144

Merged

[Stable10] Do not cache app path if app is not found #30150

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement AppManager::getAppPath #29871

Implement AppManager::getAppPath #29871

VicDeo commented Dec 15, 2017 •

edited

Loading

phil-davis Dec 15, 2017

codecov bot commented Dec 20, 2017 •

edited

Loading

PVince81 left a comment

PVince81 Dec 21, 2017

PVince81 Dec 22, 2017

PVince81 Dec 22, 2017

PVince81 Dec 22, 2017

VicDeo Dec 26, 2017 •

edited

Loading

VicDeo commented Dec 26, 2017

PVince81 Jan 8, 2018

PVince81 Jan 8, 2018

PVince81 left a comment

VicDeo commented Jan 8, 2018

VicDeo commented Jan 8, 2018

PVince81 commented Jan 8, 2018

PVince81 commented Jan 8, 2018

VicDeo commented Jan 8, 2018

lock bot commented Aug 1, 2019

Implement AppManager::getAppPath #29871

Implement AppManager::getAppPath #29871

Conversation

VicDeo commented Dec 15, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

phil-davis Dec 15, 2017

Choose a reason for hiding this comment

codecov bot commented Dec 20, 2017 • edited Loading

Codecov Report

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 Dec 21, 2017

Choose a reason for hiding this comment

PVince81 Dec 22, 2017

Choose a reason for hiding this comment

PVince81 Dec 22, 2017

Choose a reason for hiding this comment

PVince81 Dec 22, 2017

Choose a reason for hiding this comment

VicDeo Dec 26, 2017 • edited Loading

Choose a reason for hiding this comment

VicDeo commented Dec 26, 2017

PVince81 Jan 8, 2018

Choose a reason for hiding this comment

PVince81 Jan 8, 2018

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

VicDeo commented Jan 8, 2018

VicDeo commented Jan 8, 2018

PVince81 commented Jan 8, 2018

PVince81 commented Jan 8, 2018

VicDeo commented Jan 8, 2018

lock bot commented Aug 1, 2019

VicDeo commented Dec 15, 2017 •

edited

Loading

codecov bot commented Dec 20, 2017 •

edited

Loading

VicDeo Dec 26, 2017 •

edited

Loading