Skip to content

[DependencyInjection] Automatic registration of FQCN services #18268

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

Closed

Conversation

hason
Copy link
Contributor

@hason hason commented Mar 23, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets
License MIT
Doc PR

All services can be accessed by FQCN, the same concept as for form types:

# routing.yml
index:
  path: /
  defaults:
    _controller: 'AppBundle\Controller\MyController:index'
# services.yml
services:
  my_controller:
    class: AppBundle\Controller\MyController
    arguments:
      - '@Symfony\Bridge\Doctrine\RegistryInterface'
class OldController extends Controller
{
    public function indexAction()
    {
        $em = $this->get(RegistryInterface::class)->getManager();
    }
}

@Nicofuma
Copy link
Contributor

To me it looks very close to the issue solved by the autowiring... What is the issue you are trying to solve? (at least I think that a big part of the code could be shared)

@hason hason force-pushed the class-named-services branch 2 times, most recently from 46dfd0f to fb1e27e Compare March 23, 2016 09:25
@hason hason force-pushed the class-named-services branch from fb1e27e to 88ccab4 Compare March 23, 2016 09:51

class AmbiguousService
{
static public function throwException($class, $services)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the goal of this class ?

@GuilhemN
Copy link
Contributor

as @Nicofuma said, this is very close to what does autowiring and makes the container even bigger... so I'd say 👎 as is.

@dunglas
Copy link
Member

dunglas commented Mar 23, 2016

What can be nice instead is an utility class returning the list of services of a given type. (This class can be used internally by the autowiring system and in external bundles needing such features).

@backbone87
Copy link
Contributor

$em = $this->get(RegistryInterface::class)->getManager(); looks acutally quite neat, but maybe i am missing something

@dunglas
Copy link
Member

dunglas commented Mar 24, 2016

@backbone87 it is indeed attractive.

To prevent any confusion, why not introducing a new method returning a list of all services of a given type:

$container->getOfType(RegistryInterface::class); // the instance of the service of the given type, throw an exception if there is several services of this type
$container->getAllOfType(RegistryInterface::class); // an array of services having the given type

@hason
Copy link
Contributor Author

hason commented Mar 24, 2016

@dunglas And why introduce a new method (compare it with http://php-di.org/)? What is use case of the public method $container->getAllOfType(RegistryInterface::class);?

@hason
Copy link
Contributor Author

hason commented Mar 24, 2016

@Ener-Getick @Nicofuma Autowiring is only a special application of FQCN services. Symfony supports this feature for autowired services, but you can't make a reference to service by its type manually. That's inconsistent behavior.

@dunglas
Copy link
Member

dunglas commented Mar 24, 2016

A new method because it doesn't do the same thing: get retrieves a service by its name, getOfType retrieves a service by one of its types (SOLID code).

getAllOfType can be handy to retrieve all your entity managers, or all your elastica finders for instance, or all your API Platform data providers. I can think to a lot of usages.

@hason
Copy link
Contributor Author

hason commented Mar 24, 2016

@dunglas The service name can be string or FQCN. The string is alias for FQCN and vice versa.

services:
    Twig_Environment:
        class: Twig_Environment

    twig:
        alias: Twig_Environment

What is the correct call, $container->get(Twig_Environment::class) or $container->getOfType(Twig_Environment::class)?

Mentioned reasons for the method getAllOfType() are solved by tags.

@Exter-N
Copy link

Exter-N commented Mar 24, 2016

It would also be nice to make getAllOfType return the service identifiers as keys of the returned array.

@GuilhemN
Copy link
Contributor

I opened #18300 which is an alternative approach using the methods @dunglas proposed (and one more: getServiceIdsOfType).
This also replaces the utility class described by @dunglas.

@nicolas-grekas
Copy link
Member

The DI container is not a service locator and should not be
This leans over this limit to me (also getAllOfType, getOfType, etc.)
👎 as well.

@GuilhemN
Copy link
Contributor

GuilhemN commented Apr 2, 2016

@nicolas-grekas what do you think about the utility class I introduced in #18300 (i changed my mind and decided to create a new class instead of introducing this kind of methods in the container, see #18300 (comment), it's only possible to use it at compilation not at runtime) ?

@hason
Copy link
Contributor Author

hason commented Apr 5, 2016

@nicolas-grekas Exactly! This PR is introducing automatic registration of service aliases. It has nothing to do with a service locator.

@hason
Copy link
Contributor Author

hason commented Sep 28, 2016

I created the https://github.com/symfonette/class-named-services library.

@nicolas-grekas nicolas-grekas changed the title [POC] [WIP] [DependencyInjection] Automatic registration of FQCN services [DependencyInjection] Automatic registration of FQCN services Dec 6, 2016
@nicolas-grekas nicolas-grekas added this to the 3.x milestone Dec 6, 2016
fabpot added a commit that referenced this pull request Jan 7, 2017
…-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Optional class for named services

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Continues #20264:
- makes the id-to-class mapping context-free (no `class_exist` check),
- deals with ChildDefinition (which should not have this rule applied on them),
- deprecates FactoryReturnTypePass as discussed on slack and reported in this comment: #19191 (comment)

From @hason:

> I prefer class named services (in applications) because naming things are too hard:

``` yaml
services:
    Vendor\Namespace\Class:
        class: Vendor\Namespace\Class
        autowire: true
```

> This PR solves redundant parameter for class:

``` yaml
services:
    Vendor\Namespace\Class:
        autowire: true
```

> Inspirations: https://laravel.com/docs/5.3/container, #18268, http://php-di.org/,

Commits
-------

a18c4b6 [DI] Add tests for class named services
71b17c7 [DI] Optional class for named services
@TomasVotruba
Copy link
Contributor

I thinks this is solved by #21133, isn't it?

@theofidry
Copy link
Contributor

@TomasVotruba yes, actually it's been working for a while already #21133 just made it easier. There is this however that's still missing and might be interesting:

# routing.yml
index:
  path: /
  defaults:
    _controller: 'AppBundle\Controller\MyController:index'

@nicolas-grekas
Copy link
Member

Closing then, thanks @hason

@nicolas-grekas nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.