API: Public vs Private services in core

[BernhardPosselt]

Through the use of $c->query('ServerContainer')->query('Name') you can virtually access any service in core that is defined in the server container, be it private or public. We also encourage app devs to do it in that way, which is bad.

Not only is it bad for maintaining a stable public API, but can also lead to obscure bugs (dev overrides core service for instance) and it is not as easy to develop apps as it could be. What I propose is that we should get rid of the ServerContainer parameter in the DIContainer class:

$this->registerParameter('ServerContainer', \OC::$server);

and replace it with a service for now that logs a deprecated error when the ServerContainer is accessed, like:

$this->registerService('ServerContainer', function ($c) {
    \OC::$server->getLogger()->warn('Accessing the servercontainer is deprecated and will be removed in ownCloud 9. Use type annotation to inject it into your classes')
    return \OC::$server;
});

The next step should be to get everything that should be public into that container, so for instance \OCP\IConfig should be registered in the following way:

$this->registerService('OCP\IConfig', function($c) {
    return \OC::$server->getAllConfig();
});

That way the app developer can let the container resolve the interface automatically with the help of #12829

This is nice because the app dev now does not need to know where to get the dependency from (e.g. IRequest is not in the server container but in the DIContainer which is really weird) but he can simply add the public interface type hint to his constructor like:

class AuthorController {
    public function __construct($AppName, \OCP\IRequest $request, \OCP\IConfig $config)
}

This will also be less to write, so i suppose devs that struggled with DI before and just accessed the global server container will be more likely to use proper DI

@georgehrke @DeepDiver1975 @butonic @PVince81 @MorrisJobke @nickvergessen @jbtbnl @karlitschek

[oparoz]

This doc needs to be updated as soon as this PR is merged.

[BernhardPosselt]

Changes addressed in #12830

[nickvergessen]

I dont really like duplication and this idea brings it back what we try to remove with allconfig e.g.

Unless we just remove the other service and use the interface service aswell in core.

Having "private" services just forces app devs too copy paste them and if the class is private that is copied aswell and then suffers from missing patches

[MorrisJobke]

@nickvergessen Sure we also need to move to this in the core (and we already use public namespaced versions of the instances already). I don't see a duplication problem here, but it just enhances the naming of the services.

[RobinMcCorkell]

Looks like this was fixed in #12830