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.

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

Migrate away from OC_App to IAppManager #47927

Merged
merged 3 commits into from
Sep 13, 2024
Merged

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Sep 12, 2024

Follow-up of #44025

Summary

Migrate most calls of OC_App::getAppPath, OC_App::cleanAppId, OC_App::getAllApps to calls on the AppManager instance. The later two were added to the OCP interface for the occasion.

Checklist

@come-nc come-nc added this to the Nextcloud 31 milestone Sep 12, 2024
@come-nc come-nc self-assigned this Sep 12, 2024
@come-nc come-nc force-pushed the fix/migrate-away-from-oc_app branch 2 times, most recently from 07141d5 to 2abf2d8 Compare September 13, 2024 07:24
…_App

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the fix/migrate-away-from-oc_app branch from 2abf2d8 to 7a16d01 Compare September 13, 2024 08:27
@come-nc come-nc marked this pull request as ready for review September 13, 2024 09:08
@come-nc come-nc requested review from susnux and provokateurin and removed request for nickvergessen, ChristophWurst, st3iny and miaulalala September 13, 2024 09:09
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 13, 2024
@come-nc come-nc requested review from a team, ArtificialOwl and Altahrim and removed request for a team September 13, 2024 09:09
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.

Nice cleanup!

tests/lib/App/AppManagerTest.php Show resolved Hide resolved
$apps = array_unique($apps);

return $apps;
return \OCP\Server::get(IAppManager::class)->getAllAppsInAppsFolders();
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also deprecate this in favor of \OCP\Support\Subscription\IRegistry::delegateGetSupportedApps ?

Copy link
Member

Choose a reason for hiding this comment

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

I would do the opposite 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why the opposite?

@susnux yeah we should. But I won’t do it in this PR to keep it mergeable. This kind of cleanup needs to be merged quickly or it conflicts fast.

Copy link
Member

Choose a reason for hiding this comment

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

Why the opposite?

From my perspective the subscription methods are "internal" and are used by (other) public components.
All the delegate*() functions are only called from "lib/private/ implementing public interfaces", settings app, support app or update checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe getSupportedApps can be moved to the AppManager then.

In the mean time, would one of you two approve this here PR? :-P

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.

🧹

@come-nc come-nc merged commit bcb4e78 into master Sep 13, 2024
178 checks passed
@come-nc come-nc deleted the fix/migrate-away-from-oc_app branch September 13, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants