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

Refactor app loading #36591

Merged
merged 8 commits into from
Mar 20, 2023
Merged

Refactor app loading #36591

merged 8 commits into from
Mar 20, 2023

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Feb 7, 2023 •
edited by zak39
Loading

TODO

  • Move loadApp to AppManager
  • Move away from server::get as much as possible
  • Move needed methods to AppManager to avoid calling OC_App

Checklist

@come-nc come-nc self-assigned this Feb 7, 2023
@szaimen szaimen added this to the Nextcloud 27 milestone Feb 8, 2023
@come-nc come-nc requested review from juliusknorr, ChristophWurst, a team, ArtificialOwl, icewind1991 and blizzz and removed request for a team February 8, 2023 15:23
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 8, 2023
@come-nc come-nc marked this pull request as ready for review February 8, 2023 15:23
@come-nc
Copy link
Contributor Author

come-nc commented Feb 8, 2023

This is not bringing the expected performance boost, but it does cleanup things and move away from legacy OC_App, so I think it’s still worth to be merged.

@ChristophWurst
Copy link
Member

This is not bringing the expected performance boost, but it does cleanup things and move away from legacy OC_App, so I think it’s still worth to be merged.

Clean code > fast code 😉

ChristophWurst
ChristophWurst approved these changes Feb 8, 2023
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.

Looks good

@ChristophWurst
Copy link
Member

Nit: clean up the git history. Either in the three steps described in the PR or one atomic change. Future bisectors will thank you :)

@come-nc come-nc force-pushed the enh/refactor-app-loading branch 2 times, most recently from 99d9f22 to ecb3c8b Compare February 9, 2023 12:07
@come-nc
Copy link
Contributor Author

come-nc commented Feb 9, 2023

Squashed and rebased on master

@juliusknorr
Copy link
Member

Conflicts ;)

@come-nc
Copy link
Contributor Author

come-nc commented Feb 13, 2023

Conflicts ;)

😞 These are not easy to fix, the loadApp method modifications from @icewind1991 about event logging need to be transposed in the appmanager instead

@szaimen
Copy link
Contributor

szaimen commented Mar 16, 2023

@come-nc would you mind having another look at this and fixing the conflicts and tests? Thanks a lot! :)

@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 16, 2023
@ChristophWurst ChristophWurst marked this pull request as draft March 16, 2023 11:20
@come-nc come-nc force-pushed the enh/refactor-app-loading branch from ecb3c8b to 13c71ed Compare March 20, 2023 09:24
@come-nc come-nc marked this pull request as ready for review March 20, 2023 09:43
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 20, 2023
@come-nc come-nc merged commit a3ca6be into master Mar 20, 2023
@come-nc come-nc deleted the enh/refactor-app-loading branch March 20, 2023 14:34
nickvergessen
nickvergessen requested changes Mar 26, 2023
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Please document in #37039

lib/public/App/IAppManager.php

/**
* Check if an app is of a specific type
* @since 26.0.0
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
* @since 26.0.0
* @since 27.0.0

lib/public/App/IAppManager.php
* exists.
*
* if $types is set to non-empty array, only apps of those types will be loaded
* @since 26.0.0
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
* @since 26.0.0
* @since 27.0.0

lib/public/App/IAppManager.php
/**
* Check if an app is loaded
* @param string $app app id
* @since 26.0.0
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
* @since 26.0.0
* @since 27.0.0

lib/public/App/IAppManager.php
/**
* Load an app, if not already loaded
* @param string $app app id
* @since 26.0.0
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
* @since 26.0.0
* @since 27.0.0

lib/private/legacy/OC_App.php
/**
* check if an app is of a specific type
*
* @param string $app
* @param array $types
* @return bool
* @deprecated 26.0.0 use IAppManager::isType
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
* @deprecated 26.0.0 use IAppManager::isType
* @deprecated 27.0.0 use IAppManager::isType

lib/private/legacy/OC_App.php
}

/**
* load a single app
*
* @param string $app
* @throws Exception
* @deprecated 26.0.0 use IAppManager::loadApp
Copy link
Member

Choose a reason for hiding this comment

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

27 (and for the others too)

@come-nc come-nc mentioned this pull request Mar 27, 2023
Merged
4 tasks
@come-nc come-nc mentioned this pull request May 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen requested changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@juliusknorr juliusknorr juliusknorr approved these changes

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

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

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

Assignees

@come-nc come-nc

Labels
3. to review Waiting for reviews performance 🚀
Projects
None yet
Milestone
Nextcloud 27
Development

Successfully merging this pull request may close these issues.
5 participants
@come-nc @ChristophWurst @juliusknorr @szaimen @nickvergessen