[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 :))