-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] Target Attribute must fail if the target does not exist #48707
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
Conversation
It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you. Cheers! Carsonbot |
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.
Thanks for tackling this!
Here is my review as a diff.
(TypeReference::getAttributes()
should return a collection of PHP attributes.)
diff --git a/src/Symfony/Component/DependencyInjection/Attribute/Target.php b/src/Symfony/Component/DependencyInjection/Attribute/Target.php
index 7751b3813b..560f7fe2f8 100644
--- a/src/Symfony/Component/DependencyInjection/Attribute/Target.php
+++ b/src/Symfony/Component/DependencyInjection/Attribute/Target.php
@@ -28,13 +28,16 @@ final class Target
$this->name = lcfirst(str_replace(' ', '', ucwords(preg_replace('/[^a-zA-Z0-9\x7f-\xff]++/', ' ', $name))));
}
- public static function parseName(\ReflectionParameter $parameter): string
+ public static function parseName(\ReflectionParameter $parameter, self &$attribute = null): string
{
+ $attribute = null;
+
if (!$target = $parameter->getAttributes(self::class)[0] ?? null) {
return $parameter->name;
}
- $name = $target->newInstance()->name;
+ $attribute = $target->newInstance();
+ $name = $attribute->name;
if (!preg_match('/^[a-zA-Z_\x7f-\xff]/', $name)) {
if (($function = $parameter->getDeclaringFunction()) instanceof \ReflectionMethod) {
diff --git a/src/Symfony/Component/DependencyInjection/CHANGELOG.md b/src/Symfony/Component/DependencyInjection/CHANGELOG.md
index d058d01e14..74e04a5713 100644
--- a/src/Symfony/Component/DependencyInjection/CHANGELOG.md
+++ b/src/Symfony/Component/DependencyInjection/CHANGELOG.md
@@ -8,6 +8,7 @@ CHANGELOG
* Deprecate `PhpDumper` options `inline_factories_parameter` and `inline_class_loader_parameter`
* Add `RemoveBuildParametersPass`, which removes parameters starting with a dot during compilation
* Fail is Target attribute does not exists during compilation
+
6.2
---
diff --git a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
index 03f3bd4aeb..cdf3ed470b 100644
--- a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
+++ b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
@@ -110,7 +110,7 @@ class AutowirePass extends AbstractRecursivePass
return $this->processAttribute($attribute, ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $value->getInvalidBehavior());
}
- $value = new TypedReference($value->getType(), $value->getType(), $value->getInvalidBehavior(), $attribute->name);
+ $value = new TypedReference($value->getType(), $value->getType(), $value->getInvalidBehavior(), $attribute->name, [$attribute]);
}
if ($ref = $this->getAutowiredReference($value, true)) {
return $ref;
@@ -320,7 +320,9 @@ class AutowirePass extends AbstractRecursivePass
}
$getValue = function () use ($type, $parameter, $class, $method) {
- if (!$value = $this->getAutowiredReference($ref = new TypedReference($type, $type, ContainerBuilder::EXCEPTION_ON_INVALID_REFERENCE, Target::parseName($parameter), ['is_target_attr' => isset($parameter->getAttributes(Target::class)[0])]), false)) {
+ $name = Target::parseName($parameter, $target);
+
+ if (!$value = $this->getAutowiredReference($ref = new TypedReference($type, $type, ContainerBuilder::EXCEPTION_ON_INVALID_REFERENCE, $name, $target ? [$target] : []), false)) {
$failureMessage = $this->createTypeNotFoundMessageCallback($ref, sprintf('argument "$%s" of method "%s()"', $parameter->name, $class !== $this->currentId ? $class . '::' . $method : $method));
if ($parameter->isDefaultValueAvailable()) {
$value = clone $this->defaultArgument;
@@ -407,7 +409,7 @@ class AutowirePass extends AbstractRecursivePass
}
}
- if ($reference->getAttributes()['is_target_attr'] ?? false && !$this->container->has($name)) {
+ if ($reference->getAttributes()) {
return null;
}
}
@@ -534,9 +536,9 @@ class AutowirePass extends AbstractRecursivePass
}
$message = sprintf('has type "%s" but this class %s.', $type, $parentMsg ? sprintf('is missing a parent class (%s)', $parentMsg) : 'was not found');
- } elseif ($reference->getAttributes()['is_target_attr'] ?? false) {
+ } elseif ($reference->getAttributes()) {
$message = $label;
- $label = sprintf('"#[Target(\'%s\')" on ', $reference->getName());
+ $label = sprintf('"#[Target(\'%s\')" on', $reference->getName());
} else {
$alternatives = $this->createTypeAlternatives($this->container, $reference);
$message = $this->container->has($type) ? 'this service is abstract' : 'no such service exists';
diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
index 0039496d72..ace42c97b8 100644
--- a/src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
+++ b/src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
@@ -178,7 +178,7 @@ class ResolveBindingsPass extends AbstractRecursivePass
$typeHint = ltrim(ProxyHelper::exportType($parameter) ?? '', '?');
- $name = Target::parseName($parameter);
+ $name = Target::parseName($parameter, $target);
if ($typeHint && \array_key_exists($k = preg_replace('/(^|[(|&])\\\\/', '\1', $typeHint).' $'.$name, $bindings)) {
$arguments[$key] = $this->getBindingValue($bindings[$k]);
src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
Outdated
Show resolved
Hide resolved
Thanks for the clarification @nicolas-grekas . Regarding the |
src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
Outdated
Show resolved
Hide resolved
I'm not sure that the situation you anticipate can exist so I wouldn't bother until it does. BTW, it looks to me that the change near L110 that I proposed previously is needed. Is there a reason why you didn't borrow it? |
src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
Outdated
Show resolved
Hide resolved
(there's one test to fix though) |
Sorry @nicolas-grekas , I'm not realized on the approve and just pushed a commit for the test. |
One more change from https://fabbot.io/report/symfony/symfony/48707/9562119c360e8f00379181700559fbd7f053dc9c Note that I would have happily made those simple changes myself but since I can't push to your fork, I must rely on you do to them. |
Oh, and one last thing: please squash your PR so that it has only one commit. |
src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
Outdated
Show resolved
Hide resolved
Thank you @rodmen. |
…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
…ng aliases (nicolas-grekas) This PR was merged into the 6.4 branch. Discussion ---------- [DependencyInjection] Improve reporting named autowiring aliases | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - This PR started as a fix of #48707 to improve the error message reported when `#[Target]` is used with an invalid target, and ended up with many other related improvements: - Allow using `#[Target]` with no arguments. This has the effect of throwing an exception if the name of the argument that has the attribute doesn't match a named autowiring alias. - Improve the error message when a target doesn't match:  - Improve `debug:autowiring` to display the target name to use with `#[Target]`:  Commits ------- 8d60a7e [DependencyInjection] Improve reporting named autowiring aliases
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.