-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] Add constructor
option to #[Autoconfigure]
#49665
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
Conversation
c2285d2
to
e5c5092
Compare
It would be very nice to have an even shorter syntax when the factory is a static function of the current class. #[Factory('create', ['$foo' => 'foo'])]
class FactoryAttributeService
{ The attribute could also be on the factory function (we will have to deal with duplicate). class FactoryAttributeService
{
#[Factory(['$foo' => 'foo'])]
public static function create(string $foo): static |
e5c5092
to
1e378f6
Compare
Thanks for the feedback @GromNaN! I implemented the first idea, but for the second one, I'm not sure. I mean, this is indeed a great idea, but it seems it needs a lot of rework in But maybe my approach is not the right one and there's something I don't see? |
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.
Awesome. Actually I have no idea how to implement the 2nd idea.
Do we need this |
I don't know if it's a necessity, but being able to configure the specific bindings of the factory directly in the attribute seems to me a good idea, because it allows to have all the information "centralized in the same place" in some way. What do you think? |
1e378f6
to
b230ebb
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.
Thanks for starting this.
We didn't list "factory" in the possible options to autoconfigure because it just didn't make sense. The reason why it didn't make sense is that configuring a factory without telling the factory which class to instantiate doesn't make sense either.
It looks like we spotted the only situation where we do have a way to tell the factory which class it needs to instantiate : static constructors (the class is passed to the factory via static::class
.)
If we want to support this pattern, I suggest this: #[Autoconfigure(constructor: 'theMethod')]
(and I'm not sure we need a dedicated attribute for this.)
While adding support for this via attribute, we should also support it via other configuration means. Yaml/XML/PHP-DSL should support this "constructor" key for their "instance-of-conditionals" (which is what autoconfiguration works on).
About the attached patch I'd also like to have an end-to-end test case, one in PhpDumperTest and one in ContainerBuilderTest.
I am not sure what is the goal here, I could see two:
I am most interested in the first point. class C {
#[Service]
static function createA(): A {..}
#[Service]
static function createB(): B {..}
} To avoid duplicates, the attribute can have a custom id, or a suffix that will be appended to the class name. class C {
#[Service]
static function createA(): A {..}
#[Service(id: 'a.other')]
static function createOtherA(): A {..}
#[Service]
static function createB(): B {..}
#[Service(suffix: '.other')]
static function createOtherB(): B {..}
} The |
1a2f758
to
5499dda
Compare
#[Factory]
and factory
option to #[Autoconfigure]
constructor
option to #[Autoconfigure]
69a729a
to
a07bda5
Compare
Thank you for your feedback @nicolas-grekas! I completely reworked the PR. It now allows a When using @donquixote I think it would worth it to discuss this in an issue to gather feedback, as this PR's initial goal changed a bit 😄 |
a07bda5
to
ee70b26
Compare
src/Symfony/Component/DependencyInjection/Compiler/ResolveStaticConstructorPass.php
Outdated
Show resolved
Hide resolved
ee70b26
to
4c92908
Compare
...ony/Component/DependencyInjection/Tests/Compiler/RegisterAutoconfigureAttributesPassTest.php
Outdated
Show resolved
Hide resolved
1b56ed9
to
b3c97c0
Compare
src/Symfony/Component/DependencyInjection/Loader/schema/dic/services/services-1.0.xsd
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Tests/Fixtures/xml/constructor.xml
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services_with_constructor.yml
Outdated
Show resolved
Hide resolved
3d9a968
to
1374c03
Compare
src/Symfony/Component/DependencyInjection/Loader/schema/dic/services/services-1.0.xsd
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Loader/schema/dic/services/services-1.0.xsd
Outdated
Show resolved
Hide resolved
...y/Component/DependencyInjection/Tests/Fixtures/config/inline_static_constructor.expected.yml
Outdated
Show resolved
Hide resolved
dc8ae17
to
2497356
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.
LGTM thanks (but a rebase is needed.)
…n and to `#[Autoconfigure]`
2497356
to
8dda3f0
Compare
That's rebased 👍 ! |
Thank you @alexandre-daubois. |
I'm late to the game so feel free to ignore this comment. The
|
…rvice definition (alexandre-daubois) This PR was merged into the 6.3 branch. Discussion ---------- [DependencyInjection] Add the `constructor` option to service definition Related to symfony/symfony#49665 Commits ------- 6040cfe [DependencyInjection] Add the `constructor` option to service definition
…en `lint:container` builds the container from a dump (MatTheCat) This PR was merged into the 6.3 branch. Discussion ---------- [DependencyInjection] Run the `ResolveFactoryClassPass` when `lint:container` builds the container from a dump | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #50622 | License | MIT | Doc PR | N/A #49665 replaced the `factory` node by a `constructor` attribute in the XML and YAML dumper when the factory’s class is the same as the definition’s. The corresponding loader then creates a definition where the factory class is `null`. As the `ResolveFactoryClassPass` will not run when the `lint:container` command builds the container from an XML dump, such factories would make `ContainerBuilder::createService` crash. This PR adds this compiler pass to the list. Commits ------- 5cf4b63 [FrameworkBundle] Run the `ResolveFactoryClassPass` when `lint:container` builds the container from a dump
…constructor when autodiscovering (nicolas-grekas) This PR was merged into the 7.3 branch. Discussion ---------- [DependencyInjection] Don't skip classes with private constructor when autodiscovering | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | Fix #48392 | License | MIT With value objects auto-discovery becoming more mainstream (see #59704), it's time to fix registering classes with private constructors. Those are skipped today but with support for `#[Autoconfigure(constructor: 'createInstance')]` as introduced in #49665, this doesn't make sense anymore. Best reviewed [ignoring whitespace](https://github.com/symfony/symfony/pull/59712/files?w=1). Commits ------- 99830f6 [DependencyInjection] Don't skip classes with private constructor when autodiscovering
Following this discussion on Twitter, here is my try to use service factories with attributes. This PR adds the
constructor
option to theAutoconfigure
attribute, and more generally, theconstructor
option on service definitions.This allows to write services using a factory this way: