8000 [DependencyInjection] deprecate the `@required` annotation by alexislefebvre · Pull Request #48990 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DependencyInjection] deprecate the @required annotation #48990

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 10, 2023
Merged

[DependencyInjection] deprecate the @required annotation #48990

merged 1 commit into from
Mar 10, 2023

Conversation

alexislefebvre
Copy link
Contributor
@alexislefebvre alexislefebvre commented Jan 16, 2023 •
edited
Loading
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

@carsonb 8000 ot carsonbot added this to the 6.3 milestone Jan 16, 2023
@alexislefebvre alexislefebvre force-pushed the 6.3-deprecate-the-at-required-annotation branch 2 times, most recently from fbad3f3 to 43c1d66 Compare January 16, 2023 00:33
@alexislefebvre alexislefebvre force-pushed the 6.3-deprecate-the-at-required-annotation branch from 43c1d66 to dcf42d3 Compare January 16, 2023 10:56
@OskarStark OskarStark changed the title [DependencyInjection] deprecate the "@required" annotation [DependencyInjection] deprecate the @required annotation Jan 16, 2023
@alexislefebvre
Copy link
Contributor Author

Thanks for the reviews! Suggestions have been proceeded.

@alexislefebvre alexislefebvre force-pushed the 6.3-deprecate-the-at-required-annotation branch 2 times, most recently from 70b6c7d to 7c0b7ba Compare January 16, 2023 13:41
@alexislefebvre
Copy link
Contributor Author

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?

@alexislefebvre alexislefebvre force-pushed the 6.3-deprecate-the-at-required-annotation branch 3 times, most recently from 9cb1ec0 to 65100a3 Compare January 18, 2023 00:39
nicolas-grekas
nicolas-grekas reviewed Jan 18, 2023
View reviewed changes
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. 8000

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 alexislefebvre force-pushed the 6.3-deprecate-the-at-required-annotation branch from 65100a3 to 55e55dc Compare January 19, 2023 15:52
@alexislefebvre alexislefebvre force-pushed the 6.3-deprecate-the-at-required-annotation branch 3 times, most recently from 495e11b to ab4386c Compare January 19, 2023 16:14
@alexislefebvre
Copy link
Contributor Author
alexislefebvre commented Jan 19, 2023 •
edited
Loading

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.

@alexislefebvre alexislefebvre force-pushed the 6.3-deprecate-the-at-required-annotation branch from e1191b1 to 85b4f9e Compare January 25, 2023 13:49
nicolas-grekas
nicolas-grekas requested changes Jan 26, 2023
View reviewed changes
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.

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
Copy link
Contributor Author

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

@alexislefebvre alexislefebvre force-pushed the 6.3-deprecate-the-at-required-annotation branch from e3bca72 to bcc0e19 Compare January 30, 2023 10:47
stof
stof reviewed Jan 30, 2023
View reviewed changes
src/Symfony/Component/DependencyInjection/Compiler/AutowireRequiredMethodsPass.php Outdated
@@ -67,6 +69,7 @@ protected function processValue(mixed $value, bool $isRoot = false): mixed
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.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor 6D40 Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same question: #48990 (comment)

@alexislefebvre alexislefebvre force-pushed the 6.3-deprecate-the-at-required-annotation branch from 5f25ae8 to 06ad958 Compare February 13, 2023 15:51
@alex 9E88 islefebvre
Copy link
Contributor Author

Status: Needs Review

@alexislefebvre alexislefebvre mentioned this pull request Feb 14, 2023
Closed
@nicolas-grekas nicolas-grekas force-pushed the 6.3-deprecate-the-at-required-annotation branch from 06ad958 to 9d69e4b Compare March 10, 2023 16:23
@nicolas-grekas
Copy link
Member

Thank you @alexislefebvre.

@nicolas-grekas nicolas-grekas merged commit c8920e9 into symfony:6.3 Mar 10, 2023
@nicolas-grekas nicolas-grekas mentioned this pull request Mar 10, 2023
Closed
@alexislefebvre alexislefebvre deleted the 6.3-deprecate-the-at-required-annotation branch March 10, 2023 17:00
@alexislefebvre alexislefebvre mentioned this pull request Apr 6, 2023
Merged
fabpot added a commit that referenced this pull request Apr 8, 2023
@fabpot
minor #49963 Update the upgrade guide for the deprecation of ``@requi…
8a36deb 
…red`` (alexislefebvre)

This PR was merged into the 6.3 branch.

Discussion
----------

Update the upgrade guide for the deprecation of ``@required``

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | .
| License       | MIT
| Doc PR        | .

Follow-up of #48990

Commits
-------

092d889 UPGRADE-6.3.md: deprecate "`@required`"
@fabpot fabpot mentioned this pull request May 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@OskarStark OskarStark OskarStark left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas requested changes

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
DependencyInjection Deprecation Feature Status: Needs Review
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.

[RFC][DependencyInjection] Deprecate the “@required” annotation
6 participants
@alexislefebvre @nicolas-grekas @stof @OskarStark @derrabus @carsonbot
0