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

DI Container improvements #2043

Closed
rullzer opened this issue Nov 8, 2016 · 8 comments
Closed

DI Container improvements #2043

rullzer opened this issue Nov 8, 2016 · 8 comments
Labels
1. to develop Accepted and waiting to be taken care of enhancement spec

Comments

@rullzer
Copy link
Member

rullzer commented Nov 8, 2016

The AppFramework has a DI system as well. However this system has some issues.

  1. The DIContainer (https://github.com/nextcloud/server/blob/master/lib/private/AppFramework/DependencyInjection/DIContainer.php) is the basic container for each AppContainer. This means that for each app we load we initialize a new DIContainer. This quickly leads to an explosion of registerService calls etc. While 1 backend container would suffice. (My dev setup does over 2k registerService calls).

  2. The ServerContainer does not extend the DIContainer. This means that we can't do fancy automatic DI there. And have to register everything manually.

A possible solution I see is the following:

There is 1 Container-Containter. This container only holds containers and the part of the namespace they are responsible for. The ServerContainer would register itself there just like any AppContainer would.

This would mean extending the query function. First check your own container. And if it is not there ask the Container-Container. The container container will then pic the right container to query.

This means that services will only have to be registered once in a container.

Comments/Input is welcome: CC: @BernhardPosselt @LukasReschke @icewind1991 @nickvergessen @MorrisJobke @ChristophWurst

@rullzer rullzer added enhancement 1. to develop Accepted and waiting to be taken care of spec labels Nov 8, 2016
@BernhardPosselt
Copy link
Member

The system should work in a way that OCP\ prefixed things are resolved from the server container unless overwritten (re-registered) in an app container

@BernhardPosselt
Copy link
Member

@rullzer maybe it would be a good idea if you could put some work into summarizing prior art, as in: take a look at existing containers in different languages and summarize how they work. Apart from that someone at ownCloud is working on integrating Symfony DI fyi.

@rullzer
Copy link
Member Author

rullzer commented Nov 8, 2016

The system should work in a way that OCP\ prefixed things are resolved from the server container unless overwritten (re-registered) in an app container

Yes exactly that should happen

@rullzer maybe it would be a good idea if you could put some work into summarizing prior art, as in: take a look at existing containers in different languages and summarize how they work.

Yeah time time time ;)

Apart from that someone at ownCloud is working on integrating Symfony DI fyi.

Yeah I saw that. I don't have a real preference.

@rullzer
Copy link
Member Author

rullzer commented Nov 8, 2016

Actually I think it might make more sense to do this right with separation of concerns. Then we can in a later stage always switch to symfony DI if we think that is worth it. For now I don't see a big advantage yet.

@BernhardPosselt
Copy link
Member

Right, things work well right now and apart from moving stable and very infrequently touched code outside of our maintenance scope I don't think symfony DI doesnt really add anything of value. We should however keep it in mind when refactoring this so it is easy to switch to it.

@rullzer
Copy link
Member Author

rullzer commented Nov 8, 2016

Yes and I think if we pull the current stuff apart using any framework becomes already easier.

But because we have the resolving part etc already in place I don't either see a hugely added benefit of switching to symfony.

@rullzer
Copy link
Member Author

rullzer commented Mar 17, 2017

Ok after thinking about it and discussing more I think the following approach will be cleaner

  1. Move over interface registrations to the ServerContainer as alias.
  2. Extend the query method of the DI Container
  • First check own container
  • Then check ServerContainer

This just moves things around and does not change any behavior.

rullzer added a commit that referenced this issue Mar 17, 2017
To align with #2043 (comment)
This would mean that AppContainers only hold the AppSpecific services

Signed-off-by: Roeland Jago Douma <[email protected]>
rullzer added a commit that referenced this issue Mar 19, 2017
To align with #2043 (comment)
This would mean that AppContainers only hold the AppSpecific services

Signed-off-by: Roeland Jago Douma <[email protected]>
rullzer added a commit that referenced this issue Mar 20, 2017
To align with #2043 (comment)
This would mean that AppContainers only hold the AppSpecific services

Signed-off-by: Roeland Jago Douma <[email protected]>
rullzer added a commit that referenced this issue Mar 21, 2017
To align with #2043 (comment)
This would mean that AppContainers only hold the AppSpecific services

Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer
Copy link
Member Author

rullzer commented Jul 23, 2017

Basically done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of enhancement spec
Projects
None yet
Development

No branches or pull requests

3 participants