Skip to content

[DependencyInjection] Invokable Factory Services #30255

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

Merged

Conversation

zanbaldwin
Copy link
Member

@zanbaldwin zanbaldwin commented Feb 15, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR symfony/symfony-docs#11014

Failing test is in the Twig bridge, and outside of the the scope of this PR.

Allow referencing invokable factory services, just as route definitions reference invokable controllers.
This functionality was also added for service configurators for consistency.

Example

<?php

namespace App\Factory;

class ServiceFactory
{
    public function __invoke(bool $debug)
    {
        return new Service($debug);
    }
}
services:
    App\Service:
        # Prepend with "@" to differentiate between service and function.
        factory: '@App\Factory\ServiceFactory'
        arguments: [ '%kernel.debug%' ]
<?xml version="1.0" encoding="UTF-8" ?>
<container xmlns="http://symfony.com/schema/dic/services"
           xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
           xsi:schemaLocation="http://symfony.com/schema/dic/services
               http://symfony.com/schema/dic/services/services-1.0.xsd">
    <services>
        <!-- ... -->
        <service id="App\Service"
                 class="App\Service">
            <factory service="App\Factory\ServiceFactory" />
        </service>
    </services>
</container>
<?php

use App\Service;
use App\Factory\ServiceFactory;
use Symfony\Component\DependencyInjection\Reference;

$container->register(Service::class, Service::class)
    ->setFactory(new Reference(ServiceFactory::class));

@zanbaldwin zanbaldwin changed the title Invokable Factory Services [DependencyInjection] Invokable Factory Services Feb 16, 2019
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 16, 2019

Looks like there are no reasons not to do it to me.
Some notes to help:

  • should target master
  • the implementation should be quite different: there should be no need for anything special in AbstractRecursivePass. Instead, we should make ContainerBuilder and PhpDumper handle when Definition::getFactory() returns a reference. That should be enough.

@nicolas-grekas nicolas-grekas added this to the next milestone Feb 16, 2019
@zanbaldwin zanbaldwin force-pushed the feature/callable-factory-services branch from 57e42e9 to 98693be Compare February 18, 2019 14:11
@zanbaldwin zanbaldwin changed the base branch from 3.4 to master February 18, 2019 14:15
@zanbaldwin
Copy link
Member Author

  • Now targetting master
  • AbstractRecursivePass was expecting an array, now handles a Reference object.
  • Now sure how to move that logic out into ContainerBuilder and PhpDumper unfortunately 🙁

@nicolas-grekas
Copy link
Member

AbstractRecursivePass was expecting an array, now handles a Reference object.

actually, maybe this should be handled 100% at the loader level? If service reference then method defaults to __invoke?

@zanbaldwin zanbaldwin force-pushed the feature/callable-factory-services branch from 9e2edb7 to 3b7bc89 Compare February 18, 2019 17:48
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Nice. Could you please add some test cases? Then it will look to good me.

@@ -107,6 +107,10 @@ public function setFactory($factory)
$factory = explode('::', $factory, 2);
}

if ($factory instanceof Reference) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be elseif. If \is_string($factory) resolves to true, instanceof will never be true. Although it does not really matter, it seems to be more logical to me, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it should be an elseif logically to avoid an extra if condition.
Also I would suggest to extract the duplicate parsing logic in setConfigurator and setFactory into a private method.

@zanbaldwin
Copy link
Member Author

Undrafting PR because I'm getting Class 'PHPUnit_Framework_TestCase' not found errors locally.

@zanbaldwin zanbaldwin marked this pull request as ready for review February 19, 2019 11:29
@zanbaldwin zanbaldwin force-pushed the feature/callable-factory-services branch 3 times, most recently from a470019 to c1ccda1 Compare February 19, 2019 12:03
zanbaldwin added a commit to zanbaldwin/symfony-docs that referenced this pull request Feb 19, 2019
Document new invokable factories (and configurators) implemented in symfony/symfony#30255.
zanbaldwin added a commit to zanbaldwin/symfony-docs that referenced this pull request Feb 19, 2019
Document new invokable factories (and configurators) implemented in symfony/symfony#30255.
@zanbaldwin zanbaldwin force-pushed the feature/callable-factory-services branch from c1ccda1 to c65fb8e Compare February 19, 2019 16:03
@zanbaldwin zanbaldwin force-pushed the feature/callable-factory-services branch from c65fb8e to 16b7fcf Compare February 22, 2019 16:31
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(once the remaining comments are addressed)

@zanbaldwin zanbaldwin force-pushed the feature/callable-factory-services branch 2 times, most recently from 2e81b0f to 801bb8a Compare February 25, 2019 10:59
@zanbaldwin
Copy link
Member Author

Done, and rebased latest master.

@@ -107,6 +107,10 @@ public function setFactory($factory)
$factory = explode('::', $factory, 2);
}

if ($factory instanceof Reference) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it should be an elseif logically to avoid an extra if condition.
Also I would suggest to extract the duplicate parsing logic in setConfigurator and setFactory into a private method.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(I just fixed the comments about the "if")

@fabpot fabpot force-pushed the feature/callable-factory-services branch from b47aa9b to 23cb83f Compare April 3, 2019 12:19
@fabpot
Copy link
Member

fabpot commented Apr 3, 2019

Thank you @zanbaldwin.

@fabpot fabpot merged commit 23cb83f into symfony:master Apr 3, 2019
fabpot added a commit that referenced this pull request Apr 3, 2019
…aldwin)

This PR was squashed before being merged into the 4.3-dev branch (closes #30255).

Discussion
----------

[DependencyInjection] Invokable Factory Services

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

> Failing test is in the Twig bridge, and outside of the the scope of this PR.

Allow referencing invokable factory services, just as route definitions reference invokable controllers.
This functionality was also added for service configurators for consistency.

## Example

```php
<?php

namespace App\Factory;

class ServiceFactory
{
    public function __invoke(bool $debug)
    {
        return new Service($debug);
    }
}
```

```yaml
services:
    App\Service:
        # Prepend with "@" to differentiate between service and function.
        factory: '@app\Factory\ServiceFactory'
        arguments: [ '%kernel.debug%' ]
```

```xml
<?xml version="1.0" encoding="UTF-8" ?>
<container xmlns="http://symfony.com/schema/dic/services"
           xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
           xsi:schemaLocation="http://symfony.com/schema/dic/services
               http://symfony.com/schema/dic/services/services-1.0.xsd">
    <services>
        <!-- ... -->
        <service id="App\Service"
                 class="App\Service">
            <factory service="App\Factory\ServiceFactory" />
        </service>
    </services>
</container>
```

```php
<?php

use App\Service;
use App\Factory\ServiceFactory;
use Symfony\Component\DependencyInjection\Reference;

$container->register(Service::class, Service::class)
    ->setFactory(new Reference(ServiceFactory::class));
```

Commits
-------

23cb83f [DependencyInjection] Invokable Factory Services
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Apr 5, 2019
…dwin)

This PR was merged into the master branch.

Discussion
----------

[DependencyInjection] Invokable Factory Services

Document new invokable factories (and configurators) implemented in symfony/symfony#30255.

There are now four ways to reference factories, perhaps making them more clear might be something else to do.
- `function`
- `Class::method`
- `Service:method`
- `@InvokableService`

Commits
-------

2d7709f [DependencyInjection] Invokable Factory Services
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fabpot fabpot mentioned this pull request May 9, 2019
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.

8 participants