Skip to content

[Php80] Handle RenameClassRector+AnnotationToAttributeRector with auto import and existing attribute defined#5219

Merged
samsonasik merged 7 commits intomainfrom
rename-auto-iomport-attribute
Nov 2, 2023
Merged

[Php80] Handle RenameClassRector+AnnotationToAttributeRector with auto import and existing attribute defined#5219
samsonasik merged 7 commits intomainfrom
rename-auto-iomport-attribute

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@samsonasik samsonasik commented Nov 1, 2023

@jambonfarci @stefantalen @TomasVotruba

It seems the issue of:

is when there is already existing attribute defined (there is removed use statement)

when no attribute yet defined, it seems working ok (the use statement is replaced with new one)

it seems when rename a node, we need service to collect the named node need to be collected so it can be used on PostRector to only remove when there is renamed name equal.

Re-work :)

Fixes https://github.com/rectorphp/rector-symfony/issues/535

@samsonasik samsonasik marked this pull request as draft November 1, 2023 18:12
@samsonasik samsonasik marked this pull request as ready for review November 2, 2023 03:08
Comment on lines +67 to +72
if (! $result instanceof Name && ! $node instanceof FullyQualified) {
$phpAttributeName = $node->getAttribute(AttributeKey::PHP_ATTRIBUTE_NAME);
if (is_string($phpAttributeName)) {
return $this->classRenamer->renameNode(new FullyQualified($phpAttributeName, $node->getAttributes()), $oldToNewClasses, $scope);
}
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the trick 👍 , on ClassRenamingPostRector, verify that Name is not FullyQualified, which:

$this->phpAttributeGroupFactory->create()

can return a Name instead of FullyQualified with bring original attribute name to be renamed, then use it to be returned so it can be processed on next loop on FileProcessor.

@samsonasik
Copy link
Copy Markdown
Member Author

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

Sidenote for future improvement: The renamed Name and Identifier docblocks that renamed should be collected before removing use statements actually to avoid the similar issue happen again in the future, but that will need separate PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants