-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Re-enable apps that got automatically disabled while updating #8380
Conversation
We should make this a separate step - see #5539 Also the app management should be unified, because now it is distributed across OC_App, IAppManager and Installer - see #5833 and #4453 |
lib/private/legacy/app.php
Outdated
@@ -154,6 +155,7 @@ public static function loadApp($app) { | |||
if (!\OC::$server->getAppManager()->isShipped($app)) { | |||
// Only disable apps which are not shipped | |||
self::disable($app); | |||
self::$autoDisabledApps[] = $app; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get this :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this is for the update case, which disabled e.g. the files_accesscontrol app.
Steps are:
- Upload the new code of the server
- Open Nextcloud (shows the updater)
- Start the update (loads all apps, and disables files_accesscontrol, because the storagewrapper is not matching the method signature anymore (was changed between 12 and 13))
- Updater looks for updates of enabled apps (and skips files_accesscontrol, because it is not enabled)
With this patch, in step 4 it also checks for updates of apps which were disabled while calling the updater.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... it is accessed from the outside via \OC_App::$autoDisabledApps
🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if we agree on the logic, we can wrap this in beautiful methods and public interfaces 💃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it 👍
Signed-off-by: Joas Schilling <coding@schilljs.com>
00835f8
to
4b49f81
Compare
Codecov Report
@@ Coverage Diff @@
## master #8380 +/- ##
=============================================
+ Coverage 31.7% 51.97% +20.26%
- Complexity 26017 26018 +1
=============================================
Files 1660 1660
Lines 96167 96172 +5
Branches 1290 1290
=============================================
+ Hits 30490 49982 +19492
+ Misses 65677 46190 -19487
|
Rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code makes sense and checks for the apps that failed hard. 👍
@rullzer Mind to review? |
This seems like a quite simple approach to work against #8293
But maybe I'm overseeing something.
Also I noticed, that automatic disable is already running the uninstall repairsteps. I once had the idea to use the uninstall step to clean up the database of my apps.
But that basically means when the announcement app e.g. gets automatically disabled all announcements and comments would be gone.
Not sure if that sounds intended.