chore(symfony-routing): replace Symfony route annotation by attribute one #557

mcsky · 2024-01-12T18:11:28Z

Integrate in Symfony upgrade the replacement of Symfony\Component\Routing\Annotation\Route by Symfony\Component\Routing\Attribute\Route

see this issue

Closes rectorphp/rector#8384

mcsky · 2024-01-12T18:12:03Z

(i'm going to add a test for this)

config/sets/symfony/symfony64.php

composer.json

samsonasik · 2024-01-12T18:47:01Z

config/sets/symfony/annotations-to-attributes.php

@@ -18,4 +19,10 @@
        // @see Symfony 5.2+ https://github.com/symfony/doctrine-bridge/commit/02d2cf4743331e6b69ffd1d68e09b7e2dc417201#diff-1a16e2739e51eab000116d0542bd0226cea59a6d64711740ed7ce14769f95d1b
        new AnnotationToAttribute('Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity'),
    ]);
+    $rectorConfig->ruleWithConfiguration(
+        RenameClassRector::class,
+        [


It seems not needed, just update the 2nd arg of AnnotationToAttribute arg to Symfony\Component\Routing\Attribute\Route above on line 18 should auto change to target attribute with new name

new AnnotationToAttribute('Symfony\Component\Routing\Annotation\Route', 'Symfony\Component\Routing\Attribute\Route'

test is needed tho

/cc @scuben @JohJohan could you verify that "Symfony\Component\Routing\Attribute\Route" class exists in specific symfony version when upgrading from "Symfony\Component\Routing\Annotation\Route" thank you

Ok, rename seems still needed if it "already an attribute", but with "old name".

For other AnnotationToAttribute usage, the target and rename seems needed for this case, but need to ensure that the target attribute class already exists on specific symfony version when attribute introduced

Symfony\Component\Routing\Attribute\Route is available since symfony 6.4 AFAIK

I'm going to have a look at your feedbacks when at home.

That's what I mean, if that target class introduced in later version, eg transforming annotation is start from symfony 5.2, while the new target attribute class only exists since 6.4, then that RenameClassRector need to be placed in specific setlists, the issue is, that need to be checked that rename attribute, not rename annotation

samsonasik · 2024-01-15T08:05:16Z

Ok, I am checking, the PHP 8 route attribute support exists since symfony 5.2, but the new class: "Symfony\Component\Routing\Attribute\Route" not yet exists, ref:

only the class: "Symfony\Component\Routing\Annotation\Route".

Then, start from symfony 6.4, the "Symfony\Component\Routing\Attribute\Route" exists,

https://github.com/symfony/symfony/blob/914c2880393fe31335e716809edfdf66beedd246/src/Symfony/Component/Routing/Attribute/Route.php#L26-L27

but the Annotation sub namespace is not deprecated, see

https://github.com/symfony/symfony/blob/914c2880393fe31335e716809edfdf66beedd246/src/Symfony/Component/Routing/Annotation/Route.php#L12-L16

so the original "Symfony\Component\Routing*Annotation*\Route" is still valid, which means the mapping is not necessarily needed.

If this is needed, to avoid invalid rename, as it only should be used in the attribute part, I think custom rule is needed which the step is:

create custom rule
verify if it is attribute in method, if match, rename
register the rule to symfony 6.4 setlist

https://github.com/rectorphp/rector-symfony/blob/main/config/sets/symfony/symfony64.php

which the "Symfony\Component\Routing\Attribute\Route" exists.

mcsky · 2024-01-15T21:14:50Z

I give it a try very soon, thanks for all the details !

mcsky · 2024-01-20T18:41:58Z

rules/Symfony64/Rector/Class_/FixImportWhileUsingRouteAttributeButAnnotationImportRector.php

+        if ($node instanceof Attribute && $node->name !== null && (string) $node->name === self::ANNOTATION_ROUTE) {
+            // there I'd like to modify the UseUse node representing my current node->name
+            // AFAIK the node I'm returning from this method is only the Attribute node
+            // maybe should I retrieve the global AST from somewhere and manually get the UseUse node to edit the value
+            // Or maybe there is a more elegant way to do that ?
+
+            return null;


I would like some help on this part. I think this if statement is enough to identify that the #[Route] attribute is used in the file.

But I'm stuck to modify the related UseUse node of this attribute. Have you any recommendations on this ?

No need to import to use statement, that will be taken care by : $rectorConfig->importNames() when enabled.

Oh I see what you mean, in that case, $rectorConfig->importNames() is working on RenameClassRector, but not this custom rector, you may need to lookup of what AnnotationToAttributeRector implementation, set Name node, and add Name node to import nodes collector to be imported later

You can use Rector\PostRector\Collector\UseNodesToAddCollector service for that, eg:

$this->useNodesToAddCollector->addUseImport(new FullyQualifiedObjectType($namespacedAttrName));

see https://github.com/rectorphp/rector-src/blob/663f526966313e0f178c088ea667119490404da9/rules/Php80/Rector/Property/NestedAnnotationToAttributeRector.php#L219

After some thinking, replace use statement directly is unnecessary on this case, if that needed, you can do:

$rectorConfig->importNames(); $rectorConfig->removeUnusedImports();

That will be safer imo, and that will be replaced.

The improvement for this case need to be verified carefully, and that need separate PR.

mcsky · 2024-01-20T18:58:20Z

@samsonasik I started an implementation of the behaviour you described but I'm stuck. I can't figure out how to modify the use statement related to the #[Route] attribute node

rules/Symfony64/Rector/Class_/FixImportWhileUsingRouteAttributeButAnnotationImportRector.php

samsonasik · 2024-01-21T09:54:17Z

rules/Symfony64/Rector/Class_/FixImportWhileUsingRouteAttributeButAnnotationImportRector.php

+
+    public function getNodeTypes(): array
+    {
+        return [Attribute::class];


use ClassMethod instead, and rename its attrGroups so it only change on ClassMethod

The #Route[] attribute can be specified on a method but also on a class in Symfony

then, you need to define in Class_ and ClassMethod

…by attribute one Replace Symfony\Component\Routing\Annotation\Route by Symfony\Component\Routing\Attribute\Route

samsonasik · 2024-01-21T11:46:36Z

...ngeRouteAttributeFromAnnotationSubnamespaceRector/Fixture/use-annotation-should-skip.php.inc

+}
+
+?>
+-----


The second part start from ----- can be removed if equal content

...ngeRouteAttributeFromAnnotationSubnamespaceRector/Fixture/use-annotation-should-skip.php.inc

samsonasik

Looks good, thank you

samsonasik · 2024-01-22T12:49:04Z

Thank you @mcsky

TomasVotruba · 2024-01-23T08:18:43Z

Thanks @mcsky 🙏

samsonasik reviewed Jan 12, 2024

View reviewed changes

config/sets/symfony/symfony64.php Outdated Show resolved Hide resolved

mcsky force-pushed the feat-rename-route-annotation-to-attribute branch from 845d002 to 9dcc10d Compare January 12, 2024 18:37

samsonasik reviewed Jan 12, 2024

View reviewed changes

composer.json Outdated Show resolved Hide resolved

mcsky force-pushed the feat-rename-route-annotation-to-attribute branch 2 times, most recently from 497bb68 to 1cba32c Compare January 12, 2024 18:39

samsonasik reviewed Jan 12, 2024

View reviewed changes

mcsky force-pushed the feat-rename-route-annotation-to-attribute branch 2 times, most recently from c0fc2eb to 2407797 Compare January 20, 2024 18:31

mcsky commented Jan 20, 2024

View reviewed changes

samsonasik reviewed Jan 20, 2024

View reviewed changes

rules/Symfony64/Rector/Class_/FixImportWhileUsingRouteAttributeButAnnotationImportRector.php Outdated Show resolved Hide resolved

samsonasik reviewed Jan 21, 2024

View reviewed changes

mcsky force-pushed the feat-rename-route-annotation-to-attribute branch from 2407797 to 2a86bfc Compare January 21, 2024 11:15

chore(symfony-routing): replace Symfony route annotation use import …

6c57a74

…by attribute one Replace Symfony\Component\Routing\Annotation\Route by Symfony\Component\Routing\Attribute\Route

mcsky force-pushed the feat-rename-route-annotation-to-attribute branch from 2a86bfc to 6c57a74 Compare January 21, 2024 11:18

samsonasik reviewed Jan 21, 2024

View reviewed changes

samsonasik reviewed Jan 22, 2024

View reviewed changes

...ngeRouteAttributeFromAnnotationSubnamespaceRector/Fixture/use-annotation-should-skip.php.inc Outdated Show resolved Hide resolved

remove 2nd part on skip fixture

bf81b9a

samsonasik approved these changes Jan 22, 2024

View reviewed changes

samsonasik changed the title ~~chore(symfony-routing): replace Symfony route annotation use import by attribute one~~ chore(symfony-routing): replace Symfony route annotation by attribute one Jan 22, 2024

samsonasik merged commit e52eac8 into rectorphp:main Jan 22, 2024
5 checks passed

samsonasik mentioned this pull request Jan 27, 2024

Symfony 7 rector rule collection #537

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(symfony-routing): replace Symfony route annotation by attribute one #557

chore(symfony-routing): replace Symfony route annotation by attribute one #557

mcsky commented Jan 12, 2024 •

edited by samsonasik

Loading

mcsky commented Jan 12, 2024

samsonasik Jan 12, 2024

samsonasik Jan 12, 2024

samsonasik Jan 12, 2024

mcsky Jan 12, 2024 •

edited

Loading

samsonasik Jan 12, 2024

samsonasik commented Jan 15, 2024 •

edited

Loading

mcsky commented Jan 15, 2024

mcsky Jan 20, 2024

samsonasik Jan 20, 2024

samsonasik Jan 21, 2024

samsonasik Jan 21, 2024

samsonasik Jan 21, 2024

mcsky commented Jan 20, 2024

samsonasik Jan 21, 2024 •

edited

Loading

mcsky Jan 21, 2024

samsonasik Jan 21, 2024

samsonasik Jan 21, 2024

samsonasik left a comment

samsonasik commented Jan 22, 2024

TomasVotruba commented Jan 23, 2024

+              }
+              ?>
+              -----

chore(symfony-routing): replace Symfony route annotation by attribute one #557

chore(symfony-routing): replace Symfony route annotation by attribute one #557

Conversation

mcsky commented Jan 12, 2024 • edited by samsonasik Loading

mcsky commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsky Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsonasik commented Jan 15, 2024 • edited Loading

mcsky commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsky commented Jan 20, 2024

samsonasik Jan 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsonasik left a comment

Choose a reason for hiding this comment

samsonasik commented Jan 22, 2024

TomasVotruba commented Jan 23, 2024

mcsky commented Jan 12, 2024 •

edited by samsonasik

Loading

mcsky Jan 12, 2024 •

edited

Loading

samsonasik commented Jan 15, 2024 •

edited

Loading

samsonasik Jan 21, 2024 •

edited

Loading