-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
[DependencyInjection] Invokable Factory Services #30255
Conversation
Looks like there are no reasons not to do it to me.
|
57e42e9
to
98693be
Compare
|
actually, maybe this should be handled 100% at the loader level? If service reference then method defaults to |
src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php
Outdated
Show resolved
Hide resolved
9e2edb7
to
3b7bc89
Compare
There was a problem hiding this 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.
src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
Outdated
Show resolved
Hide resolved
@@ -107,6 +107,10 @@ public function setFactory($factory) | |||
$factory = explode('::', $factory, 2); | |||
} | |||
|
|||
if ($factory instanceof Reference) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Undrafting PR because I'm getting |
a470019
to
c1ccda1
Compare
Document new invokable factories (and configurators) implemented in symfony/symfony#30255.
Document new invokable factories (and configurators) implemented in symfony/symfony#30255.
c1ccda1
to
c65fb8e
Compare
src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
Outdated
Show resolved
Hide resolved
c65fb8e
to
16b7fcf
Compare
There was a problem hiding this 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)
2e81b0f
to
801bb8a
Compare
Done, and rebased latest |
@@ -107,6 +107,10 @@ public function setFactory($factory) | |||
$factory = explode('::', $factory, 2); | |||
} | |||
|
|||
if ($factory instanceof Reference) { |
There was a problem hiding this comment.
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.
801bb8a
to
b47aa9b
Compare
There was a problem hiding this 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")
b47aa9b
to
23cb83f
Compare
Thank you @zanbaldwin. |
…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
…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
master
Allow referencing invokable factory services, just as route definitions reference invokable controllers.
This functionality was also added for service configurators for consistency.
Example