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

Refactor OC\Server::getAppManager #40113

Closed

Conversation

summersab
Copy link
Contributor

This PR refactors the deprecated method OC\Server::getAppManager and replaces it with OC\Server::get(\OCP\App\IAppManager::class) throughout the entire NC codebase (excluding ./apps and ./3rdparty).

Additionally, where necessary, the \OCP\App\IAppManager class is imported via the use directive.

@summersab summersab force-pushed the refactor/OC-Server-getAppManager branch from 4fec546 to 642643a Compare August 30, 2023 01:29
@summersab summersab force-pushed the refactor/OC-Server-getAppManager branch from 642643a to 522bc4d Compare August 30, 2023 02:27
@solracsf solracsf added the 3. to review Waiting for reviews label Aug 30, 2023
@solracsf solracsf added this to the Nextcloud 28 milestone Aug 30, 2023
core/register_command.php Show resolved Hide resolved
core/register_command.php Show resolved Hide resolved
core/register_command.php Show resolved Hide resolved
core/register_command.php Show resolved Hide resolved
@@ -72,12 +74,12 @@


if (\OC::$server->getConfig()->getSystemValue('installed', false)) {
$application->add(new OC\Core\Command\App\Disable(\OC::$server->getAppManager()));
$application->add(new OC\Core\Command\App\Enable(\OC::$server->getAppManager(), \OC::$server->getGroupManager()));
$application->add(new OC\Core\Command\App\Disable(\OC::$server->get(IAppManager::class)));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$application->add(new OC\Core\Command\App\Disable(\OC::$server->get(IAppManager::class)));
$application->add(\OCP\Server::get(\OC\Core\Command\App\Disable::class));

Copy link
Member

Choose a reason for hiding this comment

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

Same for all others of course.

Copy link
Contributor Author

@summersab summersab Aug 30, 2023

Choose a reason for hiding this comment

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

I can add these changes to this PR if you would like, but should it be in its own PR? Either way works for me.

Shouldn't it be this (or something similar), though?

$application->add(\OCP\Server::get(\OC\Core\Command\App\Disable::class)(\OC::$server->get(IAppManager::class)));

Copy link
Member

@nickvergessen nickvergessen Aug 30, 2023

Choose a reason for hiding this comment

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

Well see this urgent comment before continuing the work: #40083 (comment)

Shouldn't it be this (or something similar), though?

The code you provided does not even parse in PHP. No, my suggested code basically removes all "manual injection of dependencies" and replaces it by "automatically injecting the actual command" which then on it's own will get all it's dependencies, instead of us listing hundreds of get() parameters here.

It might not work for all commands (some seem to get non-DI-able objects like new View() as a parameter), but should work for most of them.

Comment on lines +52 to +53
use OCP\App\IAppManager;
use Psr\Log\LoggerInterface;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
use OCP\App\IAppManager;
use Psr\Log\LoggerInterface;
use OCP\App\IAppManager;
use Psr\Log\LoggerInterface;

@nickvergessen nickvergessen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 30, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv
Copy link
Member

Too much conflicts, closing until this is done fresh again

@skjnldsv skjnldsv closed this May 30, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants