Skip to content
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

[DependencyInjection] Invokable Factory Services #30255

Merged

Conversation

@zanbaldwin
Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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 zanbaldwin:feature/callable-factory-services branch from 57e42e9 to 98693be Feb 18, 2019

@zanbaldwin zanbaldwin changed the base branch from 3.4 to master Feb 18, 2019

@zanbaldwin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

  • 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

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

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 zanbaldwin:feature/callable-factory-services branch from 9e2edb7 to 3b7bc89 Feb 18, 2019

@nicolas-grekas
Copy link
Member

left a comment

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) {

This comment has been minimized.

Copy link
@vudaltsov

vudaltsov Feb 19, 2019

Contributor

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?

This comment has been minimized.

Copy link
@Tobion

Tobion Mar 9, 2019

Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

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

@zanbaldwin zanbaldwin marked this pull request as ready for review Feb 19, 2019

@zanbaldwin zanbaldwin force-pushed the zanbaldwin:feature/callable-factory-services branch 3 times, most recently from a470019 to c1ccda1 Feb 19, 2019

zanbaldwin added a commit to zanbaldwin/symfony-docs that referenced this pull request Feb 19, 2019

[DependencyInjection] Invokable Factory Services
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

[DependencyInjection] Invokable Factory Services
Document new invokable factories (and configurators) implemented in symfony/symfony#30255.

@zanbaldwin zanbaldwin force-pushed the zanbaldwin:feature/callable-factory-services branch from c1ccda1 to c65fb8e Feb 19, 2019

@zanbaldwin zanbaldwin force-pushed the zanbaldwin:feature/callable-factory-services branch from c65fb8e to 16b7fcf Feb 22, 2019

@nicolas-grekas
Copy link
Member

left a comment

(once the remaining comments are addressed)

@zanbaldwin zanbaldwin force-pushed the zanbaldwin:feature/callable-factory-services branch 2 times, most recently from 2e81b0f to 801bb8a Feb 25, 2019

@zanbaldwin

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

Done, and rebased latest master.

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

This comment has been minimized.

Copy link
@Tobion

Tobion Mar 9, 2019

Member

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.

@nicolas-grekas nicolas-grekas force-pushed the zanbaldwin:feature/callable-factory-services branch from 801bb8a to b47aa9b Apr 3, 2019

@nicolas-grekas
Copy link
Member

left a comment

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

@fabpot

fabpot approved these changes Apr 3, 2019

@fabpot fabpot force-pushed the zanbaldwin:feature/callable-factory-services branch from b47aa9b to 23cb83f Apr 3, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Thank you @zanbaldwin.

@fabpot fabpot merged commit 23cb83f into symfony:master Apr 3, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 3, 2019

feature #30255 [DependencyInjection] Invokable Factory Services (zanb…
…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

minor #11014 [DependencyInjection] Invokable Factory Services (zanbal…
…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 referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.