Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[HttpKernel] fix using Target attribute with controller arguments #45572

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 7, 2022

Conversation

kbond
Copy link
Member

@kbond kbond commented Feb 27, 2022

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #45021
License MIT
Doc PR n/a

This was supposed to fix #45021 but after adding what I suspected was a failing test, it works as expected. I guess I'm not testing the correct thing?

Here's a quick reproducer that shows the problem in a "real app":

class HomepageController extends AbstractController
{
    public function __construct(#[Target('request.logger')] private LoggerInterface $logger)
    {
    }

    #[Route('/', name: 'app_homepage')]
    public function index(#[Target('request.logger')] LoggerInterface $logger): Response
    {
        dd(
            $this->logger->getName(), // "request" (expected)
            $logger->getName()        // "app" (not-expected)
        );

        // ...
    }
}

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 6, 2022

The issue happens where there are no bindings, but only a named autowiring alias.
This "else" block should look for a Target attribute:

} else {
$target = ltrim($target, '\\');
$args[$p->name] = $type ? new TypedReference($target, $type, $invalidBehavior, $p->name) : new Reference($target, $invalidBehavior);
}

Please target 5.4 as this is a bugfix IMHO.

@kbond kbond force-pushed the controller-target-param branch 2 times, most recently from 66d2268 to 164aafc Compare March 7, 2022 13:26
@kbond kbond changed the base branch from 6.1 to 5.4 March 7, 2022 13:26
@kbond kbond modified the milestones: 6.1, 5.4 Mar 7, 2022
@kbond kbond force-pushed the controller-target-param branch from 164aafc to 448cc3a Compare March 7, 2022 13:28
@kbond
Copy link
Member Author

kbond commented Mar 7, 2022

I've changed to a 5.4 bug fix. Please confirm the test is done correctly.

@nicolas-grekas
Copy link
Member

Thank you @kbond.

@nicolas-grekas nicolas-grekas merged commit 2aa63d1 into symfony:5.4 Mar 7, 2022
@nicolas-grekas nicolas-grekas added Bug and removed Feature labels Mar 7, 2022
@kbond kbond deleted the controller-target-param branch March 7, 2022 15:32
This was referenced Apr 2, 2022
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.

[DependencyInjection] Target parameter attribute does not work for controller methods
3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.