[DependencyInjection] Add #[AutowireInline] attribute to allow service definition at the class level

[DaDeather]

Q	A
Branch	7.1
Bug fix	no
New feature	yes
Deprecations	no
Issues	Fix #52819
License	MIT

For the idea behind this feature see the issue that contains examples #52819

Example usage:

class SomeSourceAwareLogger
{
    public function __construct(
        private readonly LoggerInterface $logger,
        private readonly string $someSource,
    ) {
    }
}

class SomeSourceAwareLoggerFactory
{
    public function __construct(
        private readonly LoggerInterface $logger,
    ) {
    }

    public function create(string $someSource): SomeSourceAwareLogger
    {
        return new SomeSourceAwareLogger($this->logger, $someSource);
    }

    public static function staticCreate(LoggerInterface $logger, string $someSource): SomeSourceAwareLogger
    {
        return new SomeSourceAwareLogger($logger, $someSource);
    }
}

// -----------

class SomeClass1
{
    public function __construct(
        #[AutowireInline(class: SomeSourceAwareLogger::class, args: [new Reference(LoggerInterface::class), 'bar'])]
        public SomeSourceAwareLogger $someSourceAwareLogger,
    ) {
    }
}

// AND/OR

class SomeClass2
{
    public function __construct(
        #[AutowireInline(
            class: SomeSourceAwareLogger::class,
            factory: [SomeSourceAwareLoggerFactory::class, 'staticCreate'],
            args: [new Reference(LoggerInterface::class), 'someParam'],
        )]
        public SomeSourceAwareLogger $someSourceAwareLogger,
    ) {
    }
}

// AND/OR

class SomeClass3
{
    public function __construct(
        #[AutowireInline(
            class: SomeSourceAwareLogger::class,
            factory: [new Reference(SomeSourceAwareLoggerFactory::class), 'create'],
            args: ['someParam'],
        )]
        public SomeSourceAwareLogger $someSourceAwareLogger,
    ) {
    }
}

[nicolas-grekas]

Thanks for proposing and for submitting!

I'm wondering if we couldn't get rid of the interface and rename the proposed argument to e.g. AutowireInline?
To remove the interface, we could maybe make AutowireCallable extend AutowireInline.
Also, instead of a dedicated attribute for services built by factory, can't we add a factory argument to the AutowireInline attribute?

[DaDeather]

Thanks for proposing and for submitting!

I'm wondering if we couldn't get rid of the interface and rename the proposed argument to e.g. AutowireInline? To remove the interface, we could maybe make AutowireCallable extend AutowireInline. Also, instead of a dedicated attribute for services built by factory, can't we add a factory argument to the AutowireInline attribute?

Sure I was unsure about the naming anyway though 🙈.

About the interface:
Wouldn't it rather make sense to keep it to allow extending / allowing custom Attributes that may create definitions that could be used for the Autowiring to be handled?

I'd rather see it as a benefit there allowing one to implement the interface and go for it instead of extending the base classes like the AutowireInline (after renaming). Unless this may be undesired anyway 🤷. Just being curious here.

Combining the factory thing into the AutowireInline seems fine as well since both are inlined 👍.

I made the adjustments according to your comment unless the one with the Interface. Just let me know if I still should remove it / rename it or whatever 👍.

And thanks for your fast feedback on this!

[nicolas-grekas]

I'm not convinced by the interface, I'd prefer removing it.
I'm generally not convinced by interfaces for value objects and attributes are value objects.
Extending the class makes sense to me in this case.

[DaDeather]

I'm not convinced by the interface, I'd prefer removing it. I'm generally not convinced by interfaces for value objects and attributes are value objects. Extending the class makes sense to me in this case.

Alright 👍 I have removed the interface and based the AutowireCallable on the new AutowireInline as you proposed before.

[stof]

Please update the PR description to show usages of the feature. This makes it a lot easier for the documentation team (and also for reviewers)

[nicolas-grekas]

About my remaining concerns (ResolveNamedArgumentsPass, ResolveChildDefinitionPass and other pass not running), I'm wondering if we shouldn't add a new ResolveAutowireInlineAttributesPass that would scan autowired services for this attribute and would register definitions for each of them. Then, based on some naming convention (likely built on Container::hash($definition)), AutowirePass would just reference these definitions instead of calling AutowireInline::buildDefinition().

WDYT? Up to give it a try?

[DaDeather]

About my remaining concerns (ResolveNamedArgumentsPass, ResolveChildDefinitionPass and other pass not running), I'm wondering if we shouldn't add a new ResolveAutowireInlineAttributesPass that would scan autowired services for this attribute and would register definitions for each of them. Then, based on some naming convention (likely built on Container::hash($definition)), AutowirePass would just reference these definitions instead of calling AutowireInline::buildDefinition().

WDYT? Up to give it a try?

Sure!
Could you maybe give a few hints how / where to implement this correctly?
I'm struggling a bit with the input style and the expected outcome here and therefore maybe didn't understand what's really expected here.

Would you expect a definition be like this:

class AutowireInlineAttributesBar {
    public function __construct(Foo $foo, string $someString)
    {
    }
}

class AutowireInlineAttributes
{
    public function __construct(
        #[AutowireInline(AutowireInlineAttributesBar::class, [
            '$foo' => Foo::class,
            '$someString' => 'testString',
        ])]
        public AutowireInlineAttributesBar $inlined,
    ) {
    }
}

Where the class Foo then would be a autowired as the provided service name and the someString injected as a string or what would you rather expect here to happen?

Besides that if I got you right I would have to create a class called ResolveAutowireInlineAttributesPass which then checks for a passed Definition to be $definition->isAutowired() === true and check for it to have any AutowireInline attributes to be processed (calling ->buildDefinition(...) and registering those via $this->container->setDefinition(ContainerBuilder::hash($autowireInlineAttributeInstance), $definition);.

Is this assumption correct?

[nicolas-grekas]

You've got it right I think :)

[DaDeather]

You've got it right I think :)

I've prepared something that doesn't quite work 🤷 as mentioned before maybe I'm missing something here. Could you maybe take a look so that we may correct the issues there or my misunderstanding 🙈?

I would be grateful 👍 dd16390

@nicolas-grekas

[nicolas-grekas]

Thanks for the update, the path looks good to me.
Here are some comments to go to the next step.

[chalasr]

AutowireInline looks pretty cryptic, any newcomer wouldn't be able to get what the attribute is for by just looking at its name nor its description as it currently is. I think we either need to find a super self-explanatory name (I've no clue) or extend the description so that it tells what's the purpose of the attribute and when it should be used (inline service definition only means something to advanced Symfony's DIC hackers)

[nicolas-grekas]

Naming things... :)
"inline" refers to "inline_service()" in the PHP-DSL

AutowireInlineService? but verbose
AutowireNew? AutowireObject? AutowireInstance? or keep AutowireInline?

of course, a top notch description is also desirable

[DaDeather]

I added a better description here feel free to request further suggestments to it 👍.

[nicolas-grekas]

PR is ready.
I added a second commit to make the implementation a bit more generic.
Reviews welcome 🙏

[nicolas-grekas]

I added this rule so that we don't use internal service ids in error messages. This matters to this PR because the added code creates new removed-ids, which means fixtures have to be updated with (what I consider as) noise.

[fabpot]

Thank you @DaDeather.