[DependencyInjection] deprecate the @required annotation

[alexislefebvre]

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

TODO:

• check the name of the package which should be reported in the deprecation message:
  • symfony/framework-bundle (that call addGlobalIgnoredName('required'))?
  • and/or
  • symfony/dependency-injection (that detect and is called when there is this annotation)?
• fix or remove the test of the deprecation? $this->expectDeprecation doesn't work as I expected

[alexislefebvre]

Thanks for the reviews! Suggestions have been proceeded.

[alexislefebvre]

I tried to improve the deprecation by showing the source of the deprecation, and fixed a bug that reported a deprecation when using #[Required].

Hopefully I fixed the tests, I duplicated some of them, in order to test @required and #[Required], but I kept since the PR was growing quickly. Do we need to cover each of these scenarios?

[nicolas-grekas]

The @group legacy annotation should be added only on test cases that should be removed on v7. I don't think that's the case right now.
Instead of adding it to many test cases, we should update them to use the attribute, and ensure we keep at least one test ensuring that the annotation still works.

[alexislefebvre]

I didn't know how to decide if only one test with @required was enough, so I duplicated tests to use @required annotation or #[Required]attribute. This should prevent BC breaks for 6.3 and the future 6.4.

But please let me know if some cases can be dropped.

[nicolas-grekas]

On second look, this misses at least the following patch. It's not enought because then you'll see

  4x: Since symfony/dependency-injection 6.3: The "@required" annotation on methods is deprecated, use the "Symfony\Contracts\Service\Attribute\Required" attribute instead.
    2x in AutowirePassTest::testExplicitMethodInjectionAttribute from Symfony\Component\DependencyInjection\Tests\Compiler
    2x in AutowireRequiredMethodsPassTest::testExplicitMethodInjectionAttribute from Symfony\Component\DependencyInjection\Tests\Compiler

But this is legit: testExplicitMethodInjectionAttribute is currently using SetterInjection::class, which is itself deprecated. The test needs to be updated to not use any deprecated class.

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/AutowireRequiredMethodsPass.php b/src/Symfony/Component/DependencyInjection/Compiler/AutowireRequiredMethodsPass.php
index 53ae32af85..bf1954a00e 100644
--- a/src/Symfony/Component/DependencyInjection/Compiler/AutowireRequiredMethodsPass.php
+++ b/src/Symfony/Component/DependencyInjection/Compiler/AutowireRequiredMethodsPass.php
@@ -69,6 +69,7 @@ class AutowireRequiredMethodsPass extends AbstractRecursivePass
                     if (false === stripos($doc, '@inheritdoc') || !preg_match('#(?:^/\*\*|\n\s*+\*)\s*+(?:\{@inheritdoc\}|@inheritdoc)(?:\s|\*/$)#i', $doc)) {
                         break;
                     }
+                    trigger_deprecation('symfony/dependency-injection', '6.3', 'The "@required" annotation on methods is deprecated, use the "Symfony\Contracts\Service\Attribute\Required" attribute instead.');
                 }
                 try {
                     $r = $r->getPrototype();
diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php
index 42ce6c6ee1..34ff97ce1e 100644
--- a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php
@@ -947,13 +947,9 @@ class AutowirePassTest extends TestCase
 
     /**
      * @dataProvider provideNotWireableCalls
-     *
-     * @group legacy
      */
     public function testNotWireableCalls($method, $expectedMsg)
     {
-        $this->expectDeprecation('Since symfony/dependency-injection 6.3: The "@required" annotation on methods is deprecated, use the "Symfony\Contracts\Service\Attribute\Required" attribute instead.');
-
         $container = new ContainerBuilder();
 
         $foo = $container->register('foo', NotWireable::class)->setAutowired(true)
@@ -986,13 +982,8 @@ class AutowirePassTest extends TestCase
         ];
     }
 
-    /**
-     * @group legacy
-     */
     public function testSuggestRegisteredServicesWithSimilarCase()
     {
-        $this->expectDeprecation('Since symfony/dependency-injection 6.3: The "@required" annotation on methods is deprecated, use the "Symfony\Contracts\Service\Attribute\Required" attribute instead.');
-
         $container = new ContainerBuilder();
 
         $container->register(LesTilleuls::class, LesTilleuls::class);
diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/autowiring_classes.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/autowiring_classes.php
index 3083be42c2..4a2c1f457f 100644
--- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/autowiring_classes.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/autowiring_classes.php
@@ -446,8 +446,7 @@ class NotWireable
     {
     }
 
-    // @deprecated since Symfony 6.3, to be removed in 7.0
-    /** @required */
+    #[Required]
     protected function setProtectedMethod(A $a)
     {
     }

[alexislefebvre]

@nicolas-grekas thanks for the detailed report! I applied these suggestions.

In the first code block of your last comment (in …/Compiler/AutowireRequiredMethodsPass.php), we add a trigger_deprecation after the 2 checks on @required and @inheritdoc have been performed. It looks like it will show a false positive? The deprecation will be triggered even if @required wasn't found.

[stof]

this one looks wrong to me. It will trigger the deprecation for any method using @inheritdoc, which is totally wrong

[alexislefebvre]

I had the same question: #48990 (comment)

[alexislefebvre]

Status: Needs Review

[nicolas-grekas]

Thank you @alexislefebvre.