Repository navigation
Organizing the app bootstrapping code聽#20573
Description
Activity
@blizzz @juliushaertl @kesselb @nickvergessen what do you think? Any objections? Are we missing something?
- changed the title
[-]Oranizing the app bootstrapping code[/-][+]Organizing the app bootstrapping code[/+]on Apr 21, 2020 @MorrisJobke forgot to mention you. I think you might like this as well :)
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.Then reflect their Application class and determine if they overwrite register.
馃榿 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.
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.We thought about this:
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).
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.
Reacted by Morris Jobke and Georg Ehrkegrin https://github.com/nextcloud/spreed/blob/master/lib/AppInfo/Application.php#L80
Ah nice. https://en.wikipedia.org/wiki/Fragile_base_class.
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.
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 :)
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.
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 :)
It is also less complex (and probably more performant) to check for an interface implementation. Also more transparent, less magic.
Reacted by Christoph Wurst and Georg EhrkeFor bootstrap a context object will also make sense, e.g. to access the app and server container to query some services.
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.
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.
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?
Not necessary.
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.
So to prevent the app from accessing anything via global OC::$server we would need to keep track of the app handling like cycle.
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.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?
Yes, that is correct. This should ensure that old code continues to work.
So to prevent the app from accessing anything via global OC::$server we would need to keep track of the app handling like cycle.
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.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.
Reacted by Christoph WurstPoC is at #20865
A different approach would be to have the register-stage solely through info.xml.
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.
How to use GitHub
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) orlib/AppInfo/Application.php(OOP, not always loaded). Often the app.php creates a new instance of theApplication, 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'
Applicationclasses, 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
registerandbootas 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
Applicationclass and determine if they overwriteregister. In that case we know that the app adjusted for the new mechanism and we call that. Then, whenever the\OC_App::loadAppsis called we can create the remainingApplicationclasses and invokebootstrap. 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
bootstrapa context object will also make sense, e.g. to access the app and server container to query some services.Describe alternatives you've considered
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.