-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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] Fix "Cannot replace arguments" errors caused by ResolveAutowireInlineAttributesPass #54908
Conversation
nicolas-grekas
commented
May 13, 2024
Q | A |
---|---|
Branch? | 7.1 |
Bug fix? | yes |
New feature? | no |
Deprecations? | no |
Issues | Fix #54823 |
License | MIT |
… ResolveAutowireInlineAttributesPass
@@ -52,18 +52,13 @@ protected function processValue(mixed $value, bool $isRoot = false): mixed | |||
} | |||
} | |||
|
|||
$dummy = $value; | |||
while (null === $dummy->getClass() && $dummy instanceof ChildDefinition) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is dead code: definitions with no class are already skipped before (and that's legit for child definitions too)
@@ -89,19 +84,14 @@ private function registerAutowireInlineAttributes(\ReflectionFunctionAbstract $m | |||
if ($method->isVariadic()) { | |||
array_pop($parameters); | |||
} | |||
$dummyContainer = new ContainerBuilder($this->container->getParameterBag()); | |||
$paramResolverContainer = new ContainerBuilder($this->container->getParameterBag()); | |||
|
|||
foreach ($parameters as $index => $parameter) { | |||
if ($isChildDefinition) { | |||
$index = 'index_'.$index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming of indexes for child definitions is what caused the issue.
The patch fixes this using named arguments instead, which means using Definition::setArgument() instead of replaceArgument() when resolving child definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the issue and the test is perfect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch fixes the issue for me. Thank you!
Thank you Nicolas for taking care of this regression. |