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

Add dedicated API for apps' bootstrapping process #20865

Merged
merged 6 commits into from
Jun 17, 2020

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented May 7, 2020

Fixes #20573

Possible registrations (ticket=implemented there, rest follows in other PRs)

Possible ideas for the register or boot context

  • Version of Nextcloud (e.g. to only call registration on things that are really available)
  • CLI or web request flag

@ChristophWurst ChristophWurst added this to the Nextcloud 20 milestone May 7, 2020
@ChristophWurst ChristophWurst requested a review from rullzer May 7, 2020 19:13
@ChristophWurst ChristophWurst marked this pull request as draft May 7, 2020 19:13
@ChristophWurst
Copy link
Member Author

apps/comments/lib/AppInfo/Application.php is the new show case example for the new registration style 🕺

@ChristophWurst ChristophWurst force-pushed the enhancement/app-bootstrapping branch 2 times, most recently from ee55e01 to 8c54e4a Compare June 8, 2020 17:31
@juliusknorr juliusknorr mentioned this pull request Jun 9, 2020
7 tasks
@ChristophWurst ChristophWurst force-pushed the enhancement/app-bootstrapping branch from 8587cc3 to 40153ca Compare June 9, 2020 18:02
}

/**
* Now that all register methods have been called, we can delegate the registrations
Copy link
Member Author

Choose a reason for hiding this comment

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

we don't have to do this here. we could actually either wait until right before the first boot method invocation or when the individual services are used

@rullzer
Copy link
Member

rullzer commented Jun 15, 2020

I like it.
I guess we should merge this sooner rather than later. I'm still not 100% sure how to handle disabled apps for users.

But I gues we should start to do that differently anyways. And revamp that. So that it actually works as expected.

@ChristophWurst ChristophWurst force-pushed the enhancement/app-bootstrapping branch 3 times, most recently from 285837e to 1359f2c Compare June 16, 2020 12:17
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 16, 2020
@ChristophWurst ChristophWurst marked this pull request as ready for review June 16, 2020 12:35
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

🚀 lets do this!

@ChristophWurst ChristophWurst requested a review from blizzz June 16, 2020 12:55
@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Jun 16, 2020
@kesselb
Copy link
Contributor

kesselb commented Jun 16, 2020

Looks good to me 🚀

  • Commands

Good idea. I have a suggestion for the command implementation: I looked at https://symfony.com/doc/current/console/lazy_commands.html to make occ faster. Eventually loading the apps is the expensive step and not to create all those command objects itself but it's still a good thing to do (because if one calls occ maintenance:repair we only need the RepairCommand object).

Here is a POC https://github.com/nextcloud/server/compare/enh/occ-lazy-load?expand=1. The basic requirement is that the name of the command is known before creating the object. Please consider to make the command name a required parameter for the command registration. That makes migration to lazy commands much easier and ensures that apps using the new way to register a command do it right.

@ChristophWurst
Copy link
Member Author

Makes sense, @kesselb. What I would also like to experiment with is proxy services instead of getting the real thing through constructor injection. Because right now you always build this huge tree of instances, when you'll most likely only every use some branches of that. Pimple, however, does not support it right now.

The commands are only slowing down occ operations, right? I hope we don't load them for web requests.

Looks good to me rocket

Mind pressing the approve button? :P

@@ -3,12 +3,9 @@
declare(strict_types=1);
Copy link
Member

Choose a reason for hiding this comment

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

Gotta love this "git rename" ....

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.

Looks good, didn't check to deeply nor transition over an app. But let's do it early.

@kesselb
Copy link
Contributor

kesselb commented Jun 16, 2020

I would like to withdraw my suggestion for commands. It's good to have all the commands definitions in appinfo.xml. If we add a attribute like <command name="mail:account:diagnose">OCA\Mail\Command\DiagnoseAccount</command> we are able to load only the required app for a occ command similar to what the router does with routes.php.

The commands are only slowing down occ operations, right?

Yes

I hope we don't load them for web requests.

No

Actually creating the command instances is not the expensive task. Loading all apps is the expensive one.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Organizing the app bootstrapping code
5 participants