Skip to content

[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

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Mar 10, 2023

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets NA
License MIT
Doc PR symfony/symfony-docs#18070

Following this discussion on Twitter, here is my try to use service factories with attributes. This PR adds the constructor option to the Autoconfigure attribute, and more generally, the constructor option on service definitions.

This allows to write services using a factory this way:

#[Autoconfigure(constructor: 'create')]
class FactoryAttributeService
{
    public function __construct(private readonly string $bar)
    {
    }

    public function getBar(): string
    {
        return $this->bar;
    }

    public static function create(string $foo = 'foo'): static
    {
        return new self($foo);
    }
}

@GromNaN
Copy link
Member

GromNaN commented Mar 11, 2023

It would be very nice to have an even shorter syntax when the factory is a static function of the current class.

@weaverryan proposed

#[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

@alexandre-daubois
Copy link
Member Author

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 src/Symfony/Component/DependencyInjection/Compiler/RegisterAutoconfigureAttributesPass.php. The pass loops through class attributes which makes it more complex to deal with the #[Factory] attribute on methods.

But maybe my approach is not the right one and there's something I don't see?

Copy link
Member

@GromNaN GromNaN left a 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.

@stof
Copy link
Member

stof commented Mar 12, 2023

Do we need this $bind argument ? Can't we rely on autowiring on the factory signature ?

@alexandre-daubois
Copy link
Member Author

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?

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.

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.

@donquixote
Copy link
Contributor

I am not sure what is the goal here, I could see two:

  • Discover services based on attributes, extending the resource: mechanism so it also works for factories.
  • Provide additional information for services that are already being declared somewhere.

I am most interested in the first point.
For that, it would be most natural to have the attribute on a static method, and use the return type as the service id (unless a custom id is provided).
When doing this, I would name the attribute class Service, not Factory, because the fact it is a factory is already obvious.
A class can contain more than one factory, each returning a different service.

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 #[Service] attribute could also be added on class level to explicitly register a class as a service.
I think right now this is not needed because resource: already registers all the classes. But perhaps there could be a different mode where this does not happen, and where only classes with #[Service] attribute are registered as services.

@alexandre-daubois alexandre-daubois force-pushed the factory-attribute branch 3 times, most recently from 1a2f758 to 5499dda Compare March 14, 2023 20:13
@alexandre-daubois alexandre-daubois changed the title [DependencyInjection] Add #[Factory] and factory option to #[Autoconfigure] [DependencyInjection] Add constructor option to #[Autoconfigure] Mar 14, 2023
@alexandre-daubois alexandre-daubois force-pushed the factory-attribute branch 4 times, most recently from 69a729a to a07bda5 Compare March 14, 2023 20:34
@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Mar 14, 2023

Thank you for your feedback @nicolas-grekas! I completely reworked the PR. It now allows a constructor option on Autoconfigure. Also, this PR makes available constructor in PHP-DSL, YAML and XML.

When using constructor, factory cannot be used for obvious reasons.

@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 😄

@alexandre-daubois alexandre-daubois force-pushed the factory-attribute branch 4 times, most recently from 3d9a968 to 1374c03 Compare March 18, 2023 09:43
@alexandre-daubois alexandre-daubois force-pushed the factory-attribute branch 2 times, most recently from dc8ae17 to 2497356 Compare March 25, 2023 11:10
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.

LGTM thanks (but a rebase is needed.)

@alexandre-daubois
Copy link
Member Author

That's rebased 👍 !

@fabpot
Copy link
Member

fabpot commented Mar 31, 2023

Thank you @alexandre-daubois.

@fabpot fabpot merged commit ff361c8 into symfony:6.3 Mar 31, 2023
@kaznovac
Copy link
Contributor

kaznovac commented Apr 1, 2023

I'm late to the game so feel free to ignore this comment.

The constructor property name is confusing as it points to the existing (and in this case wrong) OOP concept.
It also rises the question "How the services are instantiated if this property is not set - without the constructor?!"

staticFactoryMethod is the name of this instantiation concept.
I agree it's too mouthful for the property name but it points a developer in the correct direction.

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jun 6, 2023
…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
nicolas-grekas added a commit that referenced this pull request Jul 16, 2023
…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
nicolas-grekas added a commit that referenced this pull request Feb 10, 2025
…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
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