Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Migrate away from OC_App and toward IAppManager. #44025

Merged
merged 8 commits into from
Apr 22, 2024
Merged

Migrate away from OC_App and toward IAppManager. #44025

merged 8 commits into from
Apr 22, 2024

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Mar 6, 2024 •
edited
Loading

Summary

Migrate away from OC_App and toward IAppManager.

Checklist

@come-nc come-nc added the 2. developing Work in progress label Mar 6, 2024
@come-nc come-nc self-assigned this Mar 6, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 6, 2024
View reviewed changes
core/Command/App/Remove.php Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 6, 2024
View reviewed changes
lib/private/App/AppManager.php
/**
* @return string[] $appId => $enabled
*/
private function getInstalledAppsValues(): array {
if (!$this->installedAppsCache) {
$values = $this->appConfig->getValues(false, 'enabled');
$values = $this->getAppConfig()->getValues(false, 'enabled');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\AppConfig::getValues has been marked as deprecated
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 6, 2024
View reviewed changes
lib/private/App/AppManager.php
@@ -237,7 +249,7 @@
private function getAppTypes(string $app): array {
//load the cache
if (count($this->appTypes) === 0) {
$this->appTypes = $this->appConfig->getValues(false, 'types') ?: [];
$this->appTypes = $this->getAppConfig()->getValues(false, 'types') ?: [];

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\AppConfig::getValues has been marked as deprecated
@come-nc come-nc changed the title Fix/remove oc app calls Migrate away from OC_App and toward IAppManager. Mar 6, 2024
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 6, 2024
@come-nc come-nc marked this pull request as ready for review March 6, 2024 21:45
@come-nc come-nc requested review from a team, ArtificialOwl, icewind1991, Fenn-CS, nickvergessen and ChristophWurst and removed request for a team March 7, 2024 11:45
ChristophWurst
ChristophWurst approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

To avoid some kaboom we might want to merge this after the 29 branch-off

@come-nc come-nc added this to the Nextcloud 30 milestone Mar 7, 2024
@come-nc
fix: Migrate away from OC_App toward the IAppManager
644036a 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
fix: use DI to build main Application class in console.php
8c5f323 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
fix: Make sure that AppManager can be built before Nextcloud is insta…
0e7bac7 
…lled

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
fix: Migrate a few more classes away from OC_App::getAppPath
733a818 
Also fixed AppTest

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
fix: Fix tests following OC_App migrations to IAppManager
a0be3ff 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
fix: Fix RouterTest by adding IAppManager
1b5a0c3 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Copy link
Contributor Author

come-nc commented Apr 22, 2024

Rebased and fixed conflicts

github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 22, 2024
View reviewed changes
lib/private/App/AppManager.php
@@ -541,7 +553,7 @@
}

$this->installedAppsCache[$appId] = 'yes';
$this->appConfig->setValue($appId, 'enabled', 'yes');
$this->getAppConfig()->setValue($appId, 'enabled', 'yes');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\AppConfig::setValue has been marked as deprecated
lib/private/App/AppManager.php
@@ -595,7 +607,7 @@
}, $groups);

$this->installedAppsCache[$appId] = json_encode($groupIds);
$this->appConfig->setValue($appId, 'enabled', json_encode($groupIds));
$this->getAppConfig()->setValue($appId, 'enabled', json_encode($groupIds));

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\AppConfig::setValue has been marked as deprecated
lib/private/App/AppManager.php
@@ -616,15 +628,15 @@
}

if ($automaticDisabled) {
$previousSetting = $this->appConfig->getValue($appId, 'enabled', 'yes');
$previousSetting = $this->getAppConfig()->getValue($appId, 'enabled', 'yes');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\AppConfig::getValue has been marked as deprecated
lib/private/App/AppManager.php
if ($previousSetting !== 'yes' && $previousSetting !== 'no') {
$previousSetting = json_decode($previousSetting, true);
}
$this->autoDisabledApps[$appId] = $previousSetting;
}

unset($this->installedAppsCache[$appId]);
$this->appConfig->setValue($appId, 'enabled', 'no');
$this->getAppConfig()->setValue($appId, 'enabled', 'no');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\AppConfig::setValue has been marked as deprecated
lib/private/App/AppManager.php
@@ -689,7 +701,7 @@
$apps = $this->getInstalledApps();
foreach ($apps as $appId) {
$appInfo = $this->getAppInfo($appId);
$appDbVersion = $this->appConfig->getValue($appId, 'installed_version');
$appDbVersion = $this->getAppConfig()->getValue($appId, 'installed_version');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\AppConfig::getValue has been marked as deprecated
@come-nc
fix: Revert to OC_App in places where we need all apps even disabled …
61621ee 
…ones

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Copy link
Contributor Author

come-nc commented Apr 22, 2024

@nickvergessen Reverted both calls. We’ll need later to add a replacement for getAllApps.

But the maintenance:install command segfaults since today rebase. I can reproduce locally but have no idea what happens.
I tried reverting the IURLGenerator injection in the appmanager but it does not change anything.

@nickvergessen
Copy link
Member

nickvergessen commented Apr 22, 2024 •
edited
Loading

but have no idea what happens.

Circular dependencies in 99% of the cases

@come-nc
fix: Lazy load IURLGenerator from AppManager to avoid installation crash
ac1d868 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Copy link
Contributor Author

come-nc commented Apr 22, 2024

but have no idea what happens.

Circular dependencies in 99% of the cases

Yes, it was urlgenerator injection causing trouble as suspected. At first I thought it was not because I had missed one place it was pulled from.
Fixed now, should be good.

@come-nc come-nc merged commit 37c89f4 into master Apr 22, 2024
157 checks passed
@come-nc come-nc deleted the fix/remove-oc-app-calls branch April 22, 2024 14:41
@blizzz blizzz mentioned this pull request Jul 24, 2024
Merged
@come-nc come-nc mentioned this pull request Sep 12, 2024
Merged
4 tasks
@joshtrichards
Copy link
Member

Related to open issue: #8505 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@icewind1991 icewind1991 icewind1991 approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl was automatically assigned from nextcloud/server-backend

@Fenn-CS Fenn-CS Awaiting requested review from Fenn-CS Fenn-CS was automatically assigned from nextcloud/server-backend

Assignees

@come-nc come-nc

Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.
6 participants
@come-nc @nickvergessen @joshtrichards @icewind1991 @ChristophWurst @skjnldsv