Skip to content

[DependencyInjection] Target Attribute must fail if the target does not exist #48555

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.

Sign up for GitHub

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

Jump to bottom
Closed
lyrixx opened this issue Dec 8, 2022 · 3 comments · Fixed by #48707
Closed

[DependencyInjection] Target Attribute must fail if the target does not exist #48555

lyrixx opened this issue Dec 8, 2022 · 3 comments · Fixed by #48707
Labels
DependencyInjection Help wanted Issues and PRs which are looking for volunteers to complete them.

Comments

@lyrixx
Copy link
Member

lyrixx commented Dec 8, 2022

Description

see: https://symfony.com/blog/new-in-symfony-5-3-service-autowiring-with-attributes#selecting-autowire-alias-with-attributes

If I create an http client:

    http_client:
        scoped_clients:
            githubApi:
                scope: 'https://api\.github\.com'
                headers:
                    Accept: 'application/vnd.github.v3+json'

And I want to inject it into my service, I can do:

    public function __construct(
        #[Target('githubApi2')]
        private HttpClientInterface $something,
    ) {
    }

But oups, I made a typo. I wrote githubApi2 instead of githubApi. But for Symfony it's OK. Instead it injects a new "blank" HttpClient (with no sane default configuration - no timeout for example).

IMHO, if one want to inject a specific target, the target must be validated. And if the target does not exist, because another developer renamed it, you want an exception. for this.

see this comments for a previous discussion.

But IMHO, if the naming is not exactly the same as the autowire alias, it's fine to not raise an exception. But in this very use case, you really want this alias. Not another one.

So I would expect an exception.

WDYT?

Example

No response
@lyrixx lyrixx changed the title Target Attribute must fail if the target does not exist [DependencyInjection] Target Attribute must fail if the target does not exist Dec 8, 2022
@nicolas-grekas
Copy link
Member

I agree! and the failure message should suggest the correct name.

@Korbeil
Copy link
Contributor

Korbeil commented Dec 8, 2022 •
edited
Loading

We recently had the issue in one of our apps. We spelled the Target incorrectly and it was fine in test env but in production it failed because we add headers in the scoped client. Having an error would have helped a lot there ! 👍

@nicolas-grekas nicolas-grekas added the Help wanted Issues and PRs which are looking for volunteers to complete them. label Dec 14, 2022
@nicolas-grekas
Copy link
Member

"Help wanted" label added!

@rodmen rodmen mentioned this issue Dec 19, 2022
Merged
chalasr pushed a commit to chalasr/symfony that referenced this issue Dec 23, 2022
@nicolas-grekas @chalasr
feature symfony#48707 [DependencyInjection] Target Attribute must fai…
9f78421 
…l if the target does not exist (rodmen)

This PR was merged into the 6.3 branch.

Discussion
----------

[DependencyInjection] Target Attribute must fail if the target does not exist

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix symfony#48555
| License       | MIT
| Doc PR        | -

This change checks if an attribute is a Target to not auto bind with a new default object if the target name does not exists.
The existent behavior should be in charge of suggest possible options for a typo on the Target name.

I can't mark `allow edit by maintainers` I think because I'm open the PR from a organization repo.

Commits
-------

983cd08 [DependencyInjection] Target Attribute must fail if the target does not exist
@kbond kbond mentioned this issue Mar 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
DependencyInjection Help wanted Issues and PRs which are looking for volunteers to complete them.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[DependencyInjection] Target Attribute must fail if the target does not exist
4 participants
@nicolas-grekas @lyrixx @Korbeil @xabbuh