Skip to content

[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

Merged
merged 1 commit into from
Dec 22, 2022
Merged

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

merged 1 commit into from
Dec 22, 2022

Conversation

rodmen
Copy link
Contributor

@rodmen rodmen commented Dec 19, 2022

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #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.

@carsonbot
Copy link

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

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 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]);

@rodmen
Copy link
Contributor Author

rodmen commented Dec 20, 2022

Thanks for tackling this! Here is my review as a diff. (TypeReference::getAttributes() should return a collection of PHP attributes.)

Thanks for the clarification @nicolas-grekas .
I think of it that way, but I preferred not to modify the Target class on my first try.
I've apply the suggested changes.

Regarding the if ($reference->getAttributes()) check in AutowirePass::getAutowiredReference, what do you think to pass $parameter->getAttributes() ([]ReflectionAttributes) as TypedReference::$attributes as create the TypedReference for that method using the object's class in the ID parameter?
That way, we allow you to use the attributes for other purposes if needed.

@rodmen rodmen requested review from nicolas-grekas and removed request for dunglas December 20, 2022 16:35
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 22, 2022

Regarding the if ($reference->getAttributes()) check in AutowirePass::getAutowiredReference

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?

@nicolas-grekas
Copy link
Member

(there's one test to fix though)

@rodmen
Copy link
Contributor Author

rodmen commented Dec 22, 2022

(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.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 22, 2022

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.
It might be great to figure out a way to allow this for future PRs.

@nicolas-grekas
Copy link
Member

Oh, and one last thing: please squash your PR so that it has only one commit.

@nicolas-grekas
Copy link
Member

Thank you @rodmen.

@nicolas-grekas nicolas-grekas merged commit e9870a4 into symfony:6.3 Dec 22, 2022
chalasr pushed a commit to chalasr/symfony that referenced this pull request Dec 23, 2022
…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
@fabpot fabpot mentioned this pull request May 1, 2023
fabpot added a commit that referenced this pull request Jul 13, 2023
…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:

![image](https://github.com/symfony/symfony/assets/243674/e93bb23d-67e5-4c6f-9972-2d388124a7d9)

- Improve `debug:autowiring` to display the target name to use with `#[Target]`:

![image](https://github.com/symfony/symfony/assets/243674/951f5dae-9584-4cae-b60f-736afa824da0)

Commits
-------

8d60a7e [DependencyInjection] Improve reporting named autowiring aliases
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 Attribute must fail if the target does not exist
3 participants