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

NormalizeNamespaceByPSR4ComposerAutoloadRector: Replace references correctly #2644

Closed
wants to merge 2 commits into from

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Jul 9, 2022

Previously, NormalizeNamespaceByPSR4ComposerAutoloadRector did not update references to other normalized classes properly. This change fixes that by recording the renames using RenamedClassesDataCollector.

Supersedes #2482

]);

$rectorConfig->rule(NormalizeNamespaceByPSR4ComposerAutoloadRector::class);
$rectorConfig->rule(ClassRenamingPostRector::class);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is needed but it does not work either way – it does not look like the ClassRenamingPostRector does anything to fully-qualified name references.

Copy link
Contributor Author

@jtojnar jtojnar Jul 9, 2022

Choose a reason for hiding this comment

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

Apparently, it is not necessary. And it is skipping the processing here:

$parentNode = $name->getAttribute(AttributeKey::PARENT_NODE);
if ($this->shouldSkip($newName, $name, $parentNode)) {
return null;

Copy link
Contributor Author

@jtojnar jtojnar Jul 9, 2022

Choose a reason for hiding this comment

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

Apparently, the $parentNode is null. Possibly FullyQualifyStmtsAnalyzer is removing the attribute?

Copy link
Member

Choose a reason for hiding this comment

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

FullyQualifyStmtsAnalyzer->process() produce create new FullyQualified instance, the attribute may need to be mirrored:

return new FullyQualified($name, $node->getAttributes());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even then, it still does not seem to work properly – only fixes references in one of the files (weird, they are structurally identical). Will need to do more digging.

Copy link
Member

Choose a reason for hiding this comment

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

Possibly, you can re-use RenamedClassesDataCollector. Add old -> new names there and it should be picked up in ClassRenamingPostRector and resolve this for you.

The post-Rector are run after all other rules, to post-process the final printer file code.

Copy link
Contributor Author

@jtojnar jtojnar Sep 20, 2022

Choose a reason for hiding this comment

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

I am using RenamedClassesDataCollector now and it still updates the references only in one of the files correctly but not the other. Possibly by that time the namespace of the referenced file is changed so the rename no longer finds it.

@jtojnar jtojnar force-pushed the better-psr4 branch 2 times, most recently from ad3d9ce to 0b961d2 Compare July 9, 2022 06:55
@TomasVotruba
Copy link
Member

Do you need help with this one? It seems almost finished 🙂

@jtojnar
Copy link
Contributor Author

jtojnar commented Aug 16, 2022

I have yet to find time to investigate why it still does not work. Not sure when I will be able to work on this.

@jtojnar jtojnar force-pushed the better-psr4 branch 6 times, most recently from 67ff423 to a399935 Compare September 20, 2022 17:34
It will be needed in `NormalizeNamespaceByPSR4ComposerAutoloadRector` next.
Additionally, make it use `ClassAnalyzer::isAnonymousClass` to cover more cases.
…rrectly

Previously, `NormalizeNamespaceByPSR4ComposerAutoloadRector` did not update
references to other normalized classes properly. This change fixes that
by recording the renames using `RenamedClassesDataCollector`.
@TomasVotruba
Copy link
Member

TomasVotruba commented Oct 13, 2022

@jtojnar We'd love to have this feature in Rector :)

What is missing here to make CI pass?

@jtojnar
Copy link
Contributor Author

jtojnar commented Oct 13, 2022

I would expect the code to work as is so there is something wrong with my assumptions.

Unfortunately, I still have not found a time to dive into Rector innards to learn how rules are interleaved, and how RenamedClassesDataCollector really interacts with namespaces that are modified.

@TomasVotruba
Copy link
Member

Closing as the buggy rule is now removed to avoid causing harm.

@jtojnar jtojnar deleted the better-psr4 branch June 4, 2023 18:38
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