Allow IBootstrap::boot to have services injected directly #26723
Conversation
Signed-off-by: Christoph Wurst <[email protected]>
Signed-off-by: Christoph Wurst <[email protected]>
| $injector = new FunctionInjector($application->getContainer(), [ | ||
| IBootContext::class => $context, | ||
| ]); | ||
| $injector->injectFn([$application, 'boot']); |
There was a problem hiding this comment.
reported that Psalm chokes on this at vimeo/psalm#5667
|
@juliushaertl @miaulalala @MorrisJobke @nickvergessen @rullzer what do you think? Yay or nay? After a few days of thinking I still consider this a step forward. The uglyness of this (duck typing) mostly happens under the hood. From a developer perspective this doesn't have any noticeable downside. Also from a performance perspective I don't really have concerns as php's reflection is very fast. |
| * @param IBootContext $context | ||
| * | ||
| * @since 20.0.0 | ||
| */ |
There was a problem hiding this comment.
Should be fine to just put this in the dev manual, right? At least that is fine for me.
Yay. I like it and it is a better flow from the developers perspective, because less of our specific functions and DI concepts are needed. It's just the same concept as with the constructor and I like it 👍 |
|
Yeah I find this acceptable as well. |
|
So generally fine as well. Another thought I had recently: We could also have made the boot() function return a string of public function names to call with injection on this $application object. Would keep the boot function a bit more discoverable as it's still in the interface, but not sure if it is really better codewise. |
Not ideal either as that calls for typos and doesn't feel idiomatic at all. |
|
Since the annotated |
I heard https://github.com/ChristophWurst/nextcloud_composer can help detecting that ;) |
|
Status? :) |
This would/will be great but it needs a bit of work. I can finish when I get some time to do so. |
ChristophWurst
added
0. Needs triage
I might be doing something uncontroversial that is totally against my very own principals but I think I found a legit use case for duck typing with php.
Currently when you implement your
\OCP\AppFramework\Bootstrap\IBootstrap::bootyou are likely to need stuff from the DI container. As we all know service locators are bad, so we don't query the things but use the\OCP\AppFramework\Bootstrap\IBootContext::injectFnto indirectly call another method and some magic injects the parameters of the method/function. So far, so good, but we can do better than this.What if you can add arbitrary services to your
bootand type hint them, and Nextcloud will inject them for you? No more explicitinjectFn, yay.This is nice from an app developer perspective. The downside is that the design gets a bit impure. We are breaking the Liskov substitution principle and the application classes are not interchangeable anymore. But frankly that is not really what they are meant for, especially the boot method, so I would accept this.
Another downside is that the documentation of the method isn't as nice anymore as there is no docblock for the method but just a one-liner. That means no
@since, no parameter description and so on. This sucks, but documentation can help and we can link to those docs from there.Phpunit and psalm also seem to be okay with this.
This same concept can be reused for other similar code like occ commands (when we finally have our own API, background jobs, tasks, and so on.
Ref https://psalm.dev/r/a9d551181c some experiments with the duck typing and Psalm
Edit: forgot to mention that the idea came from https://laravel.com/docs/8.x/providers#boot-method-dependency-injection. Taking a look at their approach we might drop the
bootmethod from the interface call it after amethod_existscheck on the application instance. That way apps can also omit the method if there is no need for it.