Skip to content

Comments

Chore: Cleanup OC\Updater and OC\Installer classes#53895

Merged
provokateurin merged 20 commits intomasterfrom
fix/cleanup-updater-class
Aug 19, 2025
Merged

Chore: Cleanup OC\Updater and OC\Installer classes#53895
provokateurin merged 20 commits intomasterfrom
fix/cleanup-updater-class

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented Jul 10, 2025 •
edited
Loading

Summary

TODO

  • Remove all OC_App calls from Updater
  • Replace OC_App->enable with IAppManager::enable directly if possible
  • Move functions specific to app update to the updater directly
  • Replace emit with events or another system Not in this PR
  • Check if it’s needed to put files first in return of getEnabledApps
  • Deprecate appinfo/install.php (remove?)

Checklist

@come-nc come-nc added this to the Nextcloud 32 milestone Jul 10, 2025
@come-nc come-nc self-assigned this Jul 10, 2025
@come-nc
Copy link
Contributor Author

come-nc commented Jul 10, 2025 •
edited
Loading

The split between OC\Installer, OC\Updater and OC\App\AppManager is weird.
Theory would be Installer manages files and appstore, updater manager upgrade process and AppManager manages what’s stored in DB for apps, but there’s too many overlap and inter-dependencies.

As OC\Installer uses OC\App\AppManager but not the other way around, I’m gonna try:

  1. Inject the app manager as a property of the installer
  2. Move to the app manager all I can from OC_App
  3. Use a common method to run ugrade steps after install, ugrade, or install of a shipped app (currently 3 methods with similar code)
  4. Move stuff that needs both installer and app manager either to the installer or the updater

@come-nc come-nc force-pushed the fix/cleanup-updater-class branch from 46a8e3a to e77c927 Compare July 10, 2025 15:20
@come-nc come-nc changed the title Chore: Cleanup OC\Updater class Chore: Cleanup OC\Updater and OC\Installer classes Jul 10, 2025
@come-nc come-nc force-pushed the fix/cleanup-updater-class branch from fe79854 to 2ca8a18 Compare July 29, 2025 12:32
@come-nc come-nc force-pushed the fix/cleanup-updater-class branch from 1215976 to 1ea23cb Compare August 18, 2025 10:10
come-nc added 9 commits August 18, 2025 16:13
@come-nc
fix: Cleanup OC_App uses in Updater class
6aa675c 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Move OC_App::updateApp to IAppManager::upgradeApp
fd17555 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Add IAppManager property in Installer
42c980e 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Remove references to obsolete field ocsid in apps info.xml
f5fac64 
It’s not even allowed by our xsd schema.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Inline private method OC_App::setupLiveMigrations
4103898 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Adapt InstallerTest to Installer new construct parameters
f551b91 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Remove check for appinfo/database.xml
cb8d960 
It has been unsupported since Nextcloud 22.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Set installShippedApps as non-static in Installer
25c2279 
Make code closer to the one of installApp, to be able to compare them
 and later merge them (in the shadows).

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Move isAppCompatible and shouldUpgrade to the app manager
4abfd48 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the fix/cleanup-updater-class branch from 30721ad to d77a28b Compare August 18, 2025 14:20
come-nc added 10 commits August 18, 2025 17:09
@come-nc
chore: Deduplicate common code between installApp and installShippedApp
61a87bc 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Replace last calls to OC_App::enable by IAppManager
3e01a42 
Also added a few missing deprecations

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Move getAppPath and getAppWebPath implementations into AppManager
e5606d7 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Inline OC_App::setupBackgroundJobs
0e01bc0 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore(tests): Use valid application names for tests
3cea218 
Dash is not allowed in appid, underscore is.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Get rid of AppLocator helper
b7c1594 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: Update autoloader
7978b61 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
fix: Deduplicate code by using DependencyAnalyzer in the AppManager
12edd2b 
Was a bit more complicated than expected because of a dependency loop,
the L10N factory uses the app manager, thus the AppManager cannot depend
on I10N directly or indirectly in its constructor.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore(tests): Do not mock DependencyAnalyzer in AppManagerTest
f5111c0 
Not ideal to have coupled tests like that but it’s the easiest path
 forward to make sure the tests still covers the same usecase and avoid
 code duplication.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
chore: update psalm baseline
8ccf87f 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the fix/cleanup-updater-class branch from d77a28b to 8ccf87f Compare August 18, 2025 15:09
@come-nc
fix: Log that using an appinfo/install.php file is deprecated
a257973 
appinfo/install.php is not part of the official documentation for
 application development but some apps are still using such a file.
 Log a message to deprecate this behavior, to be able to remove support
 for this later.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 18, 2025
@come-nc come-nc marked this pull request as ready for review August 18, 2025 15:29
@come-nc come-nc requested a review from a team as a code owner August 18, 2025 15:29
@come-nc come-nc requested review from ChristophWurst, icewind1991, provokateurin, salmart-dev and susnux and removed request for a team August 18, 2025 15:29
provokateurin
provokateurin reviewed Aug 19, 2025
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, really nice cleanup! 😍

susnux
susnux approved these changes Aug 19, 2025
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice cleanup!

@come-nc come-nc requested a review from provokateurin August 19, 2025 15:13
@provokateurin provokateurin merged commit 4edfef4 into master Aug 19, 2025
214 of 221 checks passed
@provokateurin provokateurin deleted the fix/cleanup-updater-class branch August 19, 2025 15:40
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@susnux susnux susnux approved these changes

@provokateurin provokateurin provokateurin approved these changes

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 is a code owner automatically assigned from nextcloud/server-backend

@salmart-dev salmart-dev Awaiting requested review from salmart-dev salmart-dev is a code owner automatically assigned from nextcloud/server-backend

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

Assignees

@come-nc come-nc

Labels

3. to review Waiting for reviews technical debt

Projects

None yet

Milestone

Nextcloud 32

Development

Successfully merging this pull request may close these issues.

4 participants

@come-nc @susnux @provokateurin @skjnldsv