Repository navigation
Organizing the app bootstrapping code #20573
Description
How to use GitHub
- Please use the 👍 reaction to show that you are interested into the same feature.
- Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
- Subscribe to receive notifications on status change and new comments.
Is your feature request related to a problem? Please describe.
Many apps have to run some code before each request. This could necessary to register classes for the DI container, register middlewares, set up notifications, listen to events or bootstrap any other application logic. Right now apps can do this either via appinfo/app.php (procedural, always loaded) or lib/AppInfo/Application.php (OOP, not always loaded). Often the app.php creates a new instance of the Application, so that code is run.
This approach does not appear very organized. It also has the problem that you have to make a few assumptions about what other services/components/data is there when your app is activated. This then also highly depends on the loading order of apps. Like authentication apps are loaded earlier just because they register the user back-ends.
Describe the solution you'd like
Conceptually, there are (at least) two main steps in a typical application start code: registering services and actively performing some operations.
As discussed with @rullzer in detail we could extend the apps' Application classes, so they have two hooks to do their thing, while giving our server component full control over when this happens.
Register
At register, the app code can initialize the DI container or register middlewares, notification handlers, event listeners, two-factor providers and so on. The app must not assume any other app nor the server to be initialized.
Bootstrap
At bootstrap, all apps and server have registered their services, thus they can be assumed to be there. Then the app can run the necessary logic, like initializing some state.
Implementation
What we came up with is that one simple approach would be to add register and boot as public methods with empty bodies to \OCP\AppFramework\App. Apps can then overwrite the two lifecycle hooks.
In an early state of the Nextcloud process we can load all apps. Then reflect their Application class and determine if they overwrite register. In that case we know that the app adjusted for the new mechanism and we call that. Then, whenever the \OC_App::loadApps is called we can create the remaining Application classes and invoke bootstrap. This means application classes that still use the constructor to run stuff will work, but so will the adapted ones.
For the early registration we can't use the actual services like a notification manager, as it may depend on something that is not loaded at that stage. But we can, instead, just provide a tiny class that acts as a registry, where apps can only register their services (ideally with a class name to be as lazy as possible). As the set of registries will change over time, we'll pass a context object to register, which has getters for the registries. This has the benefit, that the API for this step in the application lifecycle is limited to what apps are supposed to use.
For bootstrap a context object will also make sense, e.g. to access the app and server container to query some services.
Describe alternatives you've considered
- Have very specific interfaces the
Applicationclass could implement, e.g. one for notifications, one for event listeners, .... While it gives fine grained control, the benefit is little, as we'd have to call each of these registration methods in a fixed order anyway.
Additional context
This concept was copied and adapted from https://laravel.com/docs/5.5/providers.
Activity
ChristophWurst commentedon Apr 21, 2020
@blizzz @juliushaertl @kesselb @nickvergessen what do you think? Any objections? Are we missing something?
[-]Oranizing the app bootstrapping code[/-][+]Organizing the app bootstrapping code[/+]ChristophWurst commentedon Apr 21, 2020
@MorrisJobke forgot to mention you. I think you might like this as well :)
nickvergessen commentedon Apr 21, 2020
The thing is register should be as light as possible either.
E.g. makes less sense to register your Notifier and register potentially tons of services, etc for it, when in the end no app interacts with them.
😁 https://github.com/nextcloud/spreed/blob/master/lib/AppInfo/Application.php#L80
I'm not sure, maybe instead we add an interface which the app makes their Application implement instead. Then you know it is using the new system.
ChristophWurst commentedon Apr 21, 2020
We thought about this:
So whatever is registered should be as lazy as possible. Like what we already do with notifications and even listeners, you just pass in a string (FQCN). Only when that is really used, it's loaded. Then the registration does only consume a few bytes for the string until it gets used and the class is loaded.
ChristophWurst commentedon Apr 21, 2020
Ah nice. https://en.wikipedia.org/wiki/Fragile_base_class.
Yeah we can do that as well. The idea with the reflect was just that we don't need this new type. But this is what we also originally had in mind :)
blizzz commentedon Apr 21, 2020
It is also less complex (and probably more performant) to check for an interface implementation. Also more transparent, less magic.
blizzz commentedon Apr 21, 2020
Not necessary. The base class has already a getter for the app container, which again can fetch the server container. To be future prove it might be handy, on the other side, those offer already you all that you need.
So to prevent the app from accessing anything via global OC::$server we would need to keep track of the app handling like cycle.
With regard to the app loading priority, the register stage would happen universally for all apps as late as possible, as early as necessary, while the bootstrap will follow current order of app types?
ChristophWurst commentedon Apr 21, 2020
Yep. The idea with the context object was that we can change the provided getters over time without having to break the interface of that method that is implemented in app code.
Could you elaborate on that? I'm not sure I can follow. I don't think we can hard prevent apps from doing stupid things. And
OC::$serveris one of them IMO 😉. We have that in some apps, yes, but this typically is just there because of bad design.Yes, that is correct. This should ensure that old code continues to work.
blizzz commentedon Apr 21, 2020
No, true, but we can limit them. Like $server->query() to throw Exceptions if they attempt to call it during register-phase. Whether it is worthwhile is a different question, and whether we want to try to lock down things further. A different approach would be to have the register-stage solely through info.xml.
ChristophWurst commentedon May 8, 2020
PoC is at #20865
ChristophWurst commentedon Jun 4, 2020
Personally, I feel like we should do more in php. Not only does it allow you to run some logic before you register something, it's also helping with the dev experience as your IDE can suggest and check types. With XML you always have to first dig into the schema to know how to use something.
This is also related to the attributes in php8. Once we have this, it will be a much better dev experience for the current
@NoAdminRequiredand similar phpdoc annotations.From a performance PoV I would also argue it is faster to append a string to an array then it is to load, parse and convert XML into a php data structure.