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] Add new Autoconfigure attributes #48162

Open
wants to merge 1 commit into
base: 6.3
Choose a base branch
from

Conversation

alexndlm
Copy link
Contributor

@alexndlm alexndlm commented Nov 8, 2022

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR

New Autoconfigure attributes for more convenient configuration of services.

@alexndlm
Copy link
Contributor Author

alexndlm commented Nov 8, 2022

@nicolas-grekas, would you check this PR?

If a general idea is correct and acceptable, I will create tests.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 9, 2022

I'm not sure this is useful at this stage:

  • #[Autowire] already covers most of what #[AutoconfigureBind*] would provide
  • #[Required] together with #[Autowire] already cover what #[AutoconfigureCall*] would

We might just need to allow #[Autowire] on properties so that we can also use it with #[Required], but that'd be only for completeness.

WDYT?

@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 9, 2022
@alexndlm
Copy link
Contributor Author

alexndlm commented Nov 9, 2022

With #[Autowire], I can't configure with attributes the following case:

<?php
declare(strict_types=1);

abstract class AbstractFooService
{
    public function __construct(private ProcessorInterface $processor)
    {
    }

    public function process(): void
    {
        $this->processor->process();
    }
}

final class FooService extends AbstractFooService
{
}

final class BarService extends AbstractFooService
{
}

return static function (ContainerConfigurator $containerConfigurator): void {
    $services = $containerConfigurator->services();

    $services->defaults()
        ->autowire()
        ->autoconfigure();

    $services->set(FooService::class)
        ->arg('$processor', 'processor.foo');

    $services->set(BarService::class)
        ->arg('$processor', 'processor.bar');
};

@nicolas-grekas
Copy link
Member

Thanks for the example, that gives a much better idea of the motivation for this proposal!

What about this instead?

#[Autowire(bind: '$processor', service: 'processor.foo')]
final class FooService extends AbstractFooService
{   
}

#[Autowire(bind: '$processor', service: 'processor.bar')]
final class BarService extends AbstractFooService
{
}   

The bind argument would be required when using the attribute on a class, and forbidden on parameters+properties.

@alexndlm
Copy link
Contributor Author

alexndlm commented Nov 9, 2022

Looks awesome.

Do you mind allowing #[TaggedIterator] and #[TaggedLocator] for class and property and adding call and method params?

The final result will be like this:

#[Autowire(bind: '$processor', service: 'processor.foo')]
#[Autowire(call: 'setProcessor', service: 'processor.foo')]
#[Autowire(property: 'processor', service: 'processor.foo')]
#[TaggedIterator(bind: '$iterator', 'tag.name')]
#[TaggedIterator(call: 'setIterator', 'tag.name')]
#[TaggedIterator(property: 'iterator', 'tag.name')]
#[TaggedLocator(bind: '$locator', 'tag.name')]
#[TaggedLocator(call: 'setLocator', 'tag.name')]
#[TaggedLocator(property: 'locator', 'tag.name')]|
final class FooService
{
    #[Autowire(service: 'processor.foo')]
    public FooProcessor $processor2;

    #[TaggedIterator('tag.name')]
    public iterable $iterator2;

    #[TaggedLocator('tag.name')]
    public ServiceProviderInterface $locator2;

    public function __construct(
        #[Autowire(service: 'processor.foo')]
        private FooProcessor $processor,
        #[TaggedIterator('tag.name')]
        private iterable $iterator,
        #[TaggedLocator('tag.name')]
        private ServiceProviderInterface $locator,
    ) {
    }

    #[Autowire(service: 'processor.foo')]
    public function setProcessor(FooProcessor $processor): void
    {
        ...
    }
    
    #[TaggedIterator('tag.name')]
    public function setIterator(iterable $iterator): void
    {
        ...
    }

    #[TaggedLocator('tag.name')]
    public function setLocator(ServiceProviderInterface $locator): void
    {
        ...
    }
}

@alexndlm
Copy link
Contributor Author

alexndlm commented Dec 5, 2022

@nicolas-grekas, would you please check the proposal above?

If ok, I will close this PR and create three smaller PRs.

@nicolas-grekas
Copy link
Member

Thanks for the update.

I'm not in favor of the proliferation of attributes. I'm therefor looking to add as little as possible.

As I mentioned, #[Required] already covers "call". The use case you have with inheritance can be covered already with #[Autoconfigure(bind: ...)]. But it's true that it's not very convenient to define the value of the binding.

I think we can improve all this by adding one attribute, which we might name just #[Bind].

Its signature would be the following:

    public function __construct(
        string $name,
        string|array|TaggedIterator|TaggedLocator $value = null,
        string $service = null,
        string $expression = null,
        string $env = null,
        string $param = null,
    )

Note that $value accepts also TaggedIterator|TaggedLocator so that there would be no need to add corresponding dedicated attributes. One would instead use #[Bind('foo', new TaggedIterator('tag'))].

WDYT?

@kbond
Copy link
Member

kbond commented Jan 7, 2023

I think we can improve all this by adding one attribute, which we might name just #[Bind].

Love this idea!

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.

None yet

4 participants