Skip to content
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

[AutoImport] [Renaming] Skip remove used use statement on annotation during rename + auto import when no replacement on auto import #5168

Merged
merged 3 commits into from Oct 15, 2023

Conversation

samsonasik
Copy link
Member

Added test or testing skip remove use statement part of rename during auto import.

The rename annotation is allowed only on specific:

  • symfony assert
  • doctrine
  • and serializer

see

$this->processAssertChoiceTagValueNode($oldToNewClasses, $phpDocInfo, $hasChanged);
$this->processDoctrineRelationTagValueNode($node, $oldToNewClasses, $phpDocInfo, $hasChanged);
$this->processSerializerTypeTagValueNode($oldToNewClasses, $phpDocInfo, $hasChanged);

see

on auto import, the process is overlapped, which annotation not renamed, but use statement removed during auto import.

@samsonasik samsonasik changed the title Add test for skip IsGranted annotation during rename + auto import Add test for skip remove use statement on IsGranted annotation during rename + auto import Oct 14, 2023
@samsonasik
Copy link
Member Author

@jambonfarci @stefantalen it fixed by skip remove use statement when no replacement during auto import cd3ee41 🎉

@samsonasik samsonasik changed the title Add test for skip remove use statement on IsGranted annotation during rename + auto import [AutoImport] [Renaming] Skip remove use statement on IsGranted annotation during rename + auto import when no replacement on auto import Oct 15, 2023
Comment on lines +51 to +54
// nothing to remove, as no replacement
if ($useImportTypes === []) {
return $use;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

on auto import enabled, when nothing to replace, the removing use statement should not happen, as it cause invalid removal on rename process.

Comment on lines -34 to +38
$classRenamingPostRector,
// priority: 600
$nameImportingPostRector,
// priority: 500
// priority: 600
$useAddingPostRector,
// priority: 500
$classRenamingPostRector,
Copy link
Member Author

Choose a reason for hiding this comment

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

the re-index to set name importing and use adding post rector first is on purpose to know that auto import collecting the statements that need to be added, which class renaming depends.

@samsonasik samsonasik changed the title [AutoImport] [Renaming] Skip remove use statement on IsGranted annotation during rename + auto import when no replacement on auto import [AutoImport] [Renaming] Skip remove used use statement on IsGranted annotation during rename + auto import when no replacement on auto import Oct 15, 2023
@samsonasik samsonasik changed the title [AutoImport] [Renaming] Skip remove used use statement on IsGranted annotation during rename + auto import when no replacement on auto import [AutoImport] [Renaming] Skip remove used use statement on annotation during rename + auto import when no replacement on auto import Oct 15, 2023
@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba I am merging it to have faster feedback to test ;)

@samsonasik samsonasik merged commit 29370c7 into main Oct 15, 2023
41 checks passed
@samsonasik samsonasik deleted the add-test-535 branch October 15, 2023 03:36
@TomasVotruba
Copy link
Member

👍

@samsonasik
Copy link
Member Author

@TomasVotruba @jambonfarci @stefantalen I tested in laminas-servicemanager-migration usage on auto import usage, it seems cause error:

There were 5 failures:

1) LaminasTest\ServiceManager\Migration\Rector\RenameClassRector\AutoImportTest::test with data set #0 ('/Users/samsonasik/www/laminas...hp.inc')
Failed on fixture file "factory_invoke_typehint.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 
 namespace LaminasTest\ServiceManager\Migration\Rector\RenameClassRector\FixtureAutoImport;
 
-use Psr\Container\ContainerInterface;
+use Interop\Container\ContainerInterface;
 
 class FactoryInvokeTypehint
 {
-    public function __invoke(ContainerInterface $container)
+    public function __invoke(\Psr\Container\ContainerInterface $container)

which auto import should be allowed, this definitely need rework, I will check if the original allow change SpacelessPhpDocTagNode on PR:

can be solution, but on consequency, rector-symfony list need update for AnnotationToAttribute to use targetted:

Symfony\Component\Security\Http\Attribute\IsGranted

instead of

Sensio\Bundle\FrameworkExtraBundle\Configuration\IsGranted

https://github.com/rectorphp/rector-symfony/blob/873458ca73a73f9becd1680963a347bbcb8e5f43/config/sets/sensiolabs/annotations-to-attributes.php#L14

Give me time :)

samsonasik added a commit that referenced this pull request Oct 16, 2023
…otation during rename + auto import when no replacement on auto import (#5168)"

This reverts commit 29370c7.
@samsonasik
Copy link
Member Author

Reverting at #5179

samsonasik added a commit that referenced this pull request Oct 16, 2023
* Revert "[DX] Sync order of PostFileProcessor dependencies (#5176)"

This reverts commit 4f50ad5.

* Revert "[PostRector] Fix ClassRenamingPostRector return when no auto import replacement (#5175)"

This reverts commit d14ec5e.

* Revert "[PostRector] Reduce loop on ClassRenamingPostRector (#5174)"

This reverts commit a9908f6.

* Revert "[CodingStyle] Clean up check last name on UseImportsRemover (#5173)"

This reverts commit c5d3a0e.

* Revert "[AutoImport] [Renaming] Skip remove used use statement on annotation during rename + auto import when no replacement on auto import (#5168)"

This reverts commit 29370c7.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants