Skip to content

[DependencyInjection] Add AsAlias attribute #41207

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

Closed
wants to merge 2 commits into from
Closed

[DependencyInjection] Add AsAlias attribute #41207

wants to merge 2 commits into from

Conversation

HypeMC
Copy link
Member

@HypeMC HypeMC commented May 12, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets #41188
License MIT
Doc PR TODO

Attribute for defining service aliases:

use Symfony\Component\DependencyInjection\Attribute\AsAlias;
use App\MyInterface;

#[AsAlias(id: 'my-alias', public: true)]
#[AsAlias(id: MyInterface::class)]
class MyService
{}

@HypeMC HypeMC changed the title Add AsAlias attribute [DependencyInjection] Add AsAlias attribute May 12, 2021
@derrabus derrabus added this to the 5.x milestone May 13, 2021
@nicolas-grekas
Copy link
Member

Thanks for the PR, see discussion in #41188

@nicolas-grekas
Copy link
Member

As explained in #41188, we'd also need a collision-detection-and-resolution mechanism, whatever the resolution algo, which we need to design.

@HypeMC
Copy link
Member Author

HypeMC commented May 13, 2021

As explained in #41188, we'd also need a collision-detection-and-resolution mechanism, whatever the resolution algo, which we need to design.

@nicolas-grekas Currently an exception is thrown if the alias is already in use, with a message that points to the service using it. But now that I think about it, this wouldn't work if a vendor service used the attribute, since you wouldn't be able to change that.

I'm guessing one way of doing it is to keep the current behavior and just let next definition overwrite the previous one?

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 the PR.
I'm not really convinced this provides a real benefit over configuration.
The reason is that configuration is centralized so that it's easy to know where to check for aliases. But on a class, this can be at a random location. Also, the conflicts might be confusing and I don't think we can define a behavior that makes them go away.

@HypeMC
Copy link
Member Author

HypeMC commented Jun 21, 2021

Thanks for the PR.
I'm not really convinced this provides a real benefit over configuration.
The reason is that configuration is centralized so that it's easy to know where to check for aliases. But on a class, this can be at a random location. Also, the conflicts might be confusing and I don't think we can define a behavior that makes them go away.

@nicolas-grekas Thanks for the feedback, your point is valid, it could be hard to find which class has this attribute, having it in a config file is probably better. If it's ok wit you I'll close this PR .

@OskarStark OskarStark changed the title [DependencyInjection] Add AsAlias attribute [DependencyInjection] Add AsAlias attribute Aug 1, 2021
@nicolas-grekas
Copy link
Member

Let's close, thanks for giving this a try!

@nicolas-grekas
Copy link
Member

See #49361

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.

5 participants