fix(settings): Rework download/install/update/remove app handling and respect read-only app roots #51667
Conversation
provokateurin
requested review from
ArtificialOwl,
skjnldsv and
sorbaugh
and removed request for
a team
|
/backport to stable31 |
|
/backport to stable30 |
…pect read-only apps roots Signed-off-by: provokateurin <kate@provokateurin.de>
… it is read-only Signed-off-by: provokateurin <kate@provokateurin.de>
… respect read-only app roots Signed-off-by: provokateurin <kate@provokateurin.de>
provokateurin
force-pushed
the
fix/settings/read-only-apps-root
branch
from
5fd2fa7 to
f206c60
Compare
|
/backport to stable29 |
|
(One cypress failure is related!) |
| 'active' => $this->appManager->isEnabledForUser($app['id']), | ||
| 'needsDownload' => !$existsLocally, | ||
| 'canDownload' => $anyAppsRootWritable, | ||
| 'canUpdate' => $canUpdate, |
There was a problem hiding this comment.
This would remove the crucial part that allows updating apps into another folder when the current one is read-only.
At least in the past we allowed an app to be in multiple app directories and the code simply loaded the one with the highest number in appinfo.xml
There was a problem hiding this comment.
But unfortunately it's also the case, that some paths are hardcoded to apps/, so I'm not sure that this really worked as intended.
Like I said, app directories are very broken from what I had to dig through.
There was a problem hiding this comment.
But unfortunately it's also the case, that some paths are hardcoded to apps/
What? Can you provide links?
provokateurin
dismissed
sorbaugh’s stale review
Sorry @sorbaugh, I think this will need some considerations as this entire topic could easily break things if we are not careful.
| try { | ||
| $appPath = $appManager->getAppPath($appId); | ||
| } catch (AppPathNotFoundException) { | ||
| $appPath = OC_App::getInstallPath() . '/' . $appId; |
There was a problem hiding this comment.
OC_App::getInstallPath is only used in this class it seems, it should be moved into it and deprecated in OC_App. It should also have a better name.
| */ | ||
| private function getAppsForCategory($requestedCategory = ''): array { | ||
| $anyAppsRootWritable = false; | ||
| foreach (\OC::$APPSROOTS as $appsRoot) { |
There was a problem hiding this comment.
We should have a public getter somewhere for app roots instead of accessing an OC static var from an application.
Also, this logic of testing whether at least one app folder is writable could be in a method of the AppManager directly.
| $appsRootWritable = false; | ||
| $appRootPath = dirname($appPath); | ||
| foreach (\OC::$APPSROOTS as $appsRoot) { | ||
| if ($appsRoot['path'] === $appRootPath) { | ||
| $appsRootWritable = $appsRoot['writable'] ?? false; | ||
| } | ||
| } |
There was a problem hiding this comment.
This code is duplicated in your PR, please create a method for it.
Summary
Most of the logic was quite broken regarding read-only app roots, both on backend and frontend. In general app roots seem to be a feature that isn't used a lot (at least in production, maybe more in development), otherwise some of these issues would have come up way earlier (but for example git apps are ignored for updates, so most developers will never see it broken with multiple app roots).
I'm sure there are more bugs, as the code is just a mess and definitely hasn't been adjusted well in the past.
Checklist