[DI][DX][RFC] Implement a ServiceSubscriberTrait

[tgalopin]

Q	A
Bug report?	no
Feature request?	no
BC Break report?	no
RFC?	yes
Symfony version	3.4

While the Symfony DI component is a really awesome tool to help build high quality apps, I was thinking there could be ways to improve its DX in terms of autocompletion in IDEs.

When interacting with service locators, we are used to do things like $this->get('my_service'), or more recently $this->get(MyService::class). Without a Symfony plugin, IDEs are not able to autocomplete what comes after $this->get('my_service') as the type of the returned element is not known until runtime.

After some dicussions with @nicolas-grekas, we came up with the idea of a ServiceSubscriberTrait that could be used to improve DX on this level. Here is an example of usage (the class would be userland, the trait would be in Symfony):

https://gist.github.com/tgalopin/46e2be955762d6d68360dd455afeab70

The idea is to detect in getSubscribedServices all the current class methods and inject automatically the right services in the service locator using the typehints. This would mean developers could autocomplete usages of services by implementing tiny helper methods, relying on the trait to detect what they need based on these methods.

I would like to get opinions about this before trying to work on an implementation. WDYT?

[NicoHaase]

I don't get it. Is the only change that you discourage to use $this->get(Router:class) and want to enforce $this->getRouter() instead? Looks strange to me, as you also need to have the whole cached container in your IDE - and then it looks more easy to me to extend the current container dumper to write these helper functions than rely on some trait.

Where is the problem in installing eg. the Symfony plugin for PhpStorm? In addition to the container features, you get autocompletion for routes, navigation support from one Twig template to another,... - this solves much more little problems in one convenient solution

[sstok]

@NicoHaase:

Looks strange to me, as you also need to have the whole cached container in your IDE

Why would you need the whole service container? When you call a proxy method (which is basically what this is) the method defines the return type so you can get auto completion working.

A ServiceSubscriber is completely different from injecting the whole Container, it injects a minimum service-locator container with only the services your class "subscribed" to.

I would say it can work for the DX as you don't need to create the subscriber method every time, and can still get lazy service loading. But how is the performance of this auto auto wiring? :)

[NicoHaase]

When you define a service, you write yml or xml code or whatsever. Without compiling the container, no PHP code for anything is written, and no proxy methods are generated. On compiling the changed container, the proxy methods are written to some folder in app/cache. After compiling the container, it must be made available to your IDE, so app/cache must be a folder your IDE is aware of.

Now I see the following: in app/cache/dev, I have a file containing the dumped container. It even has return types attached, but all methods are protected. Now the idea is to create a new file which contains some of these definitions, but with public visibility. Sounds like pure duplication for me,

Side note: I'd love to get corrected if I got the concept wrong - but currently, I see nothing here that is really neccessary and does not introduce more complexity than needed.

[ro0NL]

This introduces more magic; id keep getSubscribedServices() a definition to be declared by the user.

Tend to agree with @NicoHaase; not sure what it solves. You can already do this without being forced to use __FUNCTION__ as a service id.

[tgalopin]

@NicoHaase No files are generated by the container, we are just providing a useful trait to help use service subscribers with autocompletion.

Injecting the whole container is a bad practice as it let you access an undefined number of services and parameters, so Symfony is not able to dump the code in cache and developers are not able to see the dependencies clearly. Using service locators solves these two issues, by creating a smaller container containing only the services defined in the service definition. Service locators are simply generic factories based on the DI component.

My proposal is only to add a trait in Symfony that implement a conventional behavior: match all the current service methods and tell the DI component the dependencies of the current class are those really used in the class. It's nothing more than a shortcut in terms of consequences, the cache won't be differently created and the optimizations will be able to be done just the same (especially removing unused private services).

This introduces more magic; id keep getSubscribedServices() a definition to be declared by the user.

That's the whole point of the proposal though ;) . The problem I raise is the following: by declaring service locators, you can't autocomplete their usages, whereas if you built your own factory, you could (by injecting the factory in the constructor). This trait would allow developers to declare service almost the same way as before, but with autocompletion.

To compare the solutions, here is the current definition you would do to get the router in a service subscriber:

<?php

class Foo implements ServiceSubscriberInterface
{
    public function do()
    {
        $this->get('router');
    }

    public static function getSubscribedServices()
    {
        return [
            'router' => RouterInterface::class,
        ];
    }
}

With my proposal, you would declare the router dependency using a helper method instead of an array:

<?php

class Foo implements ServiceSubscriberInterface
{
    use ServiceSubscriberTrait;
  
    public function do()
    {
        $this->getRouter();
    }

    private function getRouter(): RouterInterface
    {
        return $this->container->get(__FUNCTION__);
    }
}

It's almost the same, with the huge advantage of providing autocompletion on getRouter usages. Is it clearer like this?

[NicoHaase]

So the idea is that the developer has to write into two files for a new service, the file for usual DI configuration (which is compiled to a cached container), and a second file for the new ServiceSubscriberInterface? Who guarantees that both declarations always, always match? It is a common problem that developers oftenly don't care about changing comments when changing the explained implementation, and that takes already place in the same file.....

[nicolas-grekas]

@NicoHaase nope: ServiceSubscriberInterface is already autoconfigured, so most of the time you don't have to do anything. And if you have to, this is unrelated to this RFC, which leverages a mechanism that already exists.

[ro0NL]

Think i misunderstood the feature, it'll resolve the service based on return type :) not __FUNCTION__.

The idea is to detect in getSubscribedServices all the current class methods

All methods with return type might be too much; what about getters only? Note technically the method may have required arguments.

[tgalopin]

@ro0NL I planned to match only private methods having a return type that can be autowired, but we could also add a constraint on getXXX, indeed. It shouldn't be an issue to have required arguments as we won't call the method in the trait, the developer would.

[NicoHaase]

Obviously I still don't get it.... who writes that Foo file where the typehints can be found?

[tgalopin]

@NicoHaase the developer, it's userland code.

[NicoHaase]

And how should these traits become available in my controller (or other class that is able to use $this->get())? Do I have to import them manually? Obviously, because installing an external bundle cannot change the base classes

[ro0NL]

@tgalopin private seems opinionated =/

I like the idea, yet if the service cannot resolve im not sure what should happen. Silently skipping may lead to unexpected and hard to figure out behavior. However throwing without a whitelist is also annoying. So i think the pattern should indeed be getXXX and/or do some logging.

@NicoHaase see #21708 also :) SF will just provide you a trait that auto-implements that feature (except for the config, which is good :))

[NicoHaase]

So that means I can use this only when the container is compiled and the cached container is known to my IDE? Then it would be easier to change the visibility of the methods contained in the usual container from protected to public, because then no additional files are needed

[linaori]

Honestly, I don't see the point of this, just inject what you need and be done with it. It looks like nothing more than a hack to get something working which should not be done in the first place.

[tgalopin]

@NicoHaase I don't understand why you keep mentioning the compiled container, there is no link to it. You typehint against ContainerInterface in your services, not your development or production container, so you can't have generated code to do auto-completion (which would be much more magic anyway).

@iltar The main point is to improve auto-completion (on most IDEs, you don't have it for service locators) while not having to bloat your services for that. This trait would be a small generic trait to help you achieve this.

[ro0NL]

If you use this feature (ServiceSubscriberInterface) you're probably better of using the proposed trait. Yet the benefit is minimal, i agree :) it'll only take away defining getSubscribedServices and force you to write getters

@NicoHaase no. Afer compling (not dumping) symfony will inject $this->container with a service locator based on getSubscribedServices, as it's static it's called at compile time.

[tgalopin]

it'll only take away defining getSubscribedServices

and it will provide auto-completion, which is not that minimal IMO :) .

[ro0NL]

it'll only take away defining getSubscribedServices and force you to write getters

:) that's probably the good part yes.

[nicolas-grekas]

that's probably the good part yes

that's not only the good part: autocompletion is THE reason why this RFC exists in the first place.

It looks like nothing more than a hack to get something working which should not be done in the first place

let me recap: ServiceSubscriberInterface makes your deps explicit. That means this "should not be done" misses a reason now. Without a reason to not use it, well, you can use it (whatever "it" is.)

Then, ServiceSubscriberInterface is opaque to (IDE or plugin-based) autocompletion (yet, because nothing prevents them from fixing that.)

Which means we're seeking for using the ServiceSubscriberInterface now, AND have autocompletion.
By using a trait, we add a behavior that turns implementing the interface into a convention - so that it's easier and even more useful for you to write explicit service locators.

[linaori]

The main point is to improve auto-completion (on most IDEs, you don't have it for service locators) while not having to bloat your services for that. This trait would be a small generic trait to help you achieve this.

And you will have autocompletion if you properly type-hint your constructor arguments, it still doesn't make it a valid reason IMO to add an IDE hack to a framework.

[ro0NL]

The feature is already here; this just provides a convention based use of it.

There's no IDE hack going on; you write the getters your self, which you already can do today. Proposed feature just leverages the fact we intend getters as subscribed services.

[NicoHaase]

Well, then I still don't understand what the magic is. Let me ask again with a concrete example:

• someone adds a class MagicHelperTrait to his bundle that implements ServiceSubscriberInterface and thus should help me to typehint some services from that bundle
• the goal is that my IDE gets to know some of the shortcuts defined in MagicHelperTrait when I am writing stuff inside, lets say, a controller class
• How on earth could my IDE know about the methods defined in MagicHelperTrait?

[ro0NL]

and thus should help me to typehint some services

No :)

let me recap: ServiceSubscriberInterface makes your deps explicit.

It only exposes a static array of service id => class name. It triggers no additional generated code or so.

How on earth could my IDE know about the methods defined in MagicHelperTrait?

Probably because you wrote that trait (or class.. dunno ;-))

[NicoHaase]

Well, "no" is not a really helpful answer. The initial proposal talks about auto completion and the return type of $this->get($servicename). What did I get wrong if this is not about typehinting?

[ro0NL]

The answer was in the 2nd quote :)

It's about type-hinting because if you write your Foo class as proposed (https://gist.github.com/tgalopin/46e2be955762d6d68360dd455afeab70#file-service_subscriber_trait-php-L12) that will be enabled out-of-the-box; depending on your IDE of course.

So from #23898 (comment) the before/after is actually;

// before
class Foo implements ServiceSubscriberInterface
{
    public function do()
    {
        $this->getRouter();
    }

    private function getRouter(): RouterInterface
    {
        // youll get container as a constructor arg for this Foo service i believe
        return $this->container->get(__FUNCTION__);
    }

    public static function getSubscribedServices()
    {
        return [
            'getRouter' => RouterInterface::class,
        ];
    }
}

// after
class Foo implements ServiceSubscriberInterface
{
    use ServiceSubscriberTrait;
  
    public function do()
    {
        $this->getRouter();
    }

    private function getRouter(): RouterInterface
    {
        return $this->container->get(__FUNCTION__);
    }
}

[tgalopin]

Thanks @ro0NL :)

@iltar

And you will have autocompletion if you properly type-hint your constructor arguments, it still doesn't make it a valid reason IMO to add an IDE hack to a framework.

Service locators are a really useful way to get laziness of services without having to inject the whole container or build a factory yourself. It's a feature already introduced in Symfony, this PR is not about whether you should use locators or not but what happens when you do.

When you use locators, you have a local container-like object, dedicated to your service, that you can use to obtain services lazily. It's much better than injecting the container because you still declare dependencies using getSubscribedServices. Service locators replace a direct constructor injection with useful features.

However, when using service locators, you don't get autcompletion anymore: the aim of this proposal is to improve this.

@NicoHaase

someone adds a class MagicHelperTrait to his bundle that implements ServiceSubscriberInterface and thus should help me to typehint some services from that bundle

That is not how things would work. The trait would be in Symfony, in the framework.

the goal is that my IDE gets to know some of the shortcuts defined in MagicHelperTrait when I am writing stuff inside, lets say, a controller class

Not at all, the trait ONLY aims to populate getSubscribedServices by detecting the current class getters.

How on earth could my IDE know about the methods defined in MagicHelperTrait?

It's not how this would work. This proposal is about a single trait in the framework usable by anyone wanting to use ServiceSubscriberInterface.

The initial proposal talks about auto completion and the return type of $this->get($servicename)

The proposal is NOT about adding a typehint to $this->get() but to use intermediate helpers methods that could have this typehint. With this helper methods, defined by the developer in their services, they would get autocompletion. By adding the Symfony trait, the getSubscribedServices would be already implemented to match the getters. It's just that.

@ro0NL

// youll get container as a constructor arg for this Foo service i believe

Actually you will get it from the trait (https://gist.github.com/tgalopin/46e2be955762d6d68360dd455afeab70#file-service_subscriber_trait-php-L20).

[NicoHaase]

I stop bothering you now, as I think I still don't see the improvement but many others do :D Probably the penny will drop when this is implemented and I can see where my thoughts went wrong.....

[javiereguiluz]

Given that we're trying to "deprecate" or at least unpopularize the use of $this->get('my_service') and $this->get(MyService::class) in favor of constructor/method arguments, I'd say it's OK if they don't autocomplete in IDEs. That's another great reason to stop using ->get(...), so I don't think we should fix that "issue".

[tgalopin]

@javiereguiluz we are not deprecating this->get, we are deprecating injecting the whole container into classes such as controllers. Service locators are a perfectly valid way to have this->get.

[tgalopin]

For everyone, please try to not think about controllers for this RFC. Controllers are, until 3.3, getting the whole container passed to them so they can get public services from it. This will change in future versions of Symfony as we deprecate the injection of the whole container into classes, even in controllers.

However, a great feature introduced by injecting the whole container was laziness: the dependencies would only be instantiated when requested, instead of when the parent service is constructed. This feature was removed by deprecating container injection, so we introduced Service Locators which are a much more elegant way to deal with laziness as they still express dependencies (useful to dump the container).

This proposal is only about providing this laziness feature of Service Locators with the same level of auto-completion than classical injection (see my answer: #23898 (comment))

[kbond]

I have created the following trait in my app:

namespace App;

use Psr\Container\ContainerInterface;

trait ServiceSubscriberTrait
{
    /** @var ContainerInterface */
    private $container;

    public static function getSubscribedServices(): array
    {
        $class = new \ReflectionClass(static::class);
        $services = [];

        foreach ($class->getMethods() as $method) {
            if (false === strpos($method->getDocComment(), '@service')) {
                continue;
            }

            if (!$returnType = $method->getReturnType()) {
                continue;
            }

            $returnType = $returnType->getName();

            if (!class_exists($returnType) && !interface_exists($returnType)) {
                continue;
            }

            $services[$method->getName()] = $returnType;
        }

        return $services;
    }

    /**
     * @required
     */
    public function setContainer(ContainerInterface $container): void
    {
        $this->container = $container;
    }

    protected function service(string $id)
    {
        return $this->container->get($id);
    }
}

Then my service can be defined as such:

namespace App;

use Symfony\Component\DependencyInjection\ServiceSubscriberInterface;

class MyService implements ServiceSubscriberInterface
{
    use ServiceSubscriberTrait;

    public function doSomething()
    {
        $this->getMyOtherService()->doSomething();
    }

    /**
     * @service
     */
    private function getMyOtherService(): MyOtherService
    {
        return $this->service(__FUNCTION__);
    }
}